Don't notify a11y event when in ConPTY mode

[skyline75489]

Don't notify a11y event when in ConPTY mode

In support of #10528

[skyline75489]

If I'm not mistaken, this only helps with ENABLE_VIRTUAL_TERMINAL_PROCESSING disabled? Because in WriteChars we use:

[[nodiscard]] NTSTATUS WriteChars(...) {
    if (!WI_IsFlagSet(screenInfo.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING) ||
        !WI_IsFlagSet(screenInfo.OutputMode, ENABLE_PROCESSED_OUTPUT))
    {
        return WriteCharsLegacy(screenInfo,
                                pwchBufferBackupLimit,
                                pwchBuffer,
                                pwchRealUnicode,
                                pcb,
                                pcSpaces,
                                sOriginalXPosition,
                                dwFlags,
                                psScrollY);
    }
    // ...
}

This saves ~7% CPU time:

Before:

After:

[DHowett]

It is also called through the default string handling case for the VT state machine engine, to handle all non-VT strings.

[codeofdusk]

More context on this please? Does it affect UIA? CC @carlos-zamora.

[DHowett]

@codeofdusk two important things here:

1. This is the legacy accessibility eventing system, MSAA
2. "ConPTY" is the term for when conhost is serving as the "translator" behind another terminal emulator. These events were going nowhere, as the conhost in question is not displaying a window and it is fully expected that the hosting terminal is providing accessibility.

It just wastes CPU time doing accessibility (1) poorly and (2) on a disconnected channel nobody is receiving any information from, ever.

[DHowett]

I'm not sure about this approach. There's a lot of accessibility event signalling hooked up to this provider, and doing this thing where we just suppress one of the events is just playing "whack-a-mole."

@lhecker -- if we replaced the IAccessibilityThinger with a version that just no-ops for ConPTY... is the cost of a vtable dispatch more expensive than a branch on every output to check if we're in VtIo mode?

Replacing it with a no-op IAccessibilityThing will make it easier to get every single place we would have called it -- cursor updates, scrolling, etc etc etc without having to dig through every bit of code.

[lhecker]

@lhecker -- if we replaced the IAccessibilityThinger with a version that just no-ops for ConPTY... is the cost of a vtable dispatch more expensive than a branch on every output to check if we're in VtIo mode?

Branches are probably a bit cheaper, but I doubt by much; we'd have to measure the impact. Since the vtable approach would be more ergonomic I think we should try that first...

[DHowett]

Actually, I suspect that SCREEN_INFORMATION::NotifyAccessibilityEventing on its own is already too expensive, even if we do a new no-op virtual implementation of IAccessibilityNotifier.

It reads the buffer, does some UCS2 crap, and then calls the vtable.

I think we need to do both.

You should move the check here to be inside NotifyAccessibilityEventing and add a new empty/no-op IAccessibilityNotifier that we can use in InteractivityBase.

Sounds like we're changing the approach, so preemptively dismissing my review

[skyline75489]

@DHowett I finished the first part and moved the check inside NotifyAccessibilityEventing. I found that I'm not the right person to do the second part: add a new empty/no-op IAccessibilityNotifier and move everything to the no-op one. There's a lot legacy conhost & internal Windows stuff that I don't understand enough to modify. I figure I will just limit this PR to the first part. It's still brings noticeable performance benefit (updated my comment above). That is, in fact, my initial intention for this PR.

[DHowett]

I'm okay with this if @miniksa is.

[DHowett]

@msftbot make sure @miniksa signs off

[ghost]

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

• I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

[miniksa]

I'm good but I filed a follow on and will send the second half soon. I can see the route to easy completion on it given my history here.

[ghost]

🎉Windows Terminal v1.9.1942.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download

[ghost]

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download