560013 - remove nsAccessible::NextChild

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

It's very unperformant way to iterate through children since we traverse (n-1) children for every n-th child because of GetIndexOf() call.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

BlindCoolTech site perf (see bug 548515)

before 
[Root] 60.47
   |- xul.dll!CAccessibleHypertext::get_hyperlinkIndex 28.20
   |- xul.dll!CAccessibleHypertext::get_hyperlink 28.16
   |- ntdll.dll!_RtlUserThreadStart 1.74

after
[Root] 41.36
   |- xul.dll!CAccessibleHypertext::get_hyperlinkIndex 14.98
   |- xul.dll!CAccessibleHypertext::get_hyperlink 14.87
   | -ntdll.dll!_RtlUserThreadStart 5.95

Comment 3

[alexander :surkov (:asurkov)]

It makes BlindCoolTech working faster in 5 times by eye.

Comment 4

[alexander :surkov (:asurkov)]

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

regressions were found

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

1) remove NextChild
2) fix mac's GetUnignoredChildCount since the current version returns wrong child number

Comment 6

[alexander :surkov (:asurkov)]

try server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-2099fd27f652

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch3

Use mChildren in hypertext accessible to avoid unnecessary EnsureChildren check. Hypertext accessible always uses mChildren to store its children and EnsureChildren is called when GetChildCount is called.

Comment 8

[alexander :surkov (:asurkov)]

try server build for the latest patch - http://build.mozilla.org/tryserver-builds/[e-mail]-try-df368790358e

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 439836 [details] [diff] [review]
patch3

>-  PRUint32 childCount = mChildren.Length();
>+  PRInt32 childCount = mChildren.Length();

Why this change? Do we really expect negative child indexes sometimes? This question also pertains to the other childCount usages.

Other than that question, this definitely fixes the bug. I have a feeling this is even faster than in 3.6.x, but that's a purely subjective feeling. :) r=me

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> (From update of attachment 439836 [details] [diff] [review])
> >-  PRUint32 childCount = mChildren.Length();
> >+  PRInt32 childCount = mChildren.Length();
> 
> Why this change? Do we really expect negative child indexes sometimes? This
> question also pertains to the other childCount usages.

To get rid compilation warning. For other cases: GetChildCount() returns -1 if the accessible is defunct.

> Other than that question, this definitely fixes the bug. I have a feeling this
> is even faster than in 3.6.x, but that's a purely subjective feeling. :) r=me

It's very possible, at least it should be :)

Comment 11

[Marco Zehe (:MarcoZ)]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 439836 [details] [diff] [review] [details])
> > >-  PRUint32 childCount = mChildren.Length();
> > >+  PRInt32 childCount = mChildren.Length();
> > 
> > Why this change? Do we really expect negative child indexes sometimes? This
> > question also pertains to the other childCount usages.
> 
> To get rid compilation warning. For other cases: GetChildCount() returns -1 if
> the accessible is defunct.

OK, thanks!

Comment 12

[[e-mail]]

Comment on attachment 439836 [details] [diff] [review]
patch3

>         nsTArray<nsRefPtr<nsAccessibleWrap> > children;
>-        childWrap->GetUnignoredChildren(children);
>+        childAcc->GetUnignoredChildren(children);
>         if (!children.IsEmpty()) {
>           // add the found unignored descendants to the array.
>           aChildrenArray.AppendElements(children);
On an unrelated topic, if the method was called AddUnignoredChildren then you could replace this with childAcc->AddUnignoredChidlren(aChildrenArray);

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)

> On an unrelated topic, if the method was called AddUnignoredChildren then you
> could replace this with childAcc->AddUnignoredChidlren(aChildrenArray);

right, I filed bug 560189 for this. thank you.

Comment 14

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

Comment on attachment 439836 [details] [diff] [review]
patch3

r=me, and happy=me!

>   // Loop through children and collect valid offsets, text and bounds
>-  // depending on what we need for out parameters
>-  while (NextChild(accessible)) {
>-    lastAccessible = accessible;
>-    nsRefPtr<nsAccessNode> accessNode = nsAccUtils::QueryAccessNode(accessible);
>+  // depending on what we need for out parameters.
>+  PRInt32 childCount = GetChildCount();
>+  for (PRInt32 childIdx = 0; childIdx < childCount; childIdx++) {
>+    nsAccessible *childAcc = mChildren[childIdx];
>+    lastAccessible = childAcc;

Optional nit: Personally I would find this more readable if childIdx was just named i or index.


>+  if (childIdx < childCount) {
>     *aHyperTextOffset += addTextOffset;
>-    NS_ASSERTION(accessible == childAccessible, "These should be equal whenever we exit loop and accessible != nsnull");
>+    NS_ASSERTION(childAcc == childAccAtOffset,
>+                 "These should be equal whenever we exit loop and childAcc != nsnull");

Do we really need this assertion?

>+
>     if (aFinalAccessible &&
>-        (NextChild(accessible) ||
>-         static_cast<PRInt32>(addTextOffset) < nsAccUtils::TextLength(childAccessible))) {  
>+        (childIdx < childCount - 1 ||

Optional nit: I prefer (childIdx < (childCount -1) || ...

Comment 15

[Marco Zehe (:MarcoZ)]

After chatting with David on IRC about the nits, we decided to go ahead and push this on Alexander's behalf to get it in ASAP:
http://hg.mozilla.org/mozilla-central/rev/40e602313a2a

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #14)

> Optional nit: Personally I would find this more readable if childIdx was just
> named i or index.

I would save this :) I like index names if they are more descriptive.

> >+    NS_ASSERTION(childAcc == childAccAtOffset,
> >+                 "These should be equal whenever we exit loop and childAcc != nsnull");
> 
> Do we really need this assertion?

I think we don't need it, just saved it.

> >+        (childIdx < childCount - 1 ||
> 
> Optional nit: I prefer (childIdx < (childCount -1) || ...

up to you, I don't have strong opinion

(In reply to comment #15)
> After chatting with David on IRC about the nits, we decided to go ahead and
> push this on Alexander's behalf to get it in ASAP:
> http://hg.mozilla.org/mozilla-central/rev/40e602313a2a

Thank you, guys!

(landed on 1.9.3)

Comment 17

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.3a5pre) Gecko/20100420 Minefield/3.7a5pre (.NET CLR 3.5.30729)