Bug 257065 – AX: Stop clearing caching info for table components

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

We should stop clearing caching for table components. The table hierarchy is a bit weird and has a non standard parent child relationship.

[Radar WebKit Bug Importer]

<rdar://problem/109589379>

[chris fleizach]

Created attachment 466428 [details]
Patch

[Tyler Wilcock]

Comment on attachment 466428 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.h:746
> +    virtual bool shouldClearIgnoredDataForChild(AccessibilityObject&);

Can this be const?

> Source/WebCore/accessibility/AccessibilityTable.cpp:117
> +    if (is<AccessibilityTableRow>(child))
> +        return downcast<AccessibilityTableRow>(child).parentTable() != this;

Is checking that the parentTable == this necessary? `parentTable()` is not cheap, so I wonder if we should just allow any row like so until we have a real bug where this matters. Also, parentTable() returns nullptr if the table is not exposable, which seems like a strange reason to break the cache.

if (is<AccessibilityTableRow>(child))
    return false;

return AccessibilityObject::shouldClearIgnoredDataForChild(child);

> Source/WebCore/accessibility/AccessibilityTable.h:102
> +    bool shouldClearIgnoredDataForChild(AccessibilityObject&) override;

Can this be final instead of override?

> Source/WebCore/accessibility/AccessibilityTableColumn.h:60
> +    // Don't calculate ignored because this is not a "real" parent for its children.
> +    bool shouldClearIgnoredDataForChild(AccessibilityObject&) override { return false; };

Maybe I'm misreading, but I feel like this comment may not describe what this override is doing. We still calculate whether this object is ignored. Would something like this be accurate?

// Don't clear accessibilityIsIgnored caching for children of this object because it's a mock object inserted between actual DOM / render tree accessibility objects.

Also, can this be `final` instead of `override`? The former can help the optimizer reduce the effects of virtualization.

Ditto all of these comments for TableHeaderContainer.h.

[Tyler Wilcock]

