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

Rewrite how marks are stored & add reflow #16937

Merged
merged 33 commits into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
80a0ffb
Starting from just the bottom, remove ScrollMarks from TextBuffer, an…
zadjii-msft Mar 22, 2024
f88ee30
tests are starting to feel good
zadjii-msft Mar 22, 2024
0bfe603
this is what you call test driven development
zadjii-msft Mar 22, 2024
ed79c94
well, Terminal.cpp compiles. That's something
zadjii-msft Mar 22, 2024
ec1eb90
This sorta compiles, but with huge gaps in features.
zadjii-msft Mar 22, 2024
ef86ff0
HOLY SHIT THE TESTS ALL PASS
zadjii-msft Mar 22, 2024
65f7b34
roundtrip tests, but these still don't actually fix the issues in Ter…
zadjii-msft Mar 25, 2024
d0aaf82
okay the tests aren't self-consistent but I think I'm getting closer
zadjii-msft Mar 25, 2024
0b39a61
closer
zadjii-msft Mar 25, 2024
3aa16d9
did this really work?
zadjii-msft Mar 25, 2024
c28f8c0
Yea I'm okay with these test changes
zadjii-msft Mar 25, 2024
c6e5934
rename, so we can have simple marks for optimized scrollbars
zadjii-msft Mar 25, 2024
e8aa067
hey it worked for non-shell-integration CMD
zadjii-msft Mar 25, 2024
73cd1b3
no error code is different than 0
zadjii-msft Mar 25, 2024
68f8994
start killing lots of old code
zadjii-msft Mar 25, 2024
f1ff258
some code cleanup
zadjii-msft Mar 25, 2024
bc5a638
re-add marks via the UI
zadjii-msft Mar 26, 2024
5f0933e
is this really everything?
zadjii-msft Mar 26, 2024
f78dec3
oh, yea, we should reflow them.
zadjii-msft Mar 26, 2024
6afd08f
last bits of cleanup
zadjii-msft Mar 26, 2024
ca50d96
spel
zadjii-msft Mar 26, 2024
ad3e599
audit, and I forgot to update a test
zadjii-msft Mar 26, 2024
43b64f4
actually fix build
zadjii-msft Mar 27, 2024
43d6fe9
Merge remote-tracking branch 'origin/main' into dev/migrie/b/15057-re…
zadjii-msft Mar 27, 2024
9f792fa
Merge remote-tracking branch 'origin/main' into dev/migrie/b/15057-re…
zadjii-msft Apr 3, 2024
e573896
I think we all learned something here about storing state in two places
zadjii-msft Apr 4, 2024
11d82a2
a TextBufferTest for @dhowett
zadjii-msft Apr 4, 2024
3cf6a3c
I agree with all this strongly
zadjii-msft Apr 4, 2024
e0fa1c3
a very weird reflow bug?
zadjii-msft Apr 5, 2024
4da4d1f
last nits
zadjii-msft Apr 5, 2024
5617282
Merge remote-tracking branch 'origin/main' into dev/migrie/b/15057-re…
zadjii-msft Apr 5, 2024
351208b
this is a noop
zadjii-msft Apr 5, 2024
dc21f31
do what leonard suggested instead
zadjii-msft Apr 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions src/buffer/out/Marks.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.

Module Name:
- marks.hpp

Abstract:
- Definitions for types that are used for "scroll marks" and shell integration
in the buffer.
- Scroll marks are identified by the existence of "ScrollbarData" on a ROW in the buffer.
- Shell integration will then also markup the buffer with special
TextAttributes, to identify regions of text as the Prompt, the Command, the
Output, etc.
- MarkExtents are used to abstract away those regions of text, so a caller
doesn't need to iterate over the buffer themselves.
--*/

#pragma once

enum class MarkCategory : uint8_t
{
Default = 0,
Error = 1,
Warning = 2,
Success = 3,
Prompt = 4
};

// This is the data that's stored on each ROW, to suggest that there's something
// interesting on this row to show in the scrollbar. Also used in conjunction
// with shell integration - when a prompt is added through shell integration,
// we'll also add a scrollbar mark as a quick "bookmark" to the start of that
// command.
struct ScrollbarData
{
MarkCategory category{ MarkCategory::Default };

// Scrollbar marks may have been given a color, or not.
std::optional<til::color> color;

// Prompts without an exit code haven't had a matching FTCS CommandEnd
// called yet. Any value other than 0 is an error.
std::optional<uint32_t> exitCode;
// Future consideration: stick the literal command as a string on here, if
// we were given it with the 633;E sequence.
};

// Helper struct for describing the bounds of a command and it's output,
// * The Prompt is between the start & end
// * The Command is between the end & commandEnd
// * The Output is between the commandEnd & outputEnd
//
// These are not actually stored in the buffer. The buffer can produce them for
// callers, to make reasoning about regions of the buffer easier.
struct MarkExtents
{
// Data from the row
ScrollbarData data;

til::point start;
til::point end; // exclusive
std::optional<til::point> commandEnd;
std::optional<til::point> outputEnd;

// MarkCategory category{ MarkCategory::Info };
// Other things we may want to think about in the future are listed in
// GH#11000

bool HasCommand() const noexcept
{
return commandEnd.has_value() && *commandEnd != end;
}
bool HasOutput() const noexcept
{
return outputEnd.has_value() && *outputEnd != *commandEnd;
}
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);
Copy link
Member

Choose a reason for hiding this comment

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

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

}
};

