Skip to content

Improve responsiveness of input and focus by pumping immediately instead of after a delay.#14701

Merged
seanbudd merged 3 commits into
nvaccess:masterfrom
jcsteh:immediatePump
Mar 23, 2023
Merged

Improve responsiveness of input and focus by pumping immediately instead of after a delay.#14701
seanbudd merged 3 commits into
nvaccess:masterfrom
jcsteh:immediatePump

Conversation

@jcsteh

@jcsteh jcsteh commented Mar 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

None.

Summary of the issue:

Currently, everything in NVDA, including responding to keyboard input and focus changes, goes through the core pump. The core pump is always run with a slight delay. Assuming nothing else is already queued, any input or focus change will not be processed for a minimum of 10 ms. In reality, it's potentially longer due to the system timer resolution and the fact that timers are low priority messages. We should be aiming for the fastest possible response to user input and focus changes.

Description of user facing changes

NVDA will respond slightly faster to commands and focus changes.

Description of development approach

  1. The core pump has been refactored. Rather than using NonReEntrantTimer, it now uses wx.Timer directly and guards re-entry itself for better control. There is a main thread method (request) which manages scheduling/executing the pump to make state management easier.
  2. requestPump has a new immediate argument which specifies whether the pump should happen as soon as possible or after the delay. Delayed pumps are still useful in most cases for rate limiting and filtering, so this defaults to False.
  3. queueHandler.queueFunction has an _immediate argument which is passed to requestPump. It is _ prefixed to mitigate conflicts with keyword arguments intended for queued functions.
  4. inputCore, scriptHandler, focus changes and live text output all request an immediate core pump.

Testing strategy:

I used "time since input" logging to measure the differences in times before and after the change in various situations:

  • Report time (NVDA+f12) and input help took ~10 ms before the patch, ~0 ms after.
  • Navigating in the NVDA menu took > 20 ms before the change, ~10 ms after.
  • Moving the caret in Firefox took ~35 ms before the change, ~20 ms after.

This can be quite variable, so it's only a rough guide, but the results are pretty suggestive.

Known issues with pull request:

None.

Change log entries:

Bug fixes
NVDA now responds slightly faster to commands and focus changes.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • Security precautions taken.

@jcsteh jcsteh changed the title Improve responsiveness of input and focus by pumping immediately inst… Improve responsiveness of input and focus by pumping immediately instead of after a delay. Mar 7, 2023
@jcsteh

jcsteh commented Mar 7, 2023

Copy link
Copy Markdown
Contributor Author

I realise 10 ms is a pretty small difference, but even that potentially matters when it comes to feeling snappy, especially since there are other sources of delay we can't control (cross-process communication, application delays, minimum audio latency, speech synth processing, etc.). That said, given that we are talking about such a small delta, it's hard to be sure this makes a practical difference other than looking at the numbers.

@cary-rowen

cary-rowen commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

Hi,

Even if it's only a 10ms boost, I think it's pretty cool.
In the Chinese community, NVDA is criticized for its responsiveness.

@burmancomp

burmancomp commented Mar 8, 2023

Copy link
Copy Markdown
Contributor

There is cumulative effect when thinking all users daily use.

@AppVeyorBot

AppVeyorBot commented Mar 8, 2023

Copy link
Copy Markdown

See test results for failed build of commit ee20a6c022

@jcsteh jcsteh mentioned this pull request Mar 8, 2023
Merged
6 tasks
@jcsteh jcsteh marked this pull request as ready for review March 9, 2023 06:47
@jcsteh jcsteh requested a review from a team as a code owner March 9, 2023 06:47
@jcsteh jcsteh requested a review from seanbudd March 9, 2023 06:47
michaelDCurran
michaelDCurran approved these changes Mar 10, 2023
View reviewed changes

@michaelDCurran michaelDCurran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me.
Several system tests that generally don't fail, did fail on the first run, but then succeeded on the second. We'll want to keep an eye on this.

@seanbudd seanbudd merged commit cc8b5ad into nvaccess:master Mar 23, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Mar 23, 2023
@Adriani90

Adriani90 commented Mar 23, 2023

Copy link
Copy Markdown
Collaborator

I can confirm that this improves performance in MS Word significantly, I tested with a 164 pages document with many large tables and images and including converted text from through OCR which sometimes usually slowed down NVDA. Now it works really fluently even with UIA disabled. Still there is a noticeable difference when reporting pages in document formating settings is enabled or disabled.

This was referenced Apr 3, 2023
Closed
Closed
@Adriani90 Adriani90 mentioned this pull request Apr 22, 2023
Closed
@michaelDCurran michaelDCurran mentioned this pull request May 14, 2023
Merged
michaelDCurran added a commit that referenced this pull request May 14, 2023
@michaelDCurran
Revert "Improve responsiveness of input and focus by pumping immediat…
afb7392 
…ely instead of after a delay. (#14701)"

This reverts commit cc8b5ad.
michaelDCurran added a commit that referenced this pull request May 15, 2023
@michaelDCurran
Revert "Improve responsiveness of input and focus by pumping immediat…
15b5c69 
…ely instead of after a delay. (#14701)" (#14925)

This reverts pr #14701, commit cc8b5ad

Link to issue number:
Fixes #14753
May fix #14803

Summary of the issue:
After the merging of pr #14701, It was reported in #14753 that with input help mode on and moving your finger around the touch screen, many errors would be logged and eventually input help mode would be disabled.

The recursion itself was caused by the core pump's Notify method calling request directly, which itself calls Notify.
The obvious fix would be to wx.CallAfter the call to request. However, this then caused the WX event loop to fill up with responses to these call afters, and due to the way the wx event loop works (msgWaitForMultipleObjects waiting on a pending event win32 event), the event loop never ended up getting a chance to process OS and 3rd party messages. And because we monkeypatch CallAfter to PostMessage (in case we are in a win32 modal or menu and don't wake up) the win32 message queue was being filled completely thus not allowing future messages to be posted.
@jcsteh and I have considered several different ways of trying to work around this. Most of which have not been successful. There are probably more things we could try, but for now it is better to revert until we can get this right.

Description of development approach
Fully reverts pr #14701 for now.
josephsl pushed a commit to josephsl/nvda that referenced this pull request May 16, 2023
@michaelDCurran @josephsl
Revert "Improve responsiveness of input and focus by pumping immediat…
97dd976 
…ely instead of after a delay. (nvaccess#14701)" (nvaccess#14925)

This reverts pr nvaccess#14701, commit cc8b5ad

Link to issue number:
Fixes nvaccess#14753
May fix nvaccess#14803

Summary of the issue:
After the merging of pr nvaccess#14701, It was reported in nvaccess#14753 that with input help mode on and moving your finger around the touch screen, many errors would be logged and eventually input help mode would be disabled.

The recursion itself was caused by the core pump's Notify method calling request directly, which itself calls Notify.
The obvious fix would be to wx.CallAfter the call to request. However, this then caused the WX event loop to fill up with responses to these call afters, and due to the way the wx event loop works (msgWaitForMultipleObjects waiting on a pending event win32 event), the event loop never ended up getting a chance to process OS and 3rd party messages. And because we monkeypatch CallAfter to PostMessage (in case we are in a win32 modal or menu and don't wake up) the win32 message queue was being filled completely thus not allowing future messages to be posted.
@jcsteh and I have considered several different ways of trying to work around this. Most of which have not been successful. There are probably more things we could try, but for now it is better to revert until we can get this right.

Description of development approach
Fully reverts pr nvaccess#14701 for now.
@jcsteh jcsteh deleted the immediatePump branch May 25, 2026 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran michaelDCurran approved these changes

@seanbudd seanbudd Awaiting requested review from seanbudd seanbudd is a code owner automatically assigned from nvaccess/developers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2023.2

Development

Successfully merging this pull request may close these issues.

8 participants

@jcsteh @cary-rowen @burmancomp @AppVeyorBot @Adriani90 @michaelDCurran @seanbudd @nvaccessAuto