Comment on attachment 466428 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:4270
> -    if (child->parentObject() != this) {
> +    if (shouldClearIgnoredDataForChild(*child)) {
>          child->clearIsIgnoredFromParentData();
>          return;
>      }

I wonder if the right thing to do is to just delete this block.

It was added here:

https://github.com/WebKit/WebKit/commit/ce4a8c5a1b6a9349a2c0d3bbc1df42c6b5ceb2e3

With test accessibility/insert-children-assert.html. But I just built release and debug with this block commented out and all the tests pass.

[chris fleizach]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 466428 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466428&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:4270
> > -    if (child->parentObject() != this) {
> > +    if (shouldClearIgnoredDataForChild(*child)) {
> >          child->clearIsIgnoredFromParentData();
> >          return;
> >      }
> 
> I wonder if the right thing to do is to just delete this block.
> 
> It was added here:
> 
> https://github.com/WebKit/WebKit/commit/
> ce4a8c5a1b6a9349a2c0d3bbc1df42c6b5ceb2e3
> 
> With test accessibility/insert-children-assert.html. But I just built
> release and debug with this block commented out and all the tests pass.

Does it seem like that was added for correctness in some other flow?

[Tyler Wilcock]

(In reply to chris fleizach from comment #5)
> (In reply to Tyler Wilcock from comment #4)
> > Comment on attachment 466428 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=466428&action=review
> > 
> > > Source/WebCore/accessibility/AccessibilityObject.cpp:4270
> > > -    if (child->parentObject() != this) {
> > > +    if (shouldClearIgnoredDataForChild(*child)) {
> > >          child->clearIsIgnoredFromParentData();
> > >          return;
> > >      }
> > 
> > I wonder if the right thing to do is to just delete this block.
> > 
> > It was added here:
> > 
> > https://github.com/WebKit/WebKit/commit/
> > ce4a8c5a1b6a9349a2c0d3bbc1df42c6b5ceb2e3
> > 
> > With test accessibility/insert-children-assert.html. But I just built
> > release and debug with this block commented out and all the tests pass.
> 
> Does it seem like that was added for correctness in some other flow?
Oops, looks like my GitHub link got broken. Here it is:

https://github.com/WebKit/WebKit/commit/ce4a8c5a1b6a9349a2c0d3bbc1df42c6b5ceb2e3

And actually, the `child->parentObject() != this` check was really added here:

https://github.com/WebKit/WebKit/commit/f031d0084a1585510bf5a6009a26fad4b4541255

If this prevents a bug, then we should have a test for that. But all tests pass without it. To me, this seems like added complexity with no measurable benefit (and measurable detriment -- a call to parentObject() and other virtual functions in one of our hotter codepaths).

What do you think?

[chris fleizach]

(In reply to Tyler Wilcock from comment #6)
> (In reply to chris fleizach from comment #5)
> > (In reply to Tyler Wilcock from comment #4)
> > > Comment on attachment 466428 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=466428&action=review
> > > 
> > > > Source/WebCore/accessibility/AccessibilityObject.cpp:4270
> > > > -    if (child->parentObject() != this) {
> > > > +    if (shouldClearIgnoredDataForChild(*child)) {
> > > >          child->clearIsIgnoredFromParentData();
> > > >          return;
> > > >      }
> > > 
> > > I wonder if the right thing to do is to just delete this block.
> > > 
> > > It was added here:
> > > 
> > > https://github.com/WebKit/WebKit/commit/
> > > ce4a8c5a1b6a9349a2c0d3bbc1df42c6b5ceb2e3
> > > 
> > > With test accessibility/insert-children-assert.html. But I just built
> > > release and debug with this block commented out and all the tests pass.
> > 
> > Does it seem like that was added for correctness in some other flow?
> Oops, looks like my GitHub link got broken. Here it is:
> 
> https://github.com/WebKit/WebKit/commit/
> ce4a8c5a1b6a9349a2c0d3bbc1df42c6b5ceb2e3
> 
> And actually, the `child->parentObject() != this` check was really added
> here:
> 
> https://github.com/WebKit/WebKit/commit/
> f031d0084a1585510bf5a6009a26fad4b4541255
> 
> If this prevents a bug, then we should have a test for that. But all tests
> pass without it. To me, this seems like added complexity with no measurable
> benefit (and measurable detriment -- a call to parentObject() and other
> virtual functions in one of our hotter codepaths).
> 
> What do you think?

Meant to improve performance but no way to check that. Do you think this will somehow regress performance? It's sort of hard for me to believe

[Tyler Wilcock]

(In reply to chris fleizach from comment #7)
> (In reply to Tyler Wilcock from comment #6)
> > (In reply to chris fleizach from comment #5)
> > > (In reply to Tyler Wilcock from comment #4)
> > > > Comment on attachment 466428 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=466428&action=review
> > > > 
> > > > > Source/WebCore/accessibility/AccessibilityObject.cpp:4270
> > > > > -    if (child->parentObject() != this) {
> > > > > +    if (shouldClearIgnoredDataForChild(*child)) {
> > > > >          child->clearIsIgnoredFromParentData();
> > > > >          return;
> > > > >      }
> > > > 
> > > > I wonder if the right thing to do is to just delete this block.
> > > > 
> > > > It was added here:
> > > > 
> > > > https://github.com/WebKit/WebKit/commit/
> > > > ce4a8c5a1b6a9349a2c0d3bbc1df42c6b5ceb2e3
> > > > 
> > > > With test accessibility/insert-children-assert.html. But I just built
> > > > release and debug with this block commented out and all the tests pass.
> > > 
> > > Does it seem like that was added for correctness in some other flow?
> > Oops, looks like my GitHub link got broken. Here it is:
> > 
> > https://github.com/WebKit/WebKit/commit/
> > ce4a8c5a1b6a9349a2c0d3bbc1df42c6b5ceb2e3
> > 
> > And actually, the `child->parentObject() != this` check was really added
> > here:
> > 
> > https://github.com/WebKit/WebKit/commit/
> > f031d0084a1585510bf5a6009a26fad4b4541255
> > 
> > If this prevents a bug, then we should have a test for that. But all tests
> > pass without it. To me, this seems like added complexity with no measurable
> > benefit (and measurable detriment -- a call to parentObject() and other
> > virtual functions in one of our hotter codepaths).
> > 
> > What do you think?
> 
> Meant to improve performance but no way to check that. Do you think this
> will somehow regress performance? It's sort of hard for me to believe
I think this patch as-is will definitely improve performance for tables. But I think removing this block:

    if (child->parentObject() != this) {
        child->clearIsIgnoredFromParentData();
        return;
    }

Will improve it even more, and for everything else too. It seems like we're bending over backwards to accommodate this logic when it's not even clear that it's necessary since no test relies on it.

[Tyler Wilcock]

If we remove that block, we don't need any of the changes in this patch, making our code as a whole more simple. This would improve performance for everything by a bit, and improve performance for tables by a lot.

[chris fleizach]

Created attachment 466437 [details]
Patch

[EWS]

Committed 264340@main (e1b48c3d6e0d): <https://commits.webkit.org/264340@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 466437 [details].