Bug 262941 – AX: -[WebAccessibilityObjectWrapper accessibilityIndexOfChild:] unnecessarily allocates an NSArray

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

[Tyler Wilcock]

This shows up on samples for some sites.

[Radar WebKit Bug Importer]

<rdar://problem/116717845>

[Tyler Wilcock]

Created attachment 468142 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #2)
> Created attachment 468142 [details]
> Patch

diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
index f39430c64827..48916783a1f4 100644
--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
@@ -3596,7 +3596,7 @@ - (BOOL)accessibilityShouldUseUniqueId
 }

 // API that AppKit uses for faster access
-- (NSUInteger)accessibilityIndexOfChild:(id)child
+- (NSUInteger)accessibilityIndexOfChild:(id)targetChild
 {
     RefPtr<AXCoreObject> backingObject = self.updateObjectBackingStore;
     if (!backingObject)
@@ -3605,29 +3605,27 @@ - (NSUInteger)accessibilityIndexOfChild:(id)child
     // Tree objects return their rows as their children. We can use the original method
     // here, because we won't gain any speed up.
     if (backingObject->isTree())
-        return [super accessibilityIndexOfChild:child];
+        return [super accessibilityIndexOfChild:targetChild];

-    NSArray *children = self.childrenVectorArray;
-    if (!children.count) {
+    const auto& children = backingObject->children();
+    if (!children.size()) {
         if (auto *renderWidgetChildren = [self renderWidgetChildren])
-            return [renderWidgetChildren indexOfObject:child];
+            return [renderWidgetChildren indexOfObject:targetChild];
 #if ENABLE(MODEL_ELEMENT)
         if (backingObject->isModel())
-            return backingObject->modelElementChildren().find(child);
+            return backingObject->modelElementChildren().find(targetChild);
 #endif
     }

-    NSUInteger count = [children count];
-    for (NSUInteger i = 0; i < count; ++i) {
-        WebAccessibilityObjectWrapper *wrapper = children[i];
-        auto* object = wrapper.axBackingObject;
-        if (!object)
+    size_t childCount = children.size();
+    for (size_t i = 0; i < childCount; i++) {
+        RefPtr child = children[i];

AG: you can use children[i] without this temp and the additional ref count. Also it wouldn't be needed to name the param targetChild and then keep the original child.

+        if (!child)
             continue;
-
-        if (wrapper == child || (object->isAttachment() && [wrapper attachmentView] == child))
+        WebAccessibilityObjectWrapper *childWrapper = child->wrapper();
+        if (childWrapper == targetChild || (child->isAttachment() && [childWrapper attachmentView] == targetChild))
             return i;
     }
-
     return NSNotFound;
 }

[Tyler Wilcock]

Created attachment 468152 [details]
Patch

[Tyler Wilcock]

> -    NSUInteger count = [children count];
> -    for (NSUInteger i = 0; i < count; ++i) {
> -        WebAccessibilityObjectWrapper *wrapper = children[i];
> -        auto* object = wrapper.axBackingObject;
> -        if (!object)
> +    size_t childCount = children.size();
> +    for (size_t i = 0; i < childCount; i++) {
> +        RefPtr child = children[i];
> 
> AG: you can use children[i] without this temp and the additional ref count.
> Also it wouldn't be needed to name the param targetChild and then keep the
> original child.
Removed the unnecessary RefPtr as you mentioned. I decided to keep the targetChild rename because I think it makes reading the code easier (otherwise we'd be comparing, e.g., childWrapper == child which doesn't read well in my opinion).

[chris fleizach]

Comment on attachment 468152 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3624
> +        WebAccessibilityObjectWrapper *childWrapper = children[i]->wrapper();

won't this incur a function call into Vector::indexedObject (whatever) the method name is each time? is that faster than making a local variable?
feel like we should know the answer to that before deciding which is better

[chris fleizach]

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3624
> +        WebAccessibilityObjectWrapper *childWrapper = children[i]->wrapper();

won't this incur a function call into Vector::indexedObject (whatever) the method name is each time? is that faster than making a local variable?
feel like we should know the answer to that before deciding which is better

[Andres Gonzalez]

(In reply to chris fleizach from comment #7)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468152&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3624
> > +        WebAccessibilityObjectWrapper *childWrapper = children[i]->wrapper();
> 
> won't this incur a function call into Vector::indexedObject (whatever) the
> method name is each time? is that faster than making a local variable?
> feel like we should know the answer to that before deciding which is better

Vector::operator[] is an inline, random access method, so I think this is better than creating a new RefPtr.

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #8)
> (In reply to chris fleizach from comment #7)
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=468152&action=review
> > 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3624
> > > +        WebAccessibilityObjectWrapper *childWrapper = children[i]->wrapper();
> > 
> > won't this incur a function call into Vector::indexedObject (whatever) the
> > method name is each time? is that faster than making a local variable?
> > feel like we should know the answer to that before deciding which is better
> 
> Vector::operator[] is an inline, random access method, so I think this is
> better than creating a new RefPtr.

we can also do 

+        auto& child = children[i];

that doesn't creat a new RefPtr, and you don't have to write children[i] evetywhere.

[Tyler Wilcock]

Created attachment 468157 [details]
Patch

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #9)
> (In reply to Andres Gonzalez from comment #8)
> > (In reply to chris fleizach from comment #7)
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=468152&action=review
> > > 
> > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3624
> > > > +        WebAccessibilityObjectWrapper *childWrapper = children[i]->wrapper();
> > > 
> > > won't this incur a function call into Vector::indexedObject (whatever) the
> > > method name is each time? is that faster than making a local variable?
> > > feel like we should know the answer to that before deciding which is better
> > 
> > Vector::operator[] is an inline, random access method, so I think this is
> > better than creating a new RefPtr.
> 
> we can also do 
> 
> +        auto& child = children[i];
> 
> that doesn't creat a new RefPtr, and you don't have to write children[i]
> evetywhere.
Fixed.

[EWS]

Committed 269187@main (f36f30e2e548): <https://commits.webkit.org/269187@main>

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