We do a lot of repeated live tree reads in these codepaths, so we should make sure we are caching is-ignored for better performance.
<rdar://problem/99501850>
Created attachment 462104 [details] Patch
Comment on attachment 462104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462104&action=review > COMMIT_MESSAGE:12 > +This patch also AXAttributeCacheEnabler to not disable the AXComputedObjectAttributeCache also modifies(?) > Source/WebCore/accessibility/AXObjectCache.h:605 > + bool m_wasAlreadyCachingAttributes { false }; wasAlready feel redundant to just having "m_cachingAttributes"... or maybe something shorter "m_inAttributeCaching"
(In reply to Tyler Wilcock from comment #2) > Created attachment 462104 [details] > Patch --- a/Source/WebCore/accessibility/AXObjectCache.h +++ a/Source/WebCore/accessibility/AXObjectCache.h + bool m_wasAlreadyCachingAttributes { false }; Why do we need this extra flag? --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp @@ -205,6 +205,8 @@ void AXIsolatedTree::generateSubtree(AXCoreObject& axObject) if (!axObject.objectID().isValid()) return; + // We're about to a lot of read-only work, so start the attribute cache. "to a lot" -> "to do a lot" What do you mean by "read-only work"? Not read-only for this isolated tree. @@ -533,6 +535,9 @@ void AXIsolatedTree::updateChildren(AccessibilityObject& axObject, ResolveNodeCh if (!axObject.document() || !axObject.document()->hasLivingRenderTree()) return; + // We're about to a lot of read-only work, so start the attribute cache. Dito.
Created attachment 466469 [details] Patch
(In reply to chris fleizach from comment #3) > Comment on attachment 462104 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=462104&action=review > > > COMMIT_MESSAGE:12 > > +This patch also AXAttributeCacheEnabler to not disable the AXComputedObjectAttributeCache > > also modifies(?) > > > Source/WebCore/accessibility/AXObjectCache.h:605 > > + bool m_wasAlreadyCachingAttributes { false }; > > wasAlready feel redundant to just having "m_cachingAttributes"... or maybe > something shorter "m_inAttributeCaching" The intent I'm trying to capture is whether AXObjectCache::computedObjectAttributeCache existed before this AXAttributeCacheEnabler was created (i.e. we were already caching attributes). If so, we need to set this flag so we don't destroy it when this AXAttributeCacheEnabler is destroyed. I feel like m_inAttributeCaching or m_cachingAttributes don't capture that as well as m_wasAlreadyCachingAttributes...but I don't feel too strongly about it.
(In reply to Andres Gonzalez from comment #4) > (In reply to Tyler Wilcock from comment #2) > > Created attachment 462104 [details] > > Patch > > --- a/Source/WebCore/accessibility/AXObjectCache.h > +++ a/Source/WebCore/accessibility/AXObjectCache.h > > + bool m_wasAlreadyCachingAttributes { false }; > > Why do we need this extra flag? I tried to describe this in the commit message, but let me know if that needs some more clarity.
(In reply to Tyler Wilcock from comment #5) > Created attachment 466469 [details] > Patch --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp + // We're about to a lot of read-only work, so start the attribute cache. about to a lot -> about to do a lot ? Is traversing the live tree always read-only? I think there are cases where getting the children for an object, for instance, can change the role of that object.
(In reply to Tyler Wilcock from comment #6) > (In reply to chris fleizach from comment #3) > > Comment on attachment 462104 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=462104&action=review > > > > > COMMIT_MESSAGE:12 > > > +This patch also AXAttributeCacheEnabler to not disable the AXComputedObjectAttributeCache > > > > also modifies(?) > > > > > Source/WebCore/accessibility/AXObjectCache.h:605 > > > + bool m_wasAlreadyCachingAttributes { false }; > > > > wasAlready feel redundant to just having "m_cachingAttributes"... or maybe > > something shorter "m_inAttributeCaching" > The intent I'm trying to capture is whether > AXObjectCache::computedObjectAttributeCache existed before this > AXAttributeCacheEnabler was created (i.e. we were already caching > attributes). If so, we need to set this flag so we don't destroy it when > this AXAttributeCacheEnabler is destroyed. > > I feel like m_inAttributeCaching or m_cachingAttributes don't capture that > as well as m_wasAlreadyCachingAttributes...but I don't feel too strongly > about it. Agree that it should be a shorter name. Where is this flag turned off? generateSubtree and updateChildren will turn it on but not off, i.e., once it is on, it will be on until something else turns it off. I believe this whole mechanism is quite weak and bug prone.
Created attachment 466483 [details] Patch
(In reply to Andres Gonzalez from comment #9) > (In reply to Tyler Wilcock from comment #6) > > (In reply to chris fleizach from comment #3) > > > Comment on attachment 462104 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=462104&action=review > > > > > > > COMMIT_MESSAGE:12 > > > > +This patch also AXAttributeCacheEnabler to not disable the AXComputedObjectAttributeCache > > > > > > also modifies(?) > > > > > > > Source/WebCore/accessibility/AXObjectCache.h:605 > > > > + bool m_wasAlreadyCachingAttributes { false }; > > > > > > wasAlready feel redundant to just having "m_cachingAttributes"... or maybe > > > something shorter "m_inAttributeCaching" > > The intent I'm trying to capture is whether > > AXObjectCache::computedObjectAttributeCache existed before this > > AXAttributeCacheEnabler was created (i.e. we were already caching > > attributes). If so, we need to set this flag so we don't destroy it when > > this AXAttributeCacheEnabler is destroyed. > > > > I feel like m_inAttributeCaching or m_cachingAttributes don't capture that > > as well as m_wasAlreadyCachingAttributes...but I don't feel too strongly > > about it. > > Agree that it should be a shorter name. > > Where is this flag turned off? generateSubtree and updateChildren will turn > it on but not off, i.e., once it is on, it will be on until something else > turns it off. I believe this whole mechanism is quite weak and bug prone. Re-capping our conversation off Bugzilla for posterity: The cache is turned off when the highest-most-stackframe AXAttributeCacheEnabler is destroyed, or when we post any notification. We should be safe from this causing bugs because we unconditionally bust the cache in every known way the tree mutates such that a different is-ignored value would be computed.
Also made the name of the flag shorter.
Committed 264524@main (9128760a1c6a): <https://commits.webkit.org/264524@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 466483 [details].