135485 - Active Accessibility: should we cache accessible children of an accessible node?

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Aaron Leventhal]

Recently I asked screen reader vendors if they used get_accChildCount, or
whether they simply walked siblings until they hit null.

If they do both, they will be forcing our accessibility code to do the same work
twice.

Two of the vendors explained that they still need to use get_accChildCount(), no
matter what - and that caching the children in memory would help them.

We need to think about the most efficient way to do this.

Comment 1

[Aaron Leventhal]

We need to optimize this sequence:
IAccessible::get_accChildCount(); 
QueryInterface(IID_IEnumVARIANT); 
IEnumVARIANT::Next();

Therefore, each Accessible should cache the number of children it has, and build
up the array to pass back as it goes, so that IEnumVARIANT::Next can simply pass
through the data it already has.

This sounds like no fun to fix, but it's useful.

Comment 2

[Aaron Leventhal]

Attachment: Caches first child and next sibling of each Accessible, supports IEnumVARIANT.

This really seems faster, looking forward to getting some measurements from one
of the screen reader vendors.

Reviewers, sorry for the long patch - I could help but clean up the bad
formatting and variable names in the old code.

Seeking r=

Comment 3

[John Gaunt (redfive)]

Comment on attachment 95988 [details] [diff] [review]
Caches first child and next sibling of each Accessible, supports IEnumVARIANT.

What happens in GetCachedChild if there has been a new child attached? If we
have been through before it looks like we won't update the child count. Maybe
if the arg is -1, we need to zero the childcount and then go through the
function.

Also the VARIANTs you create in GetCachedChild() need to be cleaned up. There
is a destruction method corresponding to the VariantInit() method that needs to
be called. You may be able to just call it at the end of the method and not
have to call it every time through the loop. (although varResult may need to be
cleaned up each time through the loop because accNavigate() call VariantInit()
on it ) Pretty sure something is leaking there.

Similar case in Next() with the varResult ( varStart will be freed by the
caller ), wait, reverse that, varStart needs to be freed as the varResults are
going into the VARIANT and being returned

maybe a comment that mENUMVariantPosition isn't the current accessible's
position but a cache of where the "cursor" is in the list of children.

Also in AccNavigate(), if we haven't cached the children yet, we won't return
anything here.

case NAVDIR_FIRSTCHILD:
-      mAccessible->GetAccFirstChild(getter_AddRefs(acc));
+      if (xpAccessibleStart == mXPAccessible && mCachedFirstChild) {
+	 msaaAccessible = mCachedFirstChild;
+      }
+      else if (mCachedChildCount) {
+	
xpAccessibleStart->GetAccFirstChild(getter_AddRefs(xpAccessibleResult));
+      }
       break;

I think that's it. In general, realy good. I like the changes to the variable
names and the general cleanup you've done.

Comment 4

[Aaron Leventhal]

> What happens in GetCachedChild if there has been a new child attached?
If the DOM is modified, screen readers don't update their internal model yet
anyway. I'm going to support mutation events so they can.
The only time they'll get a new accessible of a node they already have, after
the initial page load, is from events. In that case, we're creating a new
Accessible for that nsIAccessible anyway, so it will be fresh and not have
cached children.

All VariantClear does for VT_IDISPATCH variants is do a Release(). I have
replaced the Release() call in GetCachedChild() with a VariantClear. I don't
want to add a VariantClean call in Next() though, because I have just the right
number of AddRef's, and I would have to add an extra AddRef to balance it out.

Ooh, thanks for catching the NAVDIR_FIRSTCHILD thing:
It should be:
    case NAVDIR_FIRSTCHILD:
      if (mCachedChildCount == -1 || xpAccessibleStart != mXPAccessible) {
        xpAccessibleStart->GetAccFirstChild(getter_AddRefs(xpAccessibleResult));
      }
      else if (mCachedFirstChild) {
        msaaAccessible = mCachedFirstChild;
      }
      break;

Comment 5

[Aaron Leventhal]

Attachment: Fixes John's concerns, plus some leaks!

John, I got off my lazy butt and actually tested for leaks this time. Just what
I was afraid of - there were some. Took me forever to figure it out, the
problem was in ::Next(). I forgot about releasing in the case where the
"cursor" is not starting at 0.

Anyway, here it is, ready for r=.

I also found some other serious bugs in our accessibility code, which I'm going
to file now.

Comment 6

[John Gaunt (redfive)]

Comment on attachment 96140 [details] [diff] [review]
Fixes John's concerns, plus some leaks!

Looks good,
r=jgaunt

Comment 7

[Alec Flett]

Comment on attachment 96140 [details] [diff] [review]
Fixes John's concerns, plus some leaks!

sr=alecf

Comment 8

[Jerome Lacoste]

Could this be the bug that caused a 3% startup perf regression (bug 164209)?

Comment 9

[Aaron Leventhal]

this has been in for a while