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

wpf: delay-load UIAutomationCore because it's incomplete in RS1 #15614

Merged
merged 4 commits into from
Jun 27, 2023

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Jun 27, 2023

UiaRaiseNotificationEvent is not present on Windows Server 2016, even though it is documented as being present.
This also removes the cost of loading up UIAutomationCore from the critical path.

UiaRaiseNotificationEvent is not present on Windows Server 2016, even
though it is documented as being present.
This also removes the cost of loading up UIAutomationCore from the
critical path.
@DHowett
Copy link
Member Author

DHowett commented Jun 27, 2023

Note it no longer shows up in the import list!

$ depends -windir:v:\wim\windows E:\src\Terminal\bin\x64\Debug\PublicTerminalCore.dll

=== e:\src\terminal\bin\x64\debug\publicterminalcore.dll ===

  Dependencies:
    advapi32.dll
    api-ms-win-core-synch-l1-2-0
    bcrypt.dll
    d2d1.dll
    d3d11.dll
    d3dcompiler_47.dll
    dwrite.dll
    dxgi.dll
    kernel32.dll
    ole32.dll
    oleaut32.dll
    ucrtbased.dll
    user32.dll

27 modules loaded, 8563 functions resolved

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jun 27, 2023
@github-actions

This comment has been minimized.

@DHowett DHowett enabled auto-merge (squash) June 27, 2023 21:06
@@ -123,6 +133,12 @@ void HwndTerminalAutomationPeer::SignalCursorChanged()

void HwndTerminalAutomationPeer::NotifyNewOutput(std::wstring_view newOutput)
{
if (_notificationsUnavailable) [[unlikely]]
Copy link
Contributor

Choose a reason for hiding this comment

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

We're checking _notificationsUnavailable here but the first time we actually (might) set it is in _tryNotify which is called later in this function. That means the first time we enter this function we are always doing the same thing as before, and only on future calls do we potentially skip attempting to notify. Is it possible for us to set the proper value for _notificationsUnavailable for the first call as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, we are "late-binding" the failure state. We want to act as though everything works on all platforms, and only when it fails (on Windows 8.1 or RS1) update the state to avoid wasting any future effort on new notification generation.

One of the core principles of delay loading is that you get failures at the time of use (in the default config). Checking beforehand is a bit of a waste of time in that case... and the cost of one miss when accessibility is enabled on a very rarely-used platform is not high enough to make me want to do any preflight checking :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Effectively, this is "try to notify, stop ever trying again if it fails", which means we don't complicate any other codepaths.

There are other places where we could "pre"check, and none of them sound great to me. Here's where--and why.

  • We could check when a HwndTerminalAutomationPeer is constructed
    • This might happen on startup, which moves the loading of UIAutomationCore back onto the startup path
  • We could check at the top of this function
    • Doing this only once requires a magic static, which requires a thread local storage slot and a lock to ensure exclusivity

/DELAYLOAD operating as normal is cheaper than both of those 😄

We could also just not store the failure state and let the delayload failure trip every time. It's not THAT expensive... but it could fire once per keyboard input (!) or any text output (!) in the worst case.

@DHowett DHowett merged commit 99c18ce into main Jun 27, 2023
@DHowett DHowett deleted the dev/duhowett/uiauto-nope-why branch June 27, 2023 21:42
DHowett added a commit that referenced this pull request Jul 17, 2023
UiaRaiseNotificationEvent is not present on Windows Server 2016, even
though it is documented as being present.
This also removes the cost of loading up UIAutomationCore from the
critical path.

(cherry picked from commit 99c18ce)
Service-Card-Id: 89704753
Service-Version: 1.17
DHowett added a commit that referenced this pull request Jul 27, 2023
UiaRaiseNotificationEvent is not present on Windows Server 2016, even
though it is documented as being present.
This also removes the cost of loading up UIAutomationCore from the
critical path.

(cherry picked from commit 99c18ce)
Service-Card-Id: 89704754
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Tag-Fix Doesn't match tag requirements
Projects
Development

Successfully merging this pull request may close these issues.

3 participants