Bug 171443 - AX: Improve performance of addChildren()/childrenChanged()
Summary: AX: Improve performance of addChildren()/childrenChanged()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-28 12:15 PDT by Nan Wang
Modified: 2017-04-29 11:55 PDT (History)
11 users (show)

See Also:


Attachments
patch (15.03 KB, patch)
2017-04-28 12:32 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (15.13 KB, patch)
2017-04-28 18:24 PDT, Nan Wang
cfleizach: review+
n_wang: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2017-04-28 12:15:51 PDT
When childrenChanged() being called on some element, we are resetting the entire tree. 
Need to reduce the amount of unnecessary work.

<rdar://problem/31349636>
Comment 1 Nan Wang 2017-04-28 12:32:32 PDT
Created attachment 308572 [details]
patch
Comment 2 chris fleizach 2017-04-28 17:07:06 PDT
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
Comment 3 Nan Wang 2017-04-28 17:22:19 PDT
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
Comment 4 chris fleizach 2017-04-28 17:34:44 PDT
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
Comment 5 Nan Wang 2017-04-28 17:44:43 PDT
(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.
Comment 6 Nan Wang 2017-04-28 18:23:46 PDT
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
Comment 7 Nan Wang 2017-04-28 18:24:17 PDT
Created attachment 308639 [details]
patch

updated from review
Comment 8 Nan Wang 2017-04-29 11:54:24 PDT
Comment on attachment 308639 [details]
patch

The failing tests are unrelated to this patch. Will commit manually.
Comment 9 Nan Wang 2017-04-29 11:55:20 PDT
Committed r215975: <http://trac.webkit.org/changeset/215975>