1802268 - Big tables can make DocAccessible::ProcessContentInserted() very slow.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Paul Bone [:pbone]]

This is spun off from Bug 1716686

Opening a document with a large table can cause jank that I see as being able to scroll out of the APZ viewport and seeing a blank document and having to wait some seconds for it to get painted.

I tested with https://hg.mozilla.org/mozilla-central/firefoxreleases#bbb448a557b4nightlylinux3220210531170022

Here is a profile with my Firefox Nightly.
https://share.firefox.dev/3EyGmED

Here is a profile with the same installation but I set set accessibility.force_disabled = 1, the profile doesn't show jank.
https://share.firefox.dev/3Er3ViE

I recognise that the profiler is mostly-inaccessible, so I will interpret the profile: the jank is in DocAccessible::ProcessContentInserted(LocalAccessible* aContainer, const nsTArray<nsCOMPtr<nsIContent>>* aNodes); With some nodes lasting as long as 7 seconds. I can see from the children of this node that the main do-while loop in this function is iterating many many times with most of it's time spent in AfterInsert and its callees.

Comment 1

[James Teh [:Jamie]]

If I'm reading the profile right, while this is definitely related to a11y, it probably isn't related to the CTW cache. Would you mind re-testing with accessibility.cache.enabled set to false (requires browser restart)? If I'm right, you'll still see the problem.

There seem to be a lot of insertions happening here, perhaps because the page is large enough that it can't render in one hit. Those insertions cause a lot of a11y mutation events and calculating those events is hurting us badly.

Comment 2

[James Teh [:Jamie]]

My more naive (albeit simpler) caching strategy for HyperText offsets in the content process in bug 1766794 might have regressed this.

Comment 3

[Paul Bone [:pbone]]

(In reply to James Teh [:Jamie] from comment #1)

If I'm reading the profile right, while this is definitely related to a11y, it probably isn't related to the CTW cache. Would you mind re-testing with accessibility.cache.enabled set to false (requires browser restart)? If I'm right, you'll still see the problem.

There seem to be a lot of insertions happening here, perhaps because the page is large enough that it can't render in one hit. Those insertions cause a lot of a11y mutation events and calculating those events is hurting us badly.

I have restarted with CTW disabled and the problem still reproduces, it looked different because it wouldn't scroll at all, but in the profile https://share.firefox.dev/3gqG36Y it's in the same place.

Comment 4

[James Teh [:Jamie]]

It probably looks different because the parent process blocks waiting for content with the cache disabled, whereas it doesn't do that with the cache enabled. But ultimately, the root cause isn't the cache.

Thanks for filing and testing. I have at least one hypothesis here (see comment 2), so I'll dig into that when I get a chance.

Comment 5

[James Teh [:Jamie]]

A build before bug 1766794 does seem to be better, though it's hard to be entirely certain because it's still a bit janky (but I can't seem to see any reason for that jankiness in the profiler). Bug 1766794 definitely seems to have regressed this to some extent at least, so even if it isn't the complete cause, I'm going to deal with that regression first.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1766794

Comment 7

[James Teh [:Jamie]]

Attachment: Bug 1802268 part 1: Add AccAttributes::GetMutableAttribute.

We will soon have an offsets array in the parent process cache which is not built in one shot.
Instead, we will gradually cache more offsets as they are requested.
To do this, we need a way to get an array from the cache which can be modified in-place.

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1802268 part 2: Reinstate partial invalidation of cached HyperText offsets for local HyperTextAccessible.

In bug 1766794, I replaced partial invalidation of cached offsets with full cache invalidation.
I did this to simplify the code, since the offsets cache was being extended to RemoteAccessible.
This is fine for RemoteAccessible because offsets are only needed there when clients query them.
However, for local HyperTextAccessible, offsets are always required to fire text events.
Full cache invalidation there meant that many insertions into a large container was very expensive.

To fix this, reinstate the ability to partially invalidate the offsets cache.
This is largely the code I previously removed from HyperTextAccessible, with some tweaks for readability.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80097f40fc50
part 1: Add AccAttributes::GetMutableAttribute. r=eeejay,nlapre
https://hg.mozilla.org/integration/autoland/rev/e488a2686d0b
part 2: Reinstate partial invalidation of cached HyperText offsets for local HyperTextAccessible. r=eeejay

Comment 10

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/80097f40fc50
https://hg.mozilla.org/mozilla-central/rev/e488a2686d0b

Comment 11

[Andrej (:aglavic)]

== Change summary for alert #36361 (as of Thu, 08 Dec 2022 19:00:02 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
31%	a11yr dhtml.html	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	946.25 -> 651.99
30%	a11yr dhtml.html	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	950.04 -> 666.51
29%	a11yr dhtml.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	941.14 -> 667.76
24%	a11yr dhtml.html	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	1,059.01 -> 802.65
23%	a11yr dhtml.html	linux1804-64-shippable-qr	e10s fission stylo webrender	1,058.59 -> 813.22
...	...	...	...	...
19%	a11yr dhtml.html	windows10-64-shippable-qr	e10s fission stylo webrender-sw	1,198.96 -> 966.44

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=36361