Closed
Bug 396005
Opened 17 years ago
Closed 17 years ago
Severe pageload performance degradation with accessibility enabled
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: aaronlev)
References
Details
(Keywords: hang, perf)
Attachments
(3 files, 2 obsolete files)
180.53 KB,
text/html
|
Details | |
221.49 KB,
application/zip
|
Details | |
9.66 KB,
patch
|
surkov
:
review+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•17 years ago
|
||
Reporter | ||
Comment 2•17 years ago
|
||
Reporter | ||
Comment 3•17 years ago
|
||
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.
Assignee | ||
Comment 6•17 years ago
|
||
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.
Assignee | ||
Comment 7•17 years ago
|
||
Okay, I'll be on this bug either tomorrow or next week. Need to prepare for a talk.
Assignee | ||
Updated•17 years ago
|
Blocks: fox3access
Assignee | ||
Comment 8•17 years ago
|
||
Attachment #282583 -
Flags: review?(surkov.alexander)
Comment 10•17 years ago
|
||
(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 **'
Assignee | ||
Comment 11•17 years ago
|
||
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.
Reporter | ||
Comment 12•17 years ago
|
||
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.
Assignee | ||
Comment 13•17 years ago
|
||
Attachment #282627 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•17 years ago
|
Attachment #282583 -
Attachment is obsolete: true
Attachment #282583 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 14•17 years ago
|
||
Attachment #282627 -
Attachment is obsolete: true
Attachment #282759 -
Flags: review?(surkov.alexander)
Attachment #282627 -
Flags: review?(surkov.alexander)
Comment 15•17 years ago
|
||
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.
Assignee | ||
Comment 16•17 years ago
|
||
> 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•17 years ago
|
||
(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.
Assignee | ||
Comment 18•17 years ago
|
||
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•17 years ago
|
||
(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•17 years ago
|
||
Comment on attachment 282759 [details] [diff] [review]
Merged to trunk
r=me
Attachment #282759 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 21•17 years ago
|
||
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?
Assignee | ||
Comment 22•17 years ago
|
||
Ignore comment 23. That was for another bug.
This one is obviously a critical fix.
Updated•17 years ago
|
Attachment #282759 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 23•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•