Bug 273959 – AX: it's inefficient to put large object types in AXPropertyValueVariant

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

[Dominic Mazzoni]

sizeof(AXPropertyValueVariant) is currently 88 bytes. If we take out URL and Path, it drops to 32 bytes, which is a very significant savings since we're storing lots of these for every isolated tree node.

Wrapping URL and Path in shared_ptr is an easy workaround. That way only the pointer is stored in the variant. We can't use unique_ptr because we store the variant in a HashMap that requires it to be copyable.

Another alternative for URL would be to store it as a String and then parse it into a URL when needed.

[Radar WebKit Bug Importer]

<rdar://problem/127821883>

[Dominic Mazzoni]

Created attachment 471341 [details]
Patch

[Andres Gonzalez]

(In reply to Dominic Mazzoni from comment #2)
> Created attachment 471341 [details]
> Patch

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 1cf2c43bbce0..1ada0332733d 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -156,7 +156,7 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
     setProperty(AXPropertyName::ExpandedTextValue, object.expandedTextValue().isolatedCopy());
     setProperty(AXPropertyName::SupportsExpandedTextValue, object.supportsExpandedTextValue());
     setProperty(AXPropertyName::ValueAutofillButtonType, static_cast<int>(object.valueAutofillButtonType()));
-    setProperty(AXPropertyName::URL, object.url().isolatedCopy());
+    setProperty(AXPropertyName::URL, std::make_shared<URL>(object.url().isolatedCopy()));
     setProperty(AXPropertyName::AccessKey, object.accessKey().isolatedCopy());
     setProperty(AXPropertyName::AutoCompleteValue, object.autoCompleteValue().isolatedCopy());
     setProperty(AXPropertyName::ColorValue, object.colorValue());
@@ -166,7 +166,7 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
     setProperty(AXPropertyName::LiveRegionStatus, object.liveRegionStatus().isolatedCopy());
     setProperty(AXPropertyName::LiveRegionRelevant, object.liveRegionRelevant().isolatedCopy());
     setProperty(AXPropertyName::LiveRegionAtomic, object.liveRegionAtomic());
-    setProperty(AXPropertyName::Path, object.elementPath());
+    setProperty(AXPropertyName::Path, std::make_shared<Path>(object.elementPath()));
     setProperty(AXPropertyName::HasHighlighting, object.hasHighlighting());
     setProperty(AXPropertyName::HasBoldFont, object.hasBoldFont());
     setProperty(AXPropertyName::HasItalicFont, object.hasItalicFont());
@@ -462,7 +462,7 @@ void AXIsolatedObject::setProperty(AXPropertyName propertyName, AXPropertyValueV
         [](uint64_t typedValue) { return !typedValue; },
         [](AccessibilityButtonState& typedValue) { return typedValue == AccessibilityButtonState::Off; },
         [](Color& typedValue) { return typedValue == Color(); },
-        [](URL& typedValue) { return typedValue == URL(); },
+        [](std::optional<URL>& typedValue) { return !typedValue || *typedValue == URL(); },

AG: shouldn't this be shared_ptr?

         [](LayoutRect& typedValue) { return typedValue == LayoutRect(); },
         [](IntPoint& typedValue) { return typedValue == IntPoint(); },
         [](IntRect& typedValue) { return typedValue == IntRect(); },
@@ -476,7 +476,7 @@ void AXIsolatedObject::setProperty(AXPropertyName propertyName, AXPropertyValueV
         [](Vector<AXID>& typedValue) { return typedValue.isEmpty(); },
         [](Vector<std::pair<AXID, AXID>>& typedValue) { return typedValue.isEmpty(); },
         [](Vector<String>& typedValue) { return typedValue.isEmpty(); },
-        [](Path& typedValue) { return typedValue.isEmpty(); },
+        [](std::optional<Path>& typedValue) { return !typedValue || typedValue->isEmpty(); },

AG: shouldn't this be shared_ptr?

         [](OptionSet<AXAncestorFlag>& typedValue) { return typedValue.isEmpty(); },
 #if PLATFORM(COCOA)
         [](RetainPtr<NSAttributedString>& typedValue) { return !typedValue; },
@@ -953,7 +953,7 @@ URL AXIsolatedObject::urlAttributeValue(AXPropertyName propertyName) const
 {
     auto value = m_propertyMap.get(propertyName);
     return WTF::switchOn(value,
-        [] (URL& typedValue) -> URL { return typedValue; },
+        [] (std::shared_ptr<URL>& typedValue) -> URL { return *typedValue.get(); },

AG: maybe we should assert here that typedValue is not null before deref-ing.

         [] (auto&) { return URL(); }
     );
 }
@@ -962,7 +962,7 @@ Path AXIsolatedObject::pathAttributeValue(AXPropertyName propertyName) const
 {
     auto value = m_propertyMap.get(propertyName);
     return WTF::switchOn(value,
-        [] (Path& typedValue) -> Path { return typedValue; },
+        [] (std::shared_ptr<Path>& typedValue) -> Path { return *typedValue.get(); },

AG: maybe we should assert here that typedValue is not null before deref-ing.

         [] (auto&) { return Path(); }
     );
 }

[Dominic Mazzoni]

Created attachment 471424 [details]
Patch

[Dominic Mazzoni]

I confirmed that tests that cover URL and Path pass in ITM, hopefully this should be ready to go.

> AG: shouldn't this be shared_ptr?

Yes, good catch. As you can see, I experimented with std::optional, but it didn't give any memory savings.

> AG: maybe we should assert here that typedValue is not null before deref-ing.

Good idea, done.

[chris fleizach]

Comment on attachment 471424 [details]
Patch

nice work!

[EWS]

Committed 278940@main (f4cd05fb5b19): <https://commits.webkit.org/278940@main>

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