Bug 263032 – AX: AccessibilityObject::listMarkerTextForNodeAndPosition overload that takes a VisiblePosition is not performant.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

.

[Radar WebKit Bug Importer]

<rdar://problem/116826061>

[Andres Gonzalez]

Created attachment 468177 [details]
Patch

[Tyler Wilcock]

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.

[Andres Gonzalez]

(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.

[Tyler Wilcock]

(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.

[Tyler Wilcock]

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)

[Andres Gonzalez]

Created attachment 468181 [details]
Patch

[Andres Gonzalez]

(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.

[Andres Gonzalez]

Created attachment 468187 [details]
Patch

[EWS]

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].