ConPTY: Optimize SGR sequences by combining them into a single sequence

[zadjii-msft]

I wonder if there's an optimization we can take later that'll let us compress SGRs together when we know we're writing a bunch. \x1b[40m+\x1b[31m+\x1b[4m+\x1b[3m => \x1b[40;31;4;3m?

Originally posted by @DHowett-MSFT in #2917

[DHowett-MSFT]

One complication: we need to know the cancel-out combos (4, 24) and not emit crazy things that turn the state off and on in the same sequence.

[egmontkob]

Apologies again that I'm commenting without fully understanding the context. What's the source of the data that you want to optimize this way?

we need to know the cancel-out combos (4, 24) and not emit crazy things that turn the state off and on in the same sequence

If the source is some in-memory array of TextAttributes or so then this cannot happen, can it? If a cell isn't underlined but the next one is, you emit SGR 4, if it's the other way around, you emit SGR 24, but you never want to emit both of them next to each other.

If the source is a sequence of SGRs, then {

Would this particular case with cancel-out combos be a problem? I don't think so. \e[4;24m must also be interpreted from left to right, subsequent ones overriding the preceding ones, i.e. equivalent to \e[4m\e[24m or simply \e[24m.

However, it's easy to shoot yourself in the foot when you enter the territory of often misinterpreted and/or not (yet) supported escape sequences. E.g. \e[38m\e[5;3m is an invalid sequence (to be ignored) followed by enabling blinking and italics. But \e[38;5;3m is a commonly understood sequence for switching to the 3rd color of the 256-color palette (although according to the standard the subparameters should be separated by : rather than ;, and it's unclear whether that should be the case between 38 and 5 too, but it's another topic – in practice usually ; is used).

when we know we're writing a bunch.

I'm afraid this sounds like the behavior would start depending on the timing of incoming data, and/or some heuristics, making it much harder to reproduce and debug potential issues.

Also comes with additional code complexity, and along with it possibly additional runtime cost (detecting such situations), just to shave off 2 bytes on a local communication channel (if I understand the architecture correctly), and maybe (but not necessarily) make subsequent parsing of this data a tiny bit faster. Depending on many circumstances unbeknownst to me, it might result in an overall performance penalty, but even if that's not the case, it might easily come with pointless code complexity and engineering efforts not well spent – let alone the aforementioned problem of combining invalid (or just simply unknown, unrecognized) sequences into a valid one.

}

I hope my comment contained at least some usable parts :)

[zadjii-msft]

@egmontkob This kinda gets at the core difference between ConPTY and a true pty. With a true pty it's more like a dumb pipe, the pty isn't generating any sequences of its own, it's just forwarding them along from client to terminal.

With conpty however, that's not really the case. While yes, some applications (like WSL) are using VT sequences to interact with the "terminal", the majority of client apps are using the win32 Console API to interact with the "terminal". Conpty is then doing its own work to make sure that the state changed by the API is also kept in sync with the terminal, who's only reading a stream of VT sequences generated by conpty itself.

In this particular case, as we're "rendering" a string of text (and colors) to the terminal (from conpty), we might decide that we need to both change the foreground to red and enable the underline. Currently, we'll add a \x1[31m and a \x1b[4m to the "frame" to change this state. What we're suggesting doing here is that when we add multiple SGR sequences in a row like this, that we combine all the sequences into a single sequence like \x1b[32;4m.

Since conpty is fully in charge of the sequences generated here, I think we can pretty safely assume that we won't be accidentally combining a \x1b[38m, \x1b[5m and \x1b[3m into a \x1b[38;5;3m, simply because we know we;ll never be sending a \x1b[38m all by itself 😄

This would also be something that wouldn't happen in passthrough mode (#1173) at all, because in that scenario we would be acting as a dumb pipe.

[egmontkob]

Thanks a lot for the clarification! I've just rewatched the relevant part of the WT announcement video to make sure I have the right architecture in mind.

So the first "if" branch of my previous comment was the relevant one, i.e. I'm not sure how cancel-out combos like (4, 24) were feared to appear at all in the first place. Anyway, it's definitely something to remember during developing this feature, just in case.

If optimizing matters, you might also do some guesswork whether sending the actual diff (turning off certain modes only) or building up from scratch after a reset is shorter; e.g. from bold+underlined+italic to italic should you do 22;24 (turn off bold and underlined) or 0;3 (turn off everything and then re-enable italic)? The latter approach could become pretty costly with 256-color or truecolor entries when it's not the color that changes. However, when reverting to the default attributes, it's probably better to just reset them all as \e[m rather than turning them off one by one.

[DHowett-MSFT]

@egmontkob oh, you're totally right. I forgot to consider that if we're rendering from the buffer, we'll never have conflicting states stored. 😄

[zadjii-msft]

Other notes from the PR:

@miniksa:

Nnnn... what happens in the scenario where:

* We're attempting to set Italics, Blink, and Invisible.

* Italics succeeds

* Blink fails

* Invisible would have succeeded, but we early returned...

Are we OK with that happening or should we have a provision to try the others and aggregate the result code later?

I don't think there's really a way for us to make this atomic where they all work or none of them work... or is there?

EDIT: I'm worried that the potentially torn state will result in a weird debugging situation in the future...

@zadjii-msft

You're right that's it's possible that we'd end in a bad state if one of these fails. Looking at the implementation of these, this is the only spot that can actually fail:

    try
    {
        _buffer.append(str);
        return S_OK;
    }
    CATCH_RETURN();

(where str is the VT sequence we're writing). IMO, if that fails, then we're probably going to end up in a much worse torn state than just this one sequence being lost.

The idea to do them all atomically would probably match well with #3016.