Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can we have infinite scrollback? #18290

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Dec 6, 2024

Mom: We have infinite scrollback at home.
Infinite scrollback at home:

...very much finite. How do you define "infinite" scrollback anyway? Does it entail downloading RAM? It's a mystery.
So, this PR adds support for 1B rows of scrollback, because that's >600GB of memory, which makes it effectively unusable, since we currently use a line buffer that is not well suited for fast clearing of many rows (1-2GB/s only). But it's enough to have a lot of finity.

Thanks to the raised limit, this PR sets the default history size from 9001 to 64Ki which is roughly 43MB of memory. The amount could be cut in half once we remove the charOffsets cache on each ROW. We could consider using 32Ki for now and only doubling it once we did this improvement.

During the development I noticed that we caused a performance regression due to the storage of marks in attributes. Since the default attribute is None and any output sets it to Output, this meant that every row of output would at least temporarily allocate heap memory. This caused cls on a huge history to take twice as long as before.

Related to #1410

Validation Steps Performed

  • TBD

This comment has been minimized.

void _decommit() noexcept;
void _construct(const std::byte* until) noexcept;
void _destroy() const noexcept;
void _commit(const uint8_t* row);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm why these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so that cls releases the memory again. I figured that this would become much more important once the scrollback can actually become quite large.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you meant std::byte vs. uint8_t. We only use the former in a teeny tiny subset of the codebase, of which this usage here is half of it. By using the more traditional uint8_t we make our code simpler and need less specialization. It's also more predictable, because primitive integers have received compiler optimizations over decades whereas std::byte hasn't.
I'm reminded of that one blog post that showed how poorly std::byte was optimized by compilers at the time, but I was unable to find it while writing this comment.

@lhecker lhecker marked this pull request as draft December 6, 2024 23:33
@lhecker lhecker force-pushed the dev/lhecker/1410-large-scrollback branch from 5500606 to e6acccf Compare December 11, 2024 20:34
const auto wroteWholeBuffer = lengthToWrite == (currentBufferDimensions.area<size_t>());
const auto startedAtOrigin = startingCoordinate == til::point{ 0, 0 };
const auto wroteSpaces = attribute == (FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED);
const auto currentBufferDimensions{ OutContext.GetBufferSize().Dimensions() };
Copy link
Member Author

@lhecker lhecker Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Suppress whitespace differences. My goal here is to make cls work properly in regular conhost.)

@lhecker lhecker force-pushed the dev/lhecker/1410-large-scrollback branch from e6acccf to 3567827 Compare December 11, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants