Closed Bug 1766794 Opened 2 years ago Closed 2 years ago

[CTW] Terrible performance in NVDA vbuf loading using View Source

Categories

(Core :: Disability Access APIs, defect)

Desktop
Windows
defect

Tracking

()

RESOLVED FIXED
102 Branch
Tracking Status
firefox102 --- fixed

People

(Reporter: Jamie, Assigned: Jamie)

References

Details

(Whiteboard: [ctw-m2])

Attachments

(1 file)

STR (with NVDA):

  1. Enable CTW and restart Firefox.
  2. Open https://www.jantrid.net/
  3. View source.
  4. Press NVDA+f5 to refresh NVDA's buffer.
    • Expected: It takes a fraction of a second.
    • Actual: It takes more than a second.
  5. Repeat with CTW disabled.
    • Observe: It takes a fraction of a second.

This is the first time I've seen CTW perform worse than non-CTW on Windows. However, it's likely that there are significant perf gains to be had across the board based on the learnings from this.

Profile: https://share.firefox.dev/3LnP6zF

This is much, much worse on bigger pages. That page only has a few hundred lines of code. Even on a Bugzilla bug with about 20 comments, there are several thousand lines of code, and the buffer takes minutes to render.

In the spirit of "start simple and optimise later", I didn't port the HyperText offset cache from HyperTextAccessible when I was implementing HyperTextAccessibleBase. It wasn't clear to me how much we needed this optimisation. Well, it turns out we do.

The thing that's unique about view source is that it has one single container with many children (6403 in this example). That's because of all the syntax highlighting, which creates many little text nodes. Because this is such a large HyperText container, calculating the offset for each node is pretty expensive, since we have to walk through the child nodes accumulating the text length. That's O(n^2) at best, and on a page like this, n is biiig.

The algorithm for this offset cache isn't particularly difficult. What's tricky here is that it's hard to use a common cache across the local and remote code because we cache differently for local and remote. Local doesn't have mCachedFields, which is a nice place to store this so we don't waste memory in RemoteAccessibles that don't need this.

While caching HyperText offsets will obviously improve this, this profile clearly shows that searching cache hash tables definitely incurs a cost. That's the nature of hash tables, of course, but I don't think I realised how high that cost could get.

Whiteboard: [ctw-m2]
Assignee: nobody → jteh

We already have a similar cache in local HyperTextAccessible.
Unfortunately, we can't use common code here because we don't want to waste memory having a member variable on all RemoteAccessibles, but local HyperTextAccessibles don't have mCachedFields.
This improves performance significantly when walking the text attributes of a container with a large number of text leaf children, such as is encountered when using view source.

Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e12dbde5824b
Lazily cache HyperText offsets for RemoteAccessibles.

Marking as S3 because this is still behind a pref

Severity: -- → S3
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a81a40b250b4
Lazily cache HyperText offsets for RemoteAccessibles. r=eeejay
Flags: needinfo?(jteh)
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95a2cae69fb9
Lazily cache HyperText offsets for RemoteAccessibles. r=eeejay
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch
Regressions: 1768391
QA Whiteboard: [qa-102b-p2]
See Also: → 1802268
Regressions: 1802268
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: