Bug 263032 - AX: AccessibilityObject::listMarkerTextForNodeAndPosition overload that takes a VisiblePosition is not performant.
Summary: AX: AccessibilityObject::listMarkerTextForNodeAndPosition overload that takes...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-10-11 13:17 PDT by Andres Gonzalez
Modified: 2023-10-12 07:35 PDT (History)
11 users (show)

See Also:

Attachments
Patch (5.18 KB, patch)
2023-10-11 13:20 PDT, Andres Gonzalez 		no flags Details | Formatted Diff | Diff
Patch (6.15 KB, patch)
2023-10-11 16:52 PDT, Andres Gonzalez 		no flags Details | Formatted Diff | Diff
Patch (6.34 KB, patch)
2023-10-12 06:15 PDT, Andres Gonzalez 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2023-10-11 13:17:03 PDT 
.
Comment 1 Radar WebKit Bug Importer 2023-10-11 13:17:16 PDT 
<rdar://problem/116826061>
Comment 2 Andres Gonzalez 2023-10-11 13:20:43 PDT 
Created attachment 468177 [details]
Patch
Comment 3 Tyler Wilcock 2023-10-11 13:33:30 PDT 
Comment on attachment 468177 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1776
> +            builder.append(listMarkerTextForNodeAndPosition(it.node(), visiblePositionRange.start.deepEquivalent()));

I think this will harm performance, as it turns an existing VisiblePosition into a Position, and forces a conversion of that Position back into a VisiblePosition again, which is the expensive operation we are trying to avoid.
Comment 4 Andres Gonzalez 2023-10-11 13:37:26 PDT 
(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 468177 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468177&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1776
> > +            builder.append(listMarkerTextForNodeAndPosition(it.node(), visiblePositionRange.start.deepEquivalent()));
> 
> I think this will harm performance, as it turns an existing VisiblePosition
> into a Position, and forces a conversion of that Position back into a
> VisiblePosition again, which is the expensive operation we are trying to
> avoid.

No, what we are trying to avoid is the determination whether a VisiblePosition is at the beginning of a line, that's the expensive task, not the construction of a VisiblePosition.
Comment 5 Tyler Wilcock 2023-10-11 13:41:52 PDT 
(In reply to Andres Gonzalez from comment #4)
> (In reply to Tyler Wilcock from comment #3)
> > Comment on attachment 468177 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=468177&action=review
> > 
> > > Source/WebCore/accessibility/AccessibilityObject.cpp:1776
> > > +            builder.append(listMarkerTextForNodeAndPosition(it.node(), visiblePositionRange.start.deepEquivalent()));
> > 
> > I think this will harm performance, as it turns an existing VisiblePosition
> > into a Position, and forces a conversion of that Position back into a
> > VisiblePosition again, which is the expensive operation we are trying to
> > avoid.
> 
> No, what we are trying to avoid is the determination whether a
> VisiblePosition is at the beginning of a line, that's the expensive task,
> not the construction of a VisiblePosition.
That's not true. It is the construction of VisiblePosition from Position that is expensive. That is what samples show.
Comment 6 Tyler Wilcock 2023-10-11 14:34:59 PDT 
This is all from one sample taken from https://developer.android.com/reference/android/view/View#attr_android:screenReaderFocusable. 27110 samples constructing VisiblePosition from Position, and there's even more smaller chunks in the hundreds of samples that I didn't include in this summary.

Sample Count, Symbol
10605 WebCore::AccessibilityObject::attributedStringForRange(WebCore::SimpleRange const&, WebCore::AXCoreObject::SpellCheck) const (in WebCore)
10495     WebCore::AccessibilityObject::contentForRange(WebCore::SimpleRange const&, WebCore::AXCoreObject::SpellCheck) const (in WebCore)
9593           WebCore::AccessibilityObject::listMarkerTextForNodeAndPosition(WebCore::Node*, WebCore::Position&&) (in WebCore)
4874               WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity) (in WebCore)
4873                   WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in WebCore)
2523                       WebCore::previousCandidate(WebCore::Position const&) (in WebCore)
4717            WebCore::listMarkerText(WebCore::RenderListItem&, WebCore::VisiblePosition const&) (in WebCore)
4666               WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity) (in WebCore)
4666                   WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in WebCore)

6796 WebCore::AccessibilityObject::textContent() const (in WebCore)
6787     WebCore::AccessibilityObject::stringForRange(WebCore::SimpleRange const&) const (in WebCore)
6720         WebCore::AccessibilityObject::listMarkerTextForNodeAndPosition(WebCore::Node*, WebCore::Position&&) (in WebCore)
3392             WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity) (in WebCore)
3392                 WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in WebCore)
1726                      WebCore::previousCandidate(WebCore::Position const&) (in WebCore)
725                            WebCore::PositionIterator::decrement() (in WebCore)
3327         WebCore::listMarkerText(WebCore::RenderListItem&, WebCore::VisiblePosition const&) (in WebCore)
3294             WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity) (in WebCore)
3294                 WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in WebCore)
1741                       WebCore::previousCandidate(WebCore::Position const&) (in WebCore)

6828 WebCore::AccessibilityObject::attributedStringForRange(WebCore::SimpleRange const&, WebCore::AXCoreObject::SpellCheck) const (in WebCore)
6809     WebCore::AccessibilityObject::contentForRange(WebCore::SimpleRange const&, WebCore::AXCoreObject::SpellCheck) const (in WebCore)
6470         WebCore::AccessibilityObject::listMarkerTextForNodeAndPosition(WebCore::Node*, WebCore::Position&&) (in WebCore)
3291              WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity) (in WebCore)
3290                  WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in WebCore)
1913                       WebCore::previousCandidate(WebCore::Position const&) (in WebCore)
3179          WebCore::listMarkerText(WebCore::RenderListItem&, WebCore::VisiblePosition const&) (in WebCore)
3163              WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity) (in WebCore)
3163                  WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in WebCore)
1921                       WebCore::previousCandidate(WebCore::Position const&) (in WebCore)