// Another helper, for when callers would like to know just about the data of
// the scrollbar, but don't actually need all the extents of prompts.
struct ScrollMark
{
til::CoordType row{ 0 };
ScrollbarData data;
};
33 changes: 33 additions & 0 deletions src/buffer/out/Row.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1181,3 +1181,36 @@ CharToColumnMapper ROW::_createCharToColumnMapper(ptrdiff_t offset) const noexce
const auto guessedColumn = gsl::narrow_cast<til::CoordType>(clamp(offset, 0, _columnCount));
return CharToColumnMapper{ _chars.data(), _charOffsets.data(), lastChar, guessedColumn };
}

const std::optional<ScrollbarData>& ROW::GetScrollbarData() const noexcept
{
return _promptData;
}
void ROW::SetScrollbarData(std::optional<ScrollbarData> data) noexcept
{
_promptData = data;
Copy link
Member

@lhecker lhecker Apr 5, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

}

void ROW::StartPrompt() noexcept
{
if (!_promptData.has_value())
{
_promptData = ScrollbarData{
.category = MarkCategory::Prompt,
.color = std::nullopt,
.exitCode = std::nullopt,
};
Copy link
Member

@lhecker lhecker Apr 5, 2024

Choose a reason for hiding this comment

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

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.

}
}

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

{
_promptData->exitCode = error;
if (error.has_value())
{
_promptData->category = *error == 0 ? MarkCategory::Success : MarkCategory::Error;
}
}
}
8 changes: 8 additions & 0 deletions src/buffer/out/Row.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "LineRendition.hpp"
#include "OutputCell.hpp"
#include "OutputCellIterator.hpp"
#include "Marks.hpp"

class ROW;
class TextBuffer;
Expand Down Expand Up @@ -167,6 +168,11 @@ class ROW final
auto AttrBegin() const noexcept { return _attr.begin(); }
auto AttrEnd() const noexcept { return _attr.end(); }

const std::optional<ScrollbarData>& GetScrollbarData() const noexcept;
void SetScrollbarData(std::optional<ScrollbarData> data) noexcept;
void StartPrompt() noexcept;
void EndOutput(std::optional<unsigned int> error) noexcept;

#ifdef UNIT_TESTING
friend constexpr bool operator==(const ROW& a, const ROW& b) noexcept;
friend class RowTests;
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

@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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

};

#ifdef UNIT_TESTING
Expand Down
3 changes: 2 additions & 1 deletion src/buffer/out/TextAttribute.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

// Keeping TextColor compact helps us keeping TextAttribute compact,
// which in turn ensures that our buffer memory usage is low.
static_assert(sizeof(TextAttribute) == 16);
static_assert(sizeof(TextAttribute) == 18);
static_assert(alignof(TextAttribute) == 2);
// Ensure that we can memcpy() and memmove() the struct for performance.
static_assert(std::is_trivially_copyable_v<TextAttribute>);
Expand Down Expand Up @@ -434,4 +434,5 @@ void TextAttribute::SetStandardErase() noexcept
{
_attrs = CharacterAttributes::Normal;
_hyperlinkId = 0;
_markKind = MarkKind::None;
}
32 changes: 28 additions & 4 deletions src/buffer/out/TextAttribute.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ enum class UnderlineStyle
Max = DashedUnderlined
};

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

Choose a reason for hiding this comment

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

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 ^

Copy link
Member Author

Choose a reason for hiding this comment

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

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

// doesn't satisfy std::has_unique_object_representations_v
enum class MarkKind : uint16_t
{
None = 0,
Prompt = 1,
Command = 2,
Output = 3,
};

class TextAttribute final
{
public:
Expand All @@ -46,7 +56,8 @@ class TextAttribute final
_foreground{},
_background{},
_hyperlinkId{ 0 },
_underlineColor{}
_underlineColor{},
_markKind{ MarkKind::None }
{
}

Expand All @@ -55,7 +66,8 @@ class TextAttribute final
_foreground{ gsl::at(s_legacyForegroundColorMap, wLegacyAttr & FG_ATTRS) },
_background{ gsl::at(s_legacyBackgroundColorMap, (wLegacyAttr & BG_ATTRS) >> 4) },
_hyperlinkId{ 0 },
_underlineColor{}
_underlineColor{},
_markKind{ MarkKind::None }
{
}

Expand All @@ -66,7 +78,8 @@ class TextAttribute final
_foreground{ rgbForeground },
_background{ rgbBackground },
_hyperlinkId{ 0 },
_underlineColor{ rgbUnderline }
_underlineColor{ rgbUnderline },
_markKind{ MarkKind::None }
{
}

Expand All @@ -75,7 +88,8 @@ class TextAttribute final
_foreground{ foreground },
_background{ background },
_hyperlinkId{ hyperlinkId },
_underlineColor{ underlineColor }
_underlineColor{ underlineColor },
_markKind{ MarkKind::None }
{
}

Expand Down Expand Up @@ -135,6 +149,15 @@ class TextAttribute final
return _attrs;
}

constexpr void SetMarkAttributes(const MarkKind attrs) noexcept
{
_markKind = attrs;
}
constexpr MarkKind GetMarkAttributes() const noexcept
{
return _markKind;
}

bool IsHyperlink() const noexcept;

TextColor GetForeground() const noexcept;
Expand Down Expand Up @@ -202,6 +225,7 @@ class TextAttribute final
TextColor _foreground; // sizeof: 4, alignof: 1
TextColor _background; // sizeof: 4, alignof: 1
TextColor _underlineColor; // sizeof: 4, alignof: 1
MarkKind _markKind; // sizeof: 2, cause I guess a uint8_t is 2B?
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved

#ifdef UNIT_TESTING
friend class TextBufferTests;
Expand Down
Loading
Loading