Closed Bug 396005 Opened 13 years ago Closed 13 years ago

Severe pageload performance degradation with accessibility enabled

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: aaronlev)

References

Details

(Keywords: hang, perf)

Attachments

(3 files, 2 obsolete files)

STEPS TO REPRODUCE:

1)  Load attached file with accessibility disabled
2)  Write down the number shown in the alert (number of ms to load the page)
3)  Load attached file with accessibility enabled
4)  Compare the number in the alert to the number in step 2

EXPECTED RESULTS:  Numbers close to the same

ACTUAL RESULTS:  Number in step 2 is between 1500 and 2000 for me.  Number in step 4 is 237000.  That would be a _factor_ of 120 slowdown or so.
Flags: blocking1.9?
Attached file Test file
Attached file zipped-up profile
Summary of profile data:

Time is largely spent under two functions: nsDocAccessible::CreateTextChangeEventForNode and nsAccUtils::IsAncestorOf.

nsAccUtils::IsAncestorOf is called from nsDocAccessible::FireDelayedAccessibleEvent, which is called for every single piece of content added to the tree.  The setup in nsDocAccessible::FireDelayedAccessibleEvent is such that each call is O(N) in number of already-pending events, which means that the whole thing is O(N^2) in number of events that get posted between event firings.  I do have to wonder why this code, which is triggered off an API that uses nsIContent is using nsIDOMNode for the ancestry checks.  Just the QI costs are killing you if nothing else.  Doing something more like what nsContentUtils does would be a _lot_ faster.  Still O(N^2), however.

nsDocAccessible::CreateTextChangeEventForNode is called from nsDocAccessible::FlushPendingEvents, off a timer.  The time under here is spent under nsAccTextChangeEvent::nsAccTextChangeEvent (getting the text of every single textnode on the page, in this case, I expect, though there seems to be a lot more QI stuff under here too), and nsHyperTextAccessible::DOMPointToHypertextOffset which has more QI cost, textrun construction, getting frames for text nodes (which is comparatively expensive), etc.

Some numbers just to put the COM and DOM usage here into perspective, in the slice of profile attached, we have:

24% under nsCOMPtr_base::assign_from_qi (so |nsCOMPtr<nsIFoo> = do_QI(bar)|)
16% under CallQueryInterface(nsINode*, nsIDOMNode**) (basically under GetParent)

Of particular interest under that 24% is the fact that nsHyperTextAccessible::QueryInterface does some _really_ expensive things (e.g. calling Role(), which it should probably cache, QIing its mDOMNode, which it should probably cache, etc).

All that said, it seems like the basic problem is that there are just way too many events in flight, both at once and in general during the page load, with each event being expensive to process.  Should we be coalescing them better, in addition to speeding up the processing?
Of 
It's caused by this checkin.

2007-08-30 11:48	aaronleventhal%moonset.net	 mozilla/accessible/src/base/nsDocAccessible.cpp
Bug 392153. Form to file a bug is missing from MSAA tree. r=ginn.chen, a=dsicore
I took 20070830 nightly build and did the test w/ and w/o accessibility feature.
I reload the page 7 times, the results are
w/ a11y on, median 490, min 471, max 549
w/o a11y on, median 486, min 472, max 585
It's about the same.

Well, I will not be surprised if there're some other testcases can prove a11y feature slows down Firefox.
I don't know yet.
And, Bug 394115 is filed earlier than the regression, so there might be other hangs.
I believe this is basically the same as bug 395619.

This is not really because of bug 392153 -- that fix was correct but it revealed our inner weakness with these events.
Okay, I'll be on this bug either tomorrow or next week. Need to prepare for a talk.
Blocks: fox3access
Duplicate of this bug: 395619
(In reply to comment #8)
>+    else { // Only return cached accessibles, don't create anything
>+      nsCOMPtr<nsIAccessNode> accessNode;
>+      GetCachedAccessNode(currentNode, getter_AddRefs(accessNode)); // AddRefs
>+      if (accessNode) {
>+        accessNode->QueryInterface(NS_GET_IID(nsIAccessible), static_cast<void**>(aAccessible));

That last line throws a compiler error C2440: 'static_cast': 'nsIAccessible **' cannot be converted to 'void **'
Yes, I originally had that. Surkov, please review the patch with that change in mind. I forgot to rebuild for that tweak before uploading the patch, and it obviously doesn't compile.
Uh.....  WHY are we calling QI directly?  That should be a CallQI call.  Generally, if you're calling NS_GET_IID you're doing something wrong.
Attached patch Use CallQueryInterface() (obsolete) — Splinter Review
Attachment #282627 - Flags: review?(surkov.alexander)
Attachment #282583 - Attachment is obsolete: true
Attachment #282583 - Flags: review?(surkov.alexander)
Attached patch Merged to trunkSplinter Review
Attachment #282627 - Attachment is obsolete: true
Attachment #282759 - Flags: review?(surkov.alexander)
Attachment #282627 - Flags: review?(surkov.alexander)
Comment on attachment 282759 [details] [diff] [review]
Merged to trunk


> PRBool nsAccessible::IsTextInterfaceSupportCorrect(nsIAccessible *aAccessible)
> {
>   PRBool foundText = PR_FALSE;
> 
>+  nsCOMPtr<nsIAccessibleDocument> accDoc = do_QueryInterface(aAccessible);
>+  if (accDoc) {
>+    // Don't test for accessible docs, it makes us create accessibles too
>+    // early and fire mutation events before we need to
>+    return PR_TRUE;

xul documents don't implement nsIAccessibleText. Is it better to query interface?

the rest looks ok.
> xul documents don't implement nsIAccessibleText. Is it better to query
> interface?
I don't understand. We should leave the method early for any kind of doc. What would you change exactly?
(In reply to comment #16)
> > xul documents don't implement nsIAccessibleText. Is it better to query
> > interface?
> I don't understand. We should leave the method early for any kind of doc. What
> would you change exactly?
> 

In the patch you suggested to return true a priori for accessible documents but now we can return false I guess. Since XUL documents are not nsIAccessibleText then I wonder how much it is correct for them.
But it's just a debugging assertion. I don't want to assert for any kind of documents. The point is not to do any testing for those at all and just return true so we don't assert.
(In reply to comment #18)
> But it's just a debugging assertion. I don't want to assert for any kind of
> documents. The point is not to do any testing for those at all and just return
> true so we don't assert.
> 

ok, it would be nice to have some comment why we don't want to assert on documents then :)
Comment on attachment 282759 [details] [diff] [review]
Merged to trunk

r=me
Attachment #282759 - Flags: review?(surkov.alexander) → review+
Comment on attachment 282759 [details] [diff] [review]
Merged to trunk

Low risk, gets us close to Opera parity in terms of how ARIA markup is processed. Also fixes incorrect ARIA usage in a couple of places in the UI.
Attachment #282759 - Flags: approval1.9?
Ignore comment 23. That was for another bug.

This one is obviously a critical fix.

Attachment #282759 - Flags: approval1.9? → approval1.9+
Surkov, I hope that is covered here -- I'm not sure what else to add:

    // Don't test for accessible docs, it makes us create accessibles too
    // early and fire mutation events before we need to
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.