Rewrite how marks are stored & add reflow #16937

zadjii-msft · 2024-03-26T13:42:46Z

This is pretty much a huge refactoring of how marks are stored in the buffer.

Gone is the list of ScrollMarks in the buffer that store regions of text as points marking the ends. Those would be nigh impossible to reflow nicely.

Instead, we're going to use TextAttributes to store the kind of output we've got - Prompt, Command, Output, or, the default, None. Those already reflow nicely!

But we also need to store things like, the exit code for the command. That's why we've now added ScrollbarData to ROWs. There's really only going to be one prompt->output on a single row. So, we only need to store one ScrollbarData per-row. When a command ends, we can just go update the mark on the row that started that command.

But iterating over the whole buffer to find the next/previous prompt/command/output region sounds complicated. So, to avoid everyone needing to do some variant of that, we've added MarkExtents (which is literally just the same mark structure as before). TextBuffer can figure out where all the mark regions are, and hand that back to callers. This allows ControlCore to be basically unchanged.

But collecting up all the regions for all the marks sounds expensive! We need to update the scrollbar frequently, we can't just collect those up every time! No we can't! But we also don't need to. The scrollbar doesn't need to know where all the marks start and end and if they have commands and this and that - no. We only need to know the rows that have marks on them. So, we've now also got ScrollMark to represent just a mark on a scrollbar at a specific row on the buffer. We can get those quickly.

I added a bunch of tests for this.
I played with it and it feels good, even after a reflow (finally)
See:
- megathread: Scrollbar Marks #11000
- Enable marks for Stable builds #15057 (I'm not marking this as closed. The stacked PR will close this, when I move marks to Stable)

…d replace with a text attribute

I'm totally just going to hand spans back out of the text buffer. genius.

…minal

src/buffer/out/Marks.hpp

src/buffer/out/textBuffer.cpp

…flow-marks

DHowett

10/22, but submitting to give you a chance to think about these before I continue

DHowett · 2024-04-03T15:13:41Z

src/buffer/out/Marks.hpp

+    std::pair<til::point, til::point> GetExtent() const
+    {
+        til::point realEnd{ til::coalesce_value(outputEnd, commandEnd, end) };
+        return std::make_pair(til::point{ start }, realEnd);


Suggested change

return std::make_pair(til::point{ start }, realEnd);

return std::make_pair(start, realEnd);

q: is this necessary? start is already the right type

DHowett · 2024-04-03T15:17:23Z

src/buffer/out/Row.cpp

+
+void ROW::EndOutput(std::optional<unsigned int> error) noexcept
+{
+    if (_promptData.has_value())


gut check me - EndOutput may happen on a line without a prompt, right? Do we care about those instances?

I mean, technically, yes. But I think the one caller of this (TextBuffer::EndOutput) goes up and actually does find the right row

DHowett · 2024-04-03T15:19:39Z

src/buffer/out/Row.hpp

@@ -299,6 +305,8 @@ class ROW final
    bool _wrapForced = false;
    // Occurs when the user runs out of text to support a double byte character and we're forced to the next line
    bool _doubleBytePadded = false;
+
+    std::optional<ScrollbarData> _promptData = std::nullopt;


@lhecker this will grow ROW by 12+ bytes (considering padding for alignment after ScrollbarData::category, and the size of the flag in an optional), for a total of 108k for a full buffer. Is this OK?

@zadjii-msft if you want to save the cost of this in conhost, you can use the preprocessor version of TIL_FEATURE for this and the implementation bodies (fwiw)

Something else we discussed:

struct ScrollbarData { til::color color; uint32_t exitCode = 0; MarkCategory category{ MarkCategory::Default }; bool valid = false; bool hasColor = false; bool hasExitCode = false; }; struct ROW { // ... ScrollbarData _promptData; };

to avoid some overhead

Honestly I think optional is about as optimal as you can get; it's pretty much a struct { uninitialized<T> _value; bool valid; } already, but with some packing magic to make it so that alignment such-and-such works out

While I'd agree that std::optional is optimal in keeping data uninitialized, I'd find it difficult to agree that it's optimal when it comes to its memory layout. point, color and so on for instance all double in size when wrapped in an optional.

The current ScrollbarData struct is 20 bytes large, whereas the one above is 12 bytes large. Due to ROW's necessary 16 byte alignment this effectively results in 20 bytes of padding per ROW.

MarkExtents' struct size for instance is 60 bytes, compared to just 28 if we didn't use optional. 54% of the struct is just empty space.

DHowett · 2024-04-03T15:20:20Z

src/buffer/out/TextAttribute.hpp

@@ -38,6 +38,16 @@ enum class UnderlineStyle
    Max = DashedUnderlined
 };

+// We only need a few bits, but uint8_t is two bytes anyways, and apparently


Suggested change

// We only need a few bits, but uint8_t is two bytes anyways, and apparently

// We only need a few bits, but due to padding uint8_t occupies two bytes anyways, and apparently

fwiw since uint8_t is unequivocally one byte, we should explain why it's two ^

I'm gonna leave it as a u16 unfortunately due to the std::has_unique_object_representations_v thing

src/buffer/out/TextAttribute.hpp

src/buffer/out/textBuffer.hpp

DHowett · 2024-04-03T15:25:23Z

src/cascadia/UnitTests_Control/ControlCoreTests.cpp

@@ -502,7 +502,7 @@ namespace ControlUnitTests
            const auto& start = core->_terminal->GetSelectionAnchor();
            const auto& end = core->_terminal->GetSelectionEnd();
            const til::point expectedStart{ 24, 0 }; // The character after the prompt
-            const til::point expectedEnd{ 29, 3 }; // x = buffer.right
+            const til::point expectedEnd{ 21, 3 }; // x = the end of the text


whoa, what happened to cause this change?

DHowett · 2024-04-03T15:27:07Z

Looking at how we're storing marks now, it makes me wonder if we need the FTCS flushing mechanism at all - now that they're TextAttributes, can conpty emit them? Is that ~ ~ weird ~ ~ or wrong?

I'm assuming Leonard wants to say "no, please stop rendering things with conpty" but it's only 2024 😄

src/cascadia/TerminalCore/Terminal.cpp

lhecker · 2024-04-04T16:34:15Z

src/cascadia/TerminalCore/Terminal.cpp

+{
+    // We want to return _no_ marks when we're in the alt buffer, to effectively
+    // hide them.
+    return _inAltBuffer() ? std::vector<ScrollMark>{} : std::move(_activeBuffer().GetMarkRows());


nit: Don't (usually) std::move a return value. Otherwise RVO becomes impossible. Same thing below.

src/buffer/out/textBuffer.cpp

lhecker · 2024-04-04T16:57:32Z

src/buffer/out/textBuffer.cpp

+                // Otherwise, we've changed from any state -> any state, and it doesn't really matter.
+                if (markKind != MarkKind::None)


Due to the earlier check this will always be true.

lhecker · 2024-04-04T17:00:23Z

src/buffer/out/textBuffer.cpp

+
+            if (markKind != MarkKind::None)
+            {
+                lastMarkedText = { nextX, y };


It seems to me as if we could move this down to the lastMarkKind = markKind; which would IMO be easier to read.

We could also use a switch/case instead of a series of if conditions. An explicit MarkKind::None could then ensure that we don't set the lastMarkKind and lastMarkedText by doing something like...

case MarkKind::None: x = nextX; continue;

lhecker · 2024-04-04T17:01:36Z

src/buffer/out/textBuffer.cpp

+        }
+        lastPromptY = promptY;
+    }
+    std::reverse(commands.begin(), commands.end());


You can avoid reversing here if you iterate forwards through the buffer instead, right?

DHowett

I think I'm comfortable enough with this!

lhecker · 2024-04-05T13:06:18Z

src/buffer/out/Row.cpp

+}
+void ROW::SetScrollbarData(std::optional<ScrollbarData> data) noexcept
+{
+    _promptData = data;


The _promptData doesn't get reset in Reset(), which is called during buffer circling. Due to this I believe this code will start failing to work once the buffer is full.

If I'm correct, then simply adding it to Reset() should fix it.

(This is my only remaining important concern. The memory layout is IMO not optimal for perf, but we can noodle on that later.)

lhecker · 2024-04-05T13:41:10Z

src/buffer/out/Row.cpp

+        _promptData = ScrollbarData{
+            .category = MarkCategory::Prompt,
+            .color = std::nullopt,
+            .exitCode = std::nullopt,
+        };


FYI, and I'm writing this just because I think it's interesting, as I mentioned elsewhere, optional is really only good at keeping stuff uninitialized. It's not really consistently great at initializing things, however.

As Dustin said, an optional is basically

template<typename T> struct optional { union { T data }; // union keeps it uninitialized even if it has a constructor bool initialized = false; };

The problem with optional is that its constructor

template < class U = T > constexpr optional( U&& value );

takes the initial value by reference, std::moves it into data and sets initialized to true. This only works great if the compiler happens to figure out how to optimize it. This doesn't work consistently in any compiler, but MSVC in particular also happens to be really REALLY bad at avoiding unnecessary struct copies.

This means that constructing a ScrollbarData like you do looks like this:

mov BYTE PTR [rcx], 4 ; .category = 4 mov BYTE PTR [rcx+8], 0 ; .color.initialized = false mov BYTE PTR [rcx+16], 0 ; .exitCode.initialized = false

However, constructing a std::optional<ScrollbarData> for _promptData looks like this:

; $T1 == the current function stack mov BYTE PTR $T1[rsp], 4 ; stack: .category = 4 mov BYTE PTR $T1[rsp+8], 0 ; stack: .color.initialized = false movups xmm0, XMMWORD PTR $T1[rsp] ; stack -> register mov BYTE PTR $T1[rsp+16], 0 ; stack: .exitCode.initialized = false mov eax, DWORD PTR $T1[rsp+16] ; stack -> register movups XMMWORD PTR [rcx], xmm0 ; register -> ROW (.category/.color) mov DWORD PTR [rcx+16], eax ; register -> ROW (.exitCode) mov BYTE PTR [rcx+20], 1 ; ._promptData.initialized = true

In other words, MSVC constructs the ScrollbarData on stack, then copies that into register, and then copies it again into ROW. Other compilers don't do this exact thing, but they're doing similar dumb stuff under similar circumstances.

However, fret not, because the C++ consortium has solved this problem ~~by making improvements to the language like introducing proper union types~~ by adding another overload to std::optional which can be used here:

template< class... Args > constexpr explicit optional( std::in_place_t, Args&&... args );

std::optional<ScrollbarData>{ std::in_place, MarkCategory::Prompt, std::nullopt, std::nullopt, };

Just kidding:

mov BYTE PTR $T1[rsp], 4 mov BYTE PTR $T1[rsp+8], 0 movups xmm0, XMMWORD PTR $T1[rsp] mov BYTE PTR $T1[rsp+16], 0 mov BYTE PTR $T1[rsp+20], 1 movsd xmm1, QWORD PTR $T1[rsp+16] movups XMMWORD PTR [rcx], xmm0 movsd QWORD PTR [rcx+16], xmm1

This occurs because fixing the construction doesn't fix the existence of its move constructor. That one is still implemented in the STL and thus relies on the compiler optimizing it which no compiler does consistently correct.

The actual proper fix is to use this:

if (!_promptData.has_value()) { _promptData.emplace(MarkCategory::Prompt); }

which results in this:

cmp BYTE PTR [rcx+20], 0 ; _promptData.has_value() jne SHORT $LN8@test ; if (!...) goto ret; mov BYTE PTR [rcx], 4 ; .category = 4 mov BYTE PTR [rcx+8], 0 ; .color.initialized = false mov BYTE PTR [rcx+16], 0 ; .exitCode.initialized = false mov BYTE PTR [rcx+20], 1 ; ._promptData.initialized = true $LN8@test: ret 0

While using emplace fixes the issue here, my point is mainly that optional is not particularly great for performance nor memory usage. It may work, but only with careful use, as demonstrated above.

lhecker · 2024-04-05T13:42:17Z

src/buffer/out/textBuffer.cpp

+    auto attr = GetCurrentAttributes();
+    attr.SetMarkAttributes(MarkKind::Output);
+    SetCurrentAttributes(attr);


nit: You can just do

_currentAttributes.SetMarkAttributes(MarkKind::Output);

here and similar above.

lhecker · 2024-04-05T13:44:17Z

src/host/getset.cpp

+            const auto inConpty{ g.getConsoleInformation().IsInVtIoMode() };
+            const auto resizeQuirk{ g.getConsoleInformation().GetVtIo()->IsResizeQuirkEnabled() };
+            if (!(inConpty && resizeQuirk))


FWIW this change is not quite the same as it was before as it prevents the compiler from shortcutting the latter call. An alternative way to write this may be this:

if (!g.getConsoleInformation().IsInVtIoMode() || !g.getConsoleInformation().GetVtIo()->IsResizeQuirkEnabled()) {

ah sorry, this was for debugging.

…flow-marks

lhecker · 2024-04-05T18:58:48Z

src/cascadia/TerminalCore/Terminal.cpp

+    const auto currentNewPos = _mainBuffer->GetCursor().GetPosition();
+    if (currentNewPos.y < proposedTop)
+    {
+        _mainBuffer->GetCursor().SetPosition(til::point{ currentNewPos.x, proposedTop });
+    }


I believe I once read somewhere in our repository that adjusting the viewport to fit the cursor is much better than adjusting the cursor. I'm not sure what the reason was back then, but I can see how a later write should always continue where a previous one left off.

Instead I would suggest moving this up to line 390 and do this (untested):

proposedTop = std::min(proposedTop, newCursorPos.y);

* Switches the marks feature to being stable. * Renames the settings, to remove "experimental." Stacked on top of #16937, so that we actually finish reflow before merging this. Closes #15057

This is fallout from #16937. * Typing a command then backspacing the chars then asking for suggestions would think the current commandline ended with spaces, making filtering very hard. * The currently typed command would _also_ appear in the command history, which isn't useful. I actually did TDD for this and wrote the test first, then confirmed again running through the build script, I wasn't hitting any of the earlier issues. Closes #17241 Closes #17243

This is fallout from #16937. * Typing a command then backspacing the chars then asking for suggestions would think the current commandline ended with spaces, making filtering very hard. * The currently typed command would _also_ appear in the command history, which isn't useful. I actually did TDD for this and wrote the test first, then confirmed again running through the build script, I wasn't hitting any of the earlier issues. Closes #17241 Closes #17243 (cherry picked from commit bf8a647) Service-Card-Id: 92638413 Service-Version: 1.21

zadjii-msft added 20 commits March 22, 2024 12:21

Starting from just the bottom, remove ScrollMarks from TextBuffer, an…

80a0ffb

…d replace with a text attribute

tests are starting to feel good

f88ee30

this is what you call test driven development

0bfe603

well, Terminal.cpp compiles. That's something

ed79c94

This sorta compiles, but with huge gaps in features.

ec1eb90

I'm totally just going to hand spans back out of the text buffer. genius.

HOLY SHIT THE TESTS ALL PASS

ef86ff0

roundtrip tests, but these still don't actually fix the issues in Ter…

65f7b34

…minal

okay the tests aren't self-consistent but I think I'm getting closer

d0aaf82

closer

0b39a61

did this really work?

3aa16d9

Yea I'm okay with these test changes

c28f8c0

rename, so we can have simple marks for optimized scrollbars

c6e5934

hey it worked for non-shell-integration CMD

e8aa067

no error code is different than 0

73cd1b3

start killing lots of old code

68f8994

some code cleanup

f1ff258

re-add marks via the UI

bc5a638

is this really everything?

5f0933e

oh, yea, we should reflow them.

f78dec3

last bits of cleanup

6afd08f

github-advanced-security bot found potential problems Mar 26, 2024

View reviewed changes

src/buffer/out/Marks.hpp Fixed Show fixed Hide fixed

src/buffer/out/textBuffer.cpp Fixed Show fixed Hide fixed

src/buffer/out/textBuffer.cpp Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

zadjii-msft added 2 commits March 26, 2024 09:16

spel

ca50d96

audit, and I forgot to update a test

ad3e599

zadjii-msft mentioned this pull request Mar 26, 2024

Make marks a stable feature #16938

Merged

zadjii-msft added 2 commits March 27, 2024 06:40

actually fix build

43b64f4

Merge remote-tracking branch 'origin/main' into dev/migrie/b/15057-re…

43d6fe9

…flow-marks

zadjii-msft mentioned this pull request Apr 3, 2024

Add Spec for Quick Fix #17005

Merged

DHowett reviewed Apr 3, 2024

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 4, 2024

lhecker reviewed Apr 4, 2024

View reviewed changes

a TextBufferTest for @DHowett

11d82a2

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 4, 2024

I agree with all this strongly

3cf6a3c

DHowett approved these changes Apr 5, 2024

View reviewed changes

lhecker reviewed Apr 5, 2024

View reviewed changes

zadjii-msft added 2 commits April 5, 2024 13:32

a very weird reflow bug?

e0fa1c3

last nits

4da4d1f

lhecker mentioned this pull request Apr 5, 2024

ResizeWithReflow doesn't clamp _virtualBottom if the cursor is above the viewport #17023

Open

Merge remote-tracking branch 'origin/main' into dev/migrie/b/15057-re…

5617282

…flow-marks

lhecker reviewed Apr 5, 2024

View reviewed changes

zadjii-msft added 2 commits April 5, 2024 14:28

this is a noop

351208b

do what leonard suggested instead

dc21f31

lhecker approved these changes Apr 5, 2024

View reviewed changes

zadjii-msft enabled auto-merge April 5, 2024 19:54

DHowett changed the title ~~Enable reflowing marks~~ Rewrite how marks are stored & add reflow Apr 5, 2024

DHowett disabled auto-merge April 5, 2024 19:54

DHowett enabled auto-merge April 5, 2024 19:54

DHowett approved these changes Apr 5, 2024

View reviewed changes

DHowett added this pull request to the merge queue Apr 5, 2024

Merged via the queue into main with commit c3f44f7 Apr 5, 2024
20 checks passed

DHowett deleted the dev/migrie/b/15057-reflow-marks branch April 5, 2024 20:45

This was referenced Apr 25, 2024

[1.21] clearing marks seems to have regressed #17130

Closed

[1.21] Clicking on the right side of a combobox doesn't cause it to open / dropdown #17062

Closed

zadjii-msft mentioned this pull request May 10, 2024

Clean up command history context passed to suggestions UI #17245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite how marks are stored & add reflow #16937

Rewrite how marks are stored & add reflow #16937

zadjii-msft commented Mar 26, 2024

This comment has been minimized.

DHowett left a comment

DHowett Apr 3, 2024

DHowett Apr 3, 2024

zadjii-msft Apr 3, 2024

DHowett Apr 3, 2024

zadjii-msft Apr 3, 2024

DHowett Apr 3, 2024

lhecker Apr 4, 2024

DHowett Apr 3, 2024

zadjii-msft Apr 4, 2024

DHowett Apr 3, 2024

DHowett commented Apr 3, 2024

lhecker Apr 4, 2024

lhecker Apr 4, 2024

lhecker Apr 4, 2024

lhecker Apr 4, 2024

DHowett left a comment

lhecker Apr 5, 2024 •

edited

Loading

lhecker Apr 5, 2024

lhecker Apr 5, 2024 •

edited

Loading

lhecker Apr 5, 2024

lhecker Apr 5, 2024

zadjii-msft Apr 5, 2024

lhecker Apr 5, 2024 •

edited

Loading

	return std::make_pair(til::point{ start }, realEnd);
	return std::make_pair(start, realEnd);

	// We only need a few bits, but uint8_t is two bytes anyways, and apparently
	// We only need a few bits, but due to padding uint8_t occupies two bytes anyways, and apparently

		// Otherwise, we've changed from any state -> any state, and it doesn't really matter.
		if (markKind != MarkKind::None)

Rewrite how marks are stored & add reflow #16937

Rewrite how marks are stored & add reflow #16937

Conversation

zadjii-msft commented Mar 26, 2024

This comment has been minimized.

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

lhecker Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

lhecker Apr 5, 2024 •

edited

Loading

lhecker Apr 5, 2024 •

edited

Loading

lhecker Apr 5, 2024 •

edited

Loading