Bug 171443 – AX: Improve performance of addChildren()/childrenChanged()

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Nan Wang]

When childrenChanged() being called on some element, we are resetting the entire tree. 
Need to reduce the amount of unnecessary work.

<rdar://problem/31349636>

[Nan Wang]

Created attachment 308572 [details]
patch

[chris fleizach]

Comment on attachment 308572 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308572&action=review

> Source/WebCore/ChangeLog:8
> +        There's a lot of unnecessary work happening when childrenChanged() being called.

"is" being called

> Source/WebCore/ChangeLog:15
> +        2. In addChild() method we are calling accessibilityIsIgnored() on each child and that 

In the...

> Source/WebCore/ChangeLog:19
> +        3. Reduced the amount work of ARIA text controls updating its parents in childrenChanged() 

text controls "performs when" updating

> Source/WebCore/ChangeLog:22
> +        No new tests since this didn't change any functionality. 

I think we could add a test for this by getting a reference to an axObject, marking a subtree dirty, then verifying that reference (in the test) is still valid

in the old case, that element would have become invalid because it was thrown away and recreated. In the new case it should still be there even after the sub tree was cleared (likewise, we can probably get a reference to a dirty subtree object and ensure that it is indeed invalid after the update)

> Source/WebCore/ChangeLog:38
> +        (WebCore::AccessibilityObject::setNeedsToUpdateSubTree):

I suspect SubTree should be Subtree (pls check if there are other instances in WebKit code)

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:348
> +    // Only clear the child's children when we know it's in the updating chain in order to avoid unnecessary works.

work.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:351
> +        // Pass m_subTreeDirty flag down to the child so that children cache get reset properly.

gets

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:376
> +    // reset the child's m_isIgnoredFromParentData since we are done adding that child and its children.

Reset

> Source/WebCore/accessibility/AccessibilityObject.h:1095
> +    void setIsIgnoredFromParentData(AccessibilityIsIgnoredFromParentData data) { m_isIgnoredFromParentData = data; }

should this be AccessibilityIsIgnoredFromParentData&

> Source/WebCore/accessibility/AccessibilityObject.h:1096
> +    void setIsIgnoredFromParentDataForChild(AccessibilityObject*);

const AccessibilityObject

[Nan Wang]

Comment on attachment 308572 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308572&action=review

>> Source/WebCore/ChangeLog:22
>> +        No new tests since this didn't change any functionality. 
> 
> I think we could add a test for this by getting a reference to an axObject, marking a subtree dirty, then verifying that reference (in the test) is still valid
> 
> in the old case, that element would have become invalid because it was thrown away and recreated. In the new case it should still be there even after the sub tree was cleared (likewise, we can probably get a reference to a dirty subtree object and ensure that it is indeed invalid after the update)

I think in the old case, the element is still valid since its children list is being cleared instead of itself being thrown away. Here we just reduce the work that is traversing the non affected subtree and retrieving the element that supposed to be a child.

>> Source/WebCore/ChangeLog:38
>> +        (WebCore::AccessibilityObject::setNeedsToUpdateSubTree):
> 
> I suspect SubTree should be Subtree (pls check if there are other instances in WebKit code)

Right, it's Subtree

[chris fleizach]

Comment on attachment 308572 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308572&action=review

>>> Source/WebCore/ChangeLog:22
>>> +        No new tests since this didn't change any functionality. 
>> 
>> I think we could add a test for this by getting a reference to an axObject, marking a subtree dirty, then verifying that reference (in the test) is still valid
>> 
>> in the old case, that element would have become invalid because it was thrown away and recreated. In the new case it should still be there even after the sub tree was cleared (likewise, we can probably get a reference to a dirty subtree object and ensure that it is indeed invalid after the update)
> 
> I think in the old case, the element is still valid since its children list is being cleared instead of itself being thrown away. Here we just reduce the work that is traversing the non affected subtree and retrieving the element that supposed to be a child.

What if we modify my comment instead of grabbing the parent we grab a sibling (or child of that sibling) child and verify that that is not thrown away

[Nan Wang]

(In reply to chris fleizach from comment #4)
> Comment on attachment 308572 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308572&action=review
> 
> >>> Source/WebCore/ChangeLog:22
> >>> +        No new tests since this didn't change any functionality. 
> >> 
> >> I think we could add a test for this by getting a reference to an axObject, marking a subtree dirty, then verifying that reference (in the test) is still valid
> >> 
> >> in the old case, that element would have become invalid because it was thrown away and recreated. In the new case it should still be there even after the sub tree was cleared (likewise, we can probably get a reference to a dirty subtree object and ensure that it is indeed invalid after the update)
> > 
> > I think in the old case, the element is still valid since its children list is being cleared instead of itself being thrown away. Here we just reduce the work that is traversing the non affected subtree and retrieving the element that supposed to be a child.
> 
> What if we modify my comment instead of grabbing the parent we grab a
> sibling (or child of that sibling) child and verify that that is not thrown
> away
clearChildren() only empties the m_children list (not invalidating the child) but the sibling/sibling child should be still valid even in the old case. What I did to improve is that we don't need to empty the sibling's children list and don't need to dive into the sibling tree.

[Nan Wang]

Comment on attachment 308572 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308572&action=review

>> Source/WebCore/accessibility/AccessibilityObject.h:1096
>> +    void setIsIgnoredFromParentDataForChild(AccessibilityObject*);
> 
> const AccessibilityObject

We are actually modifying the child object here

[Nan Wang]

Created attachment 308639 [details]
patch

updated from review

[Nan Wang]

Comment on attachment 308639 [details]
patch

The failing tests are unrelated to this patch. Will commit manually.

[Nan Wang]

Committed r215975: <http://trac.webkit.org/changeset/215975>