4470 WebCore::AccessibilityObject::textContent() const (in WebCore)
4466     WebCore::AccessibilityObject::stringForRange(WebCore::SimpleRange const&) const (in WebCore)
4443         WebCore::AccessibilityObject::listMarkerTextForNodeAndPosition(WebCore::Node*, WebCore::Position&&) (in WebCore)
2223             WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity) (in WebCore)
2223                 WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in WebCore)
1347                      WebCore::previousCandidate(WebCore::Position const&) (in WebCore)
2219          WebCore::listMarkerText(WebCore::RenderListItem&, WebCore::VisiblePosition const&) (in WebCore)
2207              WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity) (in WebCore)
2207                  WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in WebCore)
1358                      WebCore::previousCandidate(WebCore::Position const&) (in WebCore)
Comment 7 Andres Gonzalez 2023-10-11 16:52:15 PDT 
Created attachment 468181 [details]
Patch
Comment 8 Andres Gonzalez 2023-10-11 16:56:36 PDT 
(In reply to Tyler Wilcock from comment #6)
> This is all from one sample taken from
> https://developer.android.com/reference/android/view/View#attr_android:
> screenReaderFocusable. 27110 samples constructing VisiblePosition from
> Position, and there's even more smaller chunks in the hundreds of samples
> that I didn't include in this summary.
> 
> Sample Count, Symbol
> 10605
> WebCore::AccessibilityObject::attributedStringForRange(WebCore::SimpleRange
> const&, WebCore::AXCoreObject::SpellCheck) const (in WebCore)
> 10495     WebCore::AccessibilityObject::contentForRange(WebCore::SimpleRange
> const&, WebCore::AXCoreObject::SpellCheck) const (in WebCore)
> 9593          
> WebCore::AccessibilityObject::listMarkerTextForNodeAndPosition(WebCore::
> Node*, WebCore::Position&&) (in WebCore)
> 4874              
> WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&,
> WebCore::Affinity) (in WebCore)
> 4873                  
> WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in
> WebCore)
> 2523                       WebCore::previousCandidate(WebCore::Position
> const&) (in WebCore)
> 4717            WebCore::listMarkerText(WebCore::RenderListItem&,
> WebCore::VisiblePosition const&) (in WebCore)
> 4666              
> WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&,
> WebCore::Affinity) (in WebCore)
> 4666                  
> WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in
> WebCore)
> 
> 6796 WebCore::AccessibilityObject::textContent() const (in WebCore)
> 6787     WebCore::AccessibilityObject::stringForRange(WebCore::SimpleRange
> const&) const (in WebCore)
> 6720        
> WebCore::AccessibilityObject::listMarkerTextForNodeAndPosition(WebCore::
> Node*, WebCore::Position&&) (in WebCore)
> 3392             WebCore::VisiblePosition::VisiblePosition(WebCore::Position
> const&, WebCore::Affinity) (in WebCore)
> 3392                
> WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in
> WebCore)
> 1726                      WebCore::previousCandidate(WebCore::Position
> const&) (in WebCore)
> 725                            WebCore::PositionIterator::decrement() (in
> WebCore)
> 3327         WebCore::listMarkerText(WebCore::RenderListItem&,
> WebCore::VisiblePosition const&) (in WebCore)
> 3294             WebCore::VisiblePosition::VisiblePosition(WebCore::Position
> const&, WebCore::Affinity) (in WebCore)
> 3294                
> WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in
> WebCore)
> 1741                       WebCore::previousCandidate(WebCore::Position
> const&) (in WebCore)
> 
> 6828
> WebCore::AccessibilityObject::attributedStringForRange(WebCore::SimpleRange
> const&, WebCore::AXCoreObject::SpellCheck) const (in WebCore)
> 6809     WebCore::AccessibilityObject::contentForRange(WebCore::SimpleRange
> const&, WebCore::AXCoreObject::SpellCheck) const (in WebCore)
> 6470        
> WebCore::AccessibilityObject::listMarkerTextForNodeAndPosition(WebCore::
> Node*, WebCore::Position&&) (in WebCore)
> 3291             
> WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&,
> WebCore::Affinity) (in WebCore)
> 3290                 
> WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in
> WebCore)
> 1913                       WebCore::previousCandidate(WebCore::Position
> const&) (in WebCore)
> 3179          WebCore::listMarkerText(WebCore::RenderListItem&,
> WebCore::VisiblePosition const&) (in WebCore)
> 3163             
> WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&,
> WebCore::Affinity) (in WebCore)
> 3163                 
> WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in
> WebCore)
> 1921                       WebCore::previousCandidate(WebCore::Position
> const&) (in WebCore)
> 
> 4470 WebCore::AccessibilityObject::textContent() const (in WebCore)
> 4466     WebCore::AccessibilityObject::stringForRange(WebCore::SimpleRange
> const&) const (in WebCore)
> 4443        
> WebCore::AccessibilityObject::listMarkerTextForNodeAndPosition(WebCore::
> Node*, WebCore::Position&&) (in WebCore)
> 2223             WebCore::VisiblePosition::VisiblePosition(WebCore::Position
> const&, WebCore::Affinity) (in WebCore)
> 2223                
> WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in
> WebCore)
> 1347                      WebCore::previousCandidate(WebCore::Position
> const&) (in WebCore)
> 2219          WebCore::listMarkerText(WebCore::RenderListItem&,
> WebCore::VisiblePosition const&) (in WebCore)
> 2207             
> WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&,
> WebCore::Affinity) (in WebCore)
> 2207                 
> WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (in
> WebCore)
> 1358                      WebCore::previousCandidate(WebCore::Position
> const&) (in WebCore)

A second try. We probably have a more extended problem if creating VisiblePositions is that expensive since we use them heavily. Something to investigate further.
Comment 9 Andres Gonzalez 2023-10-12 06:15:28 PDT 
Created attachment 468187 [details]
Patch
Comment 10 EWS 2023-10-12 07:35:40 PDT 
Committed 269250@main (168604874bc2): <https://commits.webkit.org/269250@main>

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