RESOLVED FIXED256526
AX: Avoid hitting the main thread during the construction of the isolated tree due to AXPrimaryScreenHeight.
https://bugs.webkit.org/show_bug.cgi?id=256526
Summary AX: Avoid hitting the main thread during the construction of the isolated tre...
Andres Gonzalez
Reported 2023-05-09 09:07:09 PDT
This is one of the factors contributing to irresponsiveness during page load of large documents.
Attachments
Patch (22.05 KB, patch)
2023-05-09 09:13 PDT, Andres Gonzalez
no flags
Patch (22.05 KB, patch)
2023-05-09 13:06 PDT, Andres Gonzalez
no flags
Radar WebKit Bug Importer
Comment 1 2023-05-09 09:07:24 PDT
Andres Gonzalez
Comment 2 2023-05-09 09:13:45 PDT
chris fleizach
Comment 3 2023-05-09 10:24:29 PDT
Comment on attachment 466293 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466293&action=review > COMMIT_MESSAGE:7 > +To accomplish not higging the main thread upon primary scrren height requests, the following changes were necessary: higging -> hitting scrren -> screen > Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:84 > + return screenRectForPrimaryScreen(); will this be expensive to call on initialization?
Tyler Wilcock
Comment 4 2023-05-09 10:24:39 PDT
Comment on attachment 466293 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466293&action=review > COMMIT_MESSAGE:7 > +To accomplish not higging the main thread upon primary scrren height requests, the following changes were necessary: typo: higging, hitting > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:308 > + if (object.isScrollView() || object.isWebArea()) { > + // For the ScrollView and WebArea objects, cache the TitleAttributeValue and DescriptionAttributeValue eagerly to avoid hitting the main thread in getOrRetrievePropertyValue during the construction of the isolated tree. > + setProperty(AXPropertyName::TitleAttributeValue, object.titleAttributeValue().isolatedCopy()); > + setProperty(AXPropertyName::DescriptionAttributeValue, object.descriptionAttributeValue().isolatedCopy()); I believe one or both of these results in calls to textUnderElement, right? Since the point of this patch to facilitate fast startup, is it possible that these functions will recursively call textUnderElement all the way to the leaves?
Andres Gonzalez
Comment 5 2023-05-09 13:06:02 PDT
Andres Gonzalez
Comment 6 2023-05-09 13:12:26 PDT
(In reply to chris fleizach from comment #3) > Comment on attachment 466293 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=466293&action=review > > > COMMIT_MESSAGE:7 > > +To accomplish not higging the main thread upon primary scrren height requests, the following changes were necessary: > > higging -> hitting > scrren -> screen Fixed. > > > Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:84 > > + return screenRectForPrimaryScreen(); > > will this be expensive to call on initialization? In this particular line, it is not initialization, this is the live object on demand. But we do call it on construction of the AXGeometryManager object and it is not expensive. the problem was that it has to be called on the main thread, not that it was expensive.
Andres Gonzalez
Comment 7 2023-05-09 13:14:22 PDT
(In reply to Tyler Wilcock from comment #4) > Comment on attachment 466293 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=466293&action=review > > > COMMIT_MESSAGE:7 > > +To accomplish not higging the main thread upon primary scrren height requests, the following changes were necessary: > > typo: higging, hitting Fixed. > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:308 > > + if (object.isScrollView() || object.isWebArea()) { > > + // For the ScrollView and WebArea objects, cache the TitleAttributeValue and DescriptionAttributeValue eagerly to avoid hitting the main thread in getOrRetrievePropertyValue during the construction of the isolated tree. > > + setProperty(AXPropertyName::TitleAttributeValue, object.titleAttributeValue().isolatedCopy()); > > + setProperty(AXPropertyName::DescriptionAttributeValue, object.descriptionAttributeValue().isolatedCopy()); > > I believe one or both of these results in calls to textUnderElement, right? > Since the point of this patch to facilitate fast startup, is it possible > that these functions will recursively call textUnderElement all the way to > the leaves? That doesn't happen for ScrollView or WebArea objects.
Andres Gonzalez
Comment 8 2023-05-09 13:17:55 PDT
(In reply to Andres Gonzalez from comment #7) > (In reply to Tyler Wilcock from comment #4) > > Comment on attachment 466293 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=466293&action=review > > > > > COMMIT_MESSAGE:7 > > > +To accomplish not higging the main thread upon primary scrren height requests, the following changes were necessary: > > > > typo: higging, hitting > > Fixed. > > > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:308 > > > + if (object.isScrollView() || object.isWebArea()) { > > > + // For the ScrollView and WebArea objects, cache the TitleAttributeValue and DescriptionAttributeValue eagerly to avoid hitting the main thread in getOrRetrievePropertyValue during the construction of the isolated tree. > > > + setProperty(AXPropertyName::TitleAttributeValue, object.titleAttributeValue().isolatedCopy()); > > > + setProperty(AXPropertyName::DescriptionAttributeValue, object.descriptionAttributeValue().isolatedCopy()); > > > > I believe one or both of these results in calls to textUnderElement, right? > > Since the point of this patch to facilitate fast startup, is it possible > > that these functions will recursively call textUnderElement all the way to > > the leaves? > > That doesn't happen for ScrollView or WebArea objects. I.e., textUnderElement is not called as a result of title or description AttributeValue for either of these object types.
EWS
Comment 9 2023-05-10 04:16:19 PDT
Committed 263896@main (5fae8467d69f): <https://commits.webkit.org/263896@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 466298 [details].
Note You need to log in before you can comment on or make changes to this bug.