576777 - provide quick access to link index

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

IA2 get_hyperlinkIndex is heavy used by AT and web pages loading performance suffers because of this. It's reasonable to cache index of embedded character so we can find quickly embedded object by text offset.

I think blindcooltech.com should be loaded even more faster. Though perhaps we should find another more heavy benchmark to see a difference.

I moved embedded object collector to nsAccessible because ATK wants to deal with embedded objects always and that allows to prevent keeping a pointer to collector for hypertext accessibles twice (bug 420993), since web pages contains huge amount of hypertext accessibles then it makes sense. However the same time it's inherited by text leafs and that's not desired (though they inherit many undesired members). Later we should work to on accessible classes hierarchy to avoid this.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

I agree, want to file a bug to make hypertext accessibles more lightweight?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> I agree, want to file a bug to make hypertext accessibles more lightweight?

I assume you meant text leaf accessibles. Perhaps we have something but technically we should reconsider whole hierarchy to make it more lightweight and it should be a part of that.

Comment 3

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-8abfdd1017e8/

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 455878 [details] [diff] [review]
patch

r=me thanks! Note that I can't actually detect a difference betwen a current nightly and this build in speed of loading blindcooltech.com. It is simply very fast already that measuring it by user experience is not possible. :-)

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 455878 [details] [diff] [review]
patch

r=me if this is required for bug 420993 or we can get a good test case for perf improvement.

Comment 6

[alexander :surkov (:asurkov)]

This is perf bug important for processing of huge HTML pages by AT like NVDA. Needed approval for 2.0.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Comment on attachment 455878 [details] [diff] [review]
> patch
> 
> r=me if this is required for bug 420993

right, this is part of bug 420993.

> or we can get a good test case for perf
> improvement.

adding test is really easy. Just grow up any web page (for example, that blindcooltech) in several times and start getLinkAtOffset() for every offset in the hypertext accessible or like
var count = text.linkCount;
for(var idx = 0; idx < count; idx++) {
  var link = text.getLinkAt(idx);
  getLinkIndexAtOffset(link.startIndex);
}

It makes sense to add it into your perf test suite for tinderbox. Actually I think every fixed perf bug should be there. And it would be great if you would care about this :) since you're on top of this.

Comment 8

[[e-mail]]

Comment on attachment 455878 [details] [diff] [review]
patch

Sorry for the delay, backlogged after the Summit.

> nsAccessible::InvalidateChildren()
> {
>   PRInt32 childCount = mChildren.Length();
>   for (PRInt32 childIdx = 0; childIdx < childCount; childIdx++) {
>     nsAccessible* child = mChildren.ElementAt(childIdx);
>     child->UnbindFromParent();
>   }
> 
>+  mEmbeddedObjCollector = nsnull;
>   mChildren.Clear();
>-  mAreChildrenInitialized = PR_FALSE;
>+  mChildrenFlags = eChildrenUninitialized;
After the children are invalidated, their mIndexOfEmbeddedChild is no longer correct, is it? How do you know that it won't be used before it is recalculated?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> After the children are invalidated, their mIndexOfEmbeddedChild is no longer
> correct, is it? How do you know that it won't be used before it is
> recalculated?

Yes, mIndexOfEmbeddedChild is -1 and mEmbeddedObjCollector is null. When index in embedded children collection is requested then embedded children collection (mEmbeddedObjCollector) is recreated and index is calculated (EmbeddedObjCollector::GetIndexAt).

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Comment on attachment 455878 [details] [diff] [review]
> patch
> 
> Sorry for the delay, backlogged after the Summit.

yes, the same for me

Comment 11

[[e-mail]]

(In reply to comment #9)
> (In reply to comment #8)
> > After the children are invalidated, their mIndexOfEmbeddedChild is no longer
> > correct, is it? How do you know that it won't be used before it is
> > recalculated?
> Yes, mIndexOfEmbeddedChild is -1 and mEmbeddedObjCollector is null.
I see where mEmbeddedObjCollector gets set to null but not where mIndexOfEmbeddedChild gets set to -1.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> I see where mEmbeddedObjCollector gets set to null but not where
> mIndexOfEmbeddedChild gets set to -1.

UnbindFromParent()

Comment 13

[[e-mail]]

(In reply to comment #12)
> (In reply to comment #11)
> > I see where mEmbeddedObjCollector gets set to null but not where
> > mIndexOfEmbeddedChild gets set to -1.
> UnbindFromParent()
So when the parent is invalidated then all the children get unbound?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > I see where mEmbeddedObjCollector gets set to null but not where
> > > mIndexOfEmbeddedChild gets set to -1.
> > UnbindFromParent()
> So when the parent is invalidated then all the children get unbound?

Yes, that's how things are processed currently, at least for all text accessibles what mEmbeddedObjCollector is used for.

Comment 15

[[e-mail]]

Comment on attachment 455878 [details] [diff] [review]
patch

Fair enough. I didn't spot anything else interesting.

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 455878 [details] [diff] [review]
patch

this is major perf improvement of big web pages pages loading by screen readers.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

Marking blocking beta N since we'll want some beta time with this before ship.

Comment 18

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/e29fc477ab4d

Comment 19

[alexander :surkov (:asurkov)]

revert mochitest fix since it fails now (I think Ehsan landed editor fix and text elements aren't exposed for placeholder text) - http://hg.mozilla.org/mozilla-central/rev/d84aff2ae1db

Comment 20

[Markus Stange [:mstange]]

I've backed everything out because the test still failed.

For example in this log, which I doesn't load for me at the moment:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281007642.1281008346.23736.gz

http://hg.mozilla.org/mozilla-central/rev/decafc4eb6e2
http://hg.mozilla.org/mozilla-central/rev/14d701572f80
http://hg.mozilla.org/mozilla-central/rev/8565e77eacdc
http://hg.mozilla.org/mozilla-central/rev/2638ec5b4151

Comment 21

[(no longer active)]

(In reply to comment #19)
> revert mochitest fix since it fails now (I think Ehsan landed editor fix and
> text elements aren't exposed for placeholder text) -
> http://hg.mozilla.org/mozilla-central/rev/d84aff2ae1db

I don't really know what the failure was, and less so how the solution works.  I can't even confirm that some of my pushes have caused this test to fail.  I usually push everything to the try server, and I'm especially wary of a11y test failures.

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

Alexander, I think this is next to land. We'll need to update the patch and get a clean tryserver run. Note if you need to disable the area 14 tests altogether, r=me.

Comment 23

[alexander :surkov (:asurkov)]

landed http://hg.mozilla.org/mozilla-central/rev/dec698262ea8 with disabled failing test since it sounds color value depends on platform settings and may vary.