Avoid UIA event flooding in NvDA by moving UIA event handler COM objects into c++ #14888
Conversation
…ect when the event is for the focus object.
…en event is for the focus.
…we already have it.
…voids calling into Python, and instead records the events, and requests NvDA's main thread to later wake and read off the events when it can. This COM object also coalesces duplicate automation and propertyChange events for the same element, so that NVDA is no longer flooded with duplicate events, E.g. textChange events in Windows consoles.
|
This is fantastic to see! Yes, I believe the activity ID is the same for all notification events, but also CCing @carlos-zamora to be sure. Another pain point with event flooding is Visual Studio ( |
|
Also, just to clarify: the diffing/notifications setting only affects Windows Terminal ( |
See test results for failed build of commit d39b6e5e81 |
…ot crash VS Code Intellisense.
michaelDCurran
force-pushed
the
UIAEventLimiting
branch
from
e70fa64
to
d191e72
Compare
|
failed |
| HRESULT RateLimitedEventHandler::queueEvent(EventRecordArgTypes&&... args) { | ||
| LOG_DEBUG(L"RateLimitedUIAEventHandler::queueEvent called"); | ||
| bool needsFlush = false; | ||
| const unsigned int flushTimeMS = (EventRecordClass::isCoalesceable)?30:0; |
There was a problem hiding this comment.
The flush time is tricky. On one hand, we want rate limiting to be useful. On the other hand, this could impact perceived performance. In particular, property change events on the focus can be relevant to immediate user interaction; e.g. value changes on a focused volume slider. The response is likely to be slower than 30 ms because of timer resolution, additional time to queue the UIA event to NVDA core, etc.
I think the gains outweigh the losses here, but this is something we should at least consider for future optimisation. Perhaps we don't coalesce certain property changes on the focus or something like that?
There was a problem hiding this comment.
I think it would be worth then only doing coalescing for automation events, and not propertyChange events in this case. Not hard to change. Noting that the main motivation for this PR was textchange automation events. I think I should also add an extra variable to track if a flush message has been posted due to a non-coalesced event, and not post any more until the next flush. Currently if there are a whole bunch of non-coalesced events in a row, we'll keep flooding NVDA with pointless flush messages.
There was a problem hiding this comment.
Doing this only for automation events seems reasonable for this first PR if you prefer. However, I could see situations where property change event flooding could be a real issue. Task Manager is a good example. In that case, I think it does make sense to coalesce property change events except for the focus.
Yeah, the pointless flush messages thing is definitely worth fixing. I hadn't considered that.
There was a problem hiding this comment.
A couple of points:
- PropertyChange events are only coalesced if their runtimeIDs match, and they are the same property (property ID). I.e. two name property changes on the same element are coalesced. I'm not too sure that this pr will really help task manager as I'm thinking its many properties across many list items.
- Selective UIA registration is used in NVDA by default on Windows 11 now. thus there are hardly any property changes on elements other than the focus or its ancestors. I'd love to have this on by default in Windows 10 also, but apparently there are some bugs in Task list / emoji picker which stop us from doing that. @josephsl @codeofdusk ?
- I've now added a commit that stops propertyChange events from being coalesced. and also a commit which stops the pointless flush messages.
|
@jcsteh re your comments around documentation, I'll go through and document much of it shortly. |
|
A regression was found in this PR.
Try to open voice settings panel with NVDA+crtl+v
NVDA stops responding on my machine.
There is no such behaviour when running latest master branch
…On 5/2/23, Michael Curran ***@***.***> wrote:
@michaelDCurran commented on this pull request.
> @@ -1376,3 +1405,14 @@ def terminate():
def _isDebug():
return config.conf["debugLog"]["UIA"]
+
+
+WM_NVDA_UIA_FLUSH = winUser.registerWindowMessage("WM_NVDA_UIA_FLUSH")
+
+
+def eventLimiterWindowProc(msg, wParam, lParam):
+ if msg == WM_NVDA_UIA_FLUSH:
+ if lParam == 0:
+ NVDAHelper.localLib.rateLimitedUIAEventHandler_flush(wParam)
We could set up a window in a dedicated background thread. But if
isUIAWindow did hang, processing of all UIA events would also pause, as they
all went via that thread. So I don't think this is much better than it being
on the main thread. It is still worth separately considering making
isUIAWindow execute on a background thread, but perhaps something a little
bit more involved where we have a thread per UI thread, so that if the UI in
one thread freezes, it does not block another one. I think that might be
separate to this pr.
--
Reply to this email directly or view it on GitHub:
#14888 (comment)
You are receiving this because you are subscribed to this thread.
Message ID: ***@***.***>
--
with best regards Beqa Gozalishvili
Tell: +995593454005
Email: ***@***.***
Web: https://gozaltech.org
Skype: beqabeqa473
Telegram: https://t.me/gozaltech
facebook: https://facebook.com/gozaltech
twitter: https://twitter.com/beqabeqa473
Instagram: https://instagram.com/beqa.gozalishvili
|
|
@michaelDCurran Overal performance of NVDA dialogs are very bad, this is shown in voice, braille panels, also in the category list |
|
@beqabeqa473 Cannot repro this in a self-signed build |
|
I am running NVDA from source and can reproduce it every time i run it.
I am now trying to bysect it.
…On 5/2/23, Bill Dengler ***@***.***> wrote:
@beqabeqa473 Cannot repro this in a self-signed build
--
Reply to this email directly or view it on GitHub:
#14888 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
--
with best regards Beqa Gozalishvili
Tell: +995593454005
Email: ***@***.***
Web: https://gozaltech.org
Skype: beqabeqa473
Telegram: https://t.me/gozaltech
facebook: https://facebook.com/gozaltech
twitter: https://twitter.com/beqabeqa473
Instagram: https://instagram.com/beqa.gozalishvili
|
|
So, yes, #de5332f119ac0b6673df0deb08a0e90ff55fd6d7 breaks NVDA settings dialog on my machine |
|
I can reproduce this with UIA registration set to global.
I'll look into this.
Possibly something about uIA events coming from NVDA's main thread which
I have not considered clearly enough.
|
See test results for failed build of commit 64574df5d2 |
… better that these are quick for responce, and it is really autoamtion events like textchange which truly need coalescing.
…ges after a quick flush is requested and before the flush has actually occured.
…thread rather than NVDA's main thread. This works around some weird freezing when accessing NVDA's own UI. Note that UIA core always calls UIA event handlers on worker threads, except for when processing events within a UI Automation event registration call. Thus, our UIA Handler MTA thread sits there doing nothing 99% of the time. So moving the flush calls into this thread does not disrrupt true UI Automation event handler threads. If this design seems okay, then we may consider registering a custom window on the UIAHandler thread in order to receive posted messages directly, rather than them being queued to UIAHandler's queue from NVDA's main thread.
See test results for failed build of commit 39f0f91c27 |
* rather than the limiter sending a window message to NvDA to request a flush, instead the limiter class has its own dedicated thread which it wakes for flushing. * Now all UI Automation events are coalesced. However, there is no delay before flushing. It will happen as soon as the flush thread can wake and flush. These changes mean that UI Automation events again enter NVDA on a dedicated MTA thread, with no deliberate delay or having to go through NvDA's main thread first. Therefore the primary reason for the event limiter in c++ is to ensure that UI Automation core is never blocked on an event handler - they should queue and return very fast and not hit Python at all. Secondarily, any duplicate UI Automation events that exist before NVDA receives them are removed. This all still has the major advantage that doing `git -P log -1000` in a Windows console will not take out UIA focus events, nor slow down NvDA while the console isn't in focus. There could be further work in UIAHandler to stop adding duplicate textchange and caret UIA events to NVDA's event queue if the last one has not yet been processed.
See test results for failed build of commit 2dc39508a4 |
|
@michaelDCurran commit b94f4a1 gives me a linker error: |
|
Hi @michaelDCurran |
|
Interesting, I can no longer reproduce the UWP OneNote issues. Thanks @LeonarddeR and @michaelDCurran for adding the flag though in case I run into it again. I'm going to leave it enabled for now and feel confident with this being merged! |
|
I have built this now from source and make sure that this works again. |
…s.UIA.XamlEditableText when removin editable field support from emoji panel. Re nvaccess#15836. Regression introduced with nvaccess#14888: Windows 11 emoji panel items are not annonced when pressing arrow keys as XAML editable text overlay class is used to mark emoji search field. Therefore, remove the newly introduced XAML editable text class from overlay class chooser.
|
Now that this is merged, is #11214 for selectively register for UIA events still needed at all in NVDA? |
…s.UIA.XamlEditableText when removin editable field support from emoji panel. Re #15836. (#15837) Regression introduced with #14888: Windows 11 emoji panel items are not announced when pressing arrow keys as XAML editable text overlay class is used to mark emoji search field. Therefore, remove the newly introduced XAML editable text class from overlay class chooser. Closes #15836 Summary of the issue: Windows 11 emoji panel items are not annonced when using arrow keys to navigate the emoji panel. Description of user facing changes Emoji panel items are once again announced when using the arrow keys to navigate Windows 11 emoji panel. Description of development approach In modern keyboard/emoji panel app modue, replace regular editable text with NVDAObjects.UIA.XamlEditableText when detecting and removing edit field commands form emoji search field.
follow up of #14888 Summary of the issue: During development of #14888, it was observed that XAML edit fields tend to fire text change events before the caret position is changed, resulting in wrong text reporting. After using Visual Studio, I discovered that the same issue applies to the text controls in Visual Studio, notably WPF. Description of user facing changes In Visual Studio, the caret position is no longer sometimes reported inaccurately. Description of development approach use a common base class for XAML and WPF editable text.
…p voice messages (#15840) This pull request adds mention about the fact that the whatsapp voice messages no longer freeze nvda as per #15169, and #14888. This pull request also fixes some mistakes and typos Description of user facing changes Fixes for the documentation Description of development approach Edited the english changes.t2t
…erations library on NVDA exit. (#16222) Fixes #16072 Summary of the issue: As seen in a dump file provided by @irrah68 when debugging #16072 , NVDA can hang on exit while trying to destruct an instance of the CUIAutomation8 COMClass object: Call stack Details In short, it looks as if the destruction of the CUIAutomation8 COMClass object held by the UIA abstraction Remote Operations library locks up as it is happening from a different thread from where it was created. It was originally created in NvDA's UIA MTA thread, but it is being automatically cleaned up by the CRT of NVDA's UIARemote.dll in NVDA's main thread. I'm not entirely sure why this is only a problem on @irrah68's machine, and why it is more likely to happen with the UIA Rate Limited event handler in #14888. Either way though, NVDA is not cleaning it up correctly. Description of user facing changes NVDA is less likely to hang on NvDA exit. Description of development approach Add a cleanup function to UIARemote.dll which calls the UIA abstraction remote operations library's cleanup function. This function frees all remote contexts, but most importantly, releases the CUIAutomation8 COMClass object which was passed in on initialization. Add a terminate function to UIAHandler/remote.py which calls UIARemote.dll's cleanup function. at the end of UIA's MTA thread in Python, after removing event handlers, finally call UIAHandler.remtoe.terminate, ensuring that the UIA abstraction remote ops library is correctly cleaned up in the UIA MTA thread, where it was originally initialized.
Link to issue number:
Summary of the issue:
If NVDA is flooded with UI Automation events, such as textChange events from Windows consoles, NVDA may seem to freeze, and or UIA focus tracking completely stops.
Description of user facing changes
NVDA should remain responsive when being flooded with many UI Automation events, E.g. large amounts of text being written to a Windows console.
Description of development approach
Implement a new UI Automation event handler COM object in c++ which avoids calling into Python, and instead stores the events, and requests NvDA's main thread to later wake and read off the events when it can. This COM object also coalesces duplicate automation and propertyChange events for the same element, so that NVDA is no longer flooded with duplicate events, E.g. textChange events in Windows consoles.
The path for a UI Automation event is now as follows:
These changes mean that UIA core in the MTA is never blocked waiting for the Python GIL, and from its perspective, event handling is instantanious. Thus UIA core should never feel the need to kill off events, including focus change events.
This pr also provides a couple of extra optimizations for UIA handling in Python:
Testing strategy:
git -P log -1000. to majorly flood the console with text.Known issues with pull request:
UIA notification events are currently not coalesced in any way. Though I feel that Windows Terminal provides more 'responsible' rate of notification events, as compared to textchange events. If this is a problem, then perhaps we could coalesce on activityID, though I'm not sure if that is the same value for all console text events. Do you know / have an opinion @codeofdusk?
Change log entries:
New features
Changes
Bug fixes
For Developers
Code Review Checklist: