Replace MarkKind::None with MarkKind::Output #18291
Conversation
lhecker
added
Area-Performance
| endThisMark(lastMarkedText.x, lastMarkedText.y); | ||
| } | ||
| // Otherwise, we've changed from any state -> any state, and it doesn't really matter. | ||
| lastMarkKind = markKind; |
There was a problem hiding this comment.
This lastMarkKind variable is only ever written to but never read.
The condition above, if (markKind != MarkKind::None), also seems wrong to me. Is it supposed to test lastMarkKind?
carlos-zamora
left a comment
carlos-zamora
left a comment
There was a problem hiding this comment.
Looks good to me. Just mildly concerned about this comment, but since we're changing the default value to "Output", this should probably work the same way as before.
| endThisMark(lastMarkedText.x, lastMarkedText.y); | ||
| } | ||
| // Otherwise, we've changed from any state -> any state, and it doesn't really matter. | ||
| lastMarkKind = markKind; |
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
okay I haven't read the code yet, but I did check out the branch, and this definitely broke selecting output.
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
If you enable shell integrations, and then also enable the right-click context menu, that's usually the easiest way. Though the |
| const auto markKind{ attr.GetMarkAttributes() }; | ||
|
|
||
| if (markKind != MarkKind::None) | ||
| if (markKind != MarkKind::Output) |
There was a problem hiding this comment.
You know, entirely removing this conditional seems to work, form 1 minute of playing with it
|
I'll just bump the attributes vector size from 1 to 3. That seems easier to me until we have differential compression of attributes. That is, instead of storing run-lengths, store changes-between-lengths. Removing the charOffset vector from ROW will be much more effective for reducing the ROW size than caring about the attribute vector IMO. |
microsoft-github-policy-service
bot
added
Needs-Attention
An alternative approach for #18291. Improves perf by ~7%.
During the development of #18290 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.
Validation Steps Performed