396005 - Severe pageload performance degradation with accessibility enabled

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Test file

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: zipped-up profile

Comment 3

[Boris Zbarsky [:bzbarsky]]

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 

Comment 4

[Ginn Chen]

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

Comment 5

[Ginn Chen]

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.

Comment 6

[Aaron Leventhal]

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.

Comment 7

[Aaron Leventhal]

Okay, I'll be on this bug either tomorrow or next week. Need to prepare for a talk.

Comment 8

[Aaron Leventhal]

Attachment: 1) When new nodes created during page load, do bare minimum (invalidate parent's children) if they are not created from user input, 2) Don't create extra nodes from debug code during page load

Comment 10

[Marco Zehe (:MarcoZ)]

(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 **'

Comment 11

[Aaron Leventhal]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Aaron Leventhal]

Attachment: Use CallQueryInterface()

Comment 14

[Aaron Leventhal]

Attachment: Merged to trunk

Comment 15

[alexander :surkov (:asurkov)]

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.

Comment 16

[Aaron Leventhal]

> 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?

Comment 17

[alexander :surkov (:asurkov)]

(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.

Comment 18

[Aaron Leventhal]

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.

Comment 19

[alexander :surkov (:asurkov)]

(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 20

[alexander :surkov (:asurkov)]

Comment on attachment 282759 [details] [diff] [review]
Merged to trunk

r=me

Comment 21

[Aaron Leventhal]

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.

Comment 22

[Aaron Leventhal]

Ignore comment 23. That was for another bug.

This one is obviously a critical fix.

Comment 23

[Aaron Leventhal]

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