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

Initialize rows lazily #15524

Merged
merged 3 commits into from
Jun 10, 2023

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jun 7, 2023

For a 120x9001 terminal, a01500f reduced the private working set of
conhost by roughly 0.7MB, presumably due to tighter ROW packing, but
also increased it by 2.1MB due to the addition of the _charOffsets
array on each ROW instance. An option to fix this would be to only
allocate a _charOffsets if the first wide or complex Unicode glyph
is encountered. But on one hand this would be quite western-centric
and unfairly hurt most languages that exist and on another we can get
rid of the _charOffsets array entirely in the future by injecting
ZWNJs if a write begins with a combining glyph and just recount each
row from the start. That's still faster than fragmented memory.

This commit goes a different way and instead reduces the working
set of conhost after it launches from 7MB down to just 2MB,
by only committing ROWs when they're first used.

Finally, it adds a "scratchpad" row which can be used to build
more complex contents, for instance to horizontally scroll them.

Validation Steps Performed

  • Traditional resize
    • Horizontal shrinking works ✅
    • Vertical shrinking works ✅ and cursor stays in the viewport ✅
  • Reflow works ✅
  • Filling the buffer with ASCII works ✅ and no leaks ✅
  • Filling the buffer with complex Unicode works ✅ and no leaks ✅
  • ^[[3J erases scrollback ✅
  • Test ScrollRows with a positive delta ✅
  • I don't know how to test Reset. ❔ Unit tests use it though

@lhecker lhecker added Product-Conhost For issues in the Console codebase Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) labels Jun 7, 2023
@lhecker lhecker force-pushed the dev/lhecker/buffer-lazy-commit branch 2 times, most recently from c7b8783 to 11c2215 Compare June 7, 2023 18:04
@lhecker lhecker force-pushed the dev/lhecker/buffer-lazy-commit branch 2 times, most recently from d74e8fb to f2934ad Compare June 8, 2023 11:26
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

you answered this comment really, so i'm gonna submit it to get it out of my queue

@@ -495,7 +630,7 @@ void TextBuffer::_SetWrapOnCurrentRow() noexcept
// - fSet - True if this row has a wrap. False otherwise.
//Return Value:
// - <none>
void TextBuffer::_AdjustWrapOnCurrentRow(const bool fSet) noexcept
void TextBuffer::_AdjustWrapOnCurrentRow(const bool fSet)
Copy link
Member

Choose a reason for hiding this comment

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

gut feeling: the current row should not need to be faulted in, and so should not be exposed to a commit exception. Is that a faulty understanding on my part?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is possible for the current row to fault if you use CUP (or whatever) to go somewhere that hasn't been previously accessed or rendered yet (which is also possible since the renderer is async). We should decide whether we want to crash under OOM. It'd be a valid strategy. Alternatively we can strategically make use of a hypothetical GetRowByOffsetNoInit function that returns nullptr when faulting. I would personally not prefer it because it makes the code more complex (= "is it safe to use NoInit here? etc.), but I'm absolutely open to implement either change.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Wow, love how much simpler some of this is!

Honestly, once you gave me an explanation of _commitWatermark, everything kinda just made sense. I suggest adding a big blurb at the top of textBuffer.hpp or near the _commitWatermark declaration. That'll help a ton.

I'm gonna approve, but I really do think the _commitWatermark comment will help. Nice work!

src/buffer/out/textBuffer.hpp Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Show resolved Hide resolved
step = -1;
}

for (; y != end; y += step)
Copy link
Member

Choose a reason for hiding this comment

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

I don't completely understand why this doesn't have the same problem as memcpy. Depending on the direction you are moving, we need to copy end-to-start or start-to-end, right?

Copy link
Member

Choose a reason for hiding this comment

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

oh, i see. we are. this is just a generic loop that goes both ways. Oi.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you prefer we can make it 2 separate loops! I initially had 2 loops, and wrote this single loop because I was curious if I could.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Deeply horrified that we are placement-new-ing and placement-delete-ing ROWs inline in the buffer, but honestly at this point I would expect nothing less than that.

I've read the code over a couple times and I think I trust it. It scares me, but I trust it. Let's go!

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

// Retrieves a row from the buffer by its offset from the first row of the text buffer
// (what corresponds to the top row of the screen buffer).
ROW& TextBuffer::GetRowByOffset(const til::CoordType index)
Copy link
Member

Choose a reason for hiding this comment

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

for height=10, GetRowByOffset(1) and GetRowByOffset(11) are the same... but there are measurably not 11 rows in the buffer.

This isn't something to worry about, right?

Copy link
Member Author

@lhecker lhecker Jun 9, 2023

Choose a reason for hiding this comment

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

Yeah I think that's actually good, because it makes writing code that loops around simpler and safer. For instance, when you accidentally scroll (= rotate) more rows than there are in the buffer. The result will be weird, but it'll not crash and behave consistently.

Edit: Also, just double checking, it used to work the same way before this PR right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it's even preferable if you scroll less rows than there are in the buffer. For instance, let's say you want to copy the viewport contents of height 4 starting row 8. In your example it'll just copy row 8,9,10,11 = 8,9,0,1 and it'll not require the caller to perform any modulo or similar logic. It should just work. That's why I added the underflow / negative modulo if condition as well. It makes copying/scrolling rows backwards work correctly without extra logic.

@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 10, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot deleted the dev/lhecker/buffer-lazy-commit branch June 10, 2023 13:17
auto& row = _getRowByOffsetDirect(0);
row.Reset(_currentAttributes);
return row;
return _getRowByOffsetDirect(0);
Copy link
Member

Choose a reason for hiding this comment

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

wait, this wasn't explained in the new commit. why did it change? is this meaningful? the comment above the function is incorrect now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry, I forgot to update the comment. I made the change because I wasn't sure what I'd need. Over the weekend I figured that out though, so in the PR I just opened I've fixed this. #15541

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants