1926472 - selection_select in Charts-observable-plot is 12x slower than Chrome on Android

Status: RESOLVED DUPLICATE of bug 1844878

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[Denis Palmeiro [:denispal]]

We seem to spend a lot of time just adding new slots here.

Firefox: https://share.firefox.dev/4eT5qao
Chrome: https://share.firefox.dev/3BWWBxK

Function Definition:
https://github.com/mozilla/Speedometer/blob/537d5769ef9ef3fa9a236884eae4d109c0628833//resources/charts/dist/assets/plot-37d2a5fb.js#L894

function selection_select(select2) {
  if (typeof select2 !== "function")
    select2 = selector(select2);
  for (var groups2 = this._groups, m = groups2.length, subgroups = new Array(m), j = 0; j < m; ++j) {
    for (var group2 = groups2[j], n = group2.length, subgroup = subgroups[j] = new Array(n), node, subnode, i = 0; i < n; ++i) {
      if ((node = group2[i]) && (subnode = select2.call(node, node.__data__, i, group2))) {
        if ("__data__" in node)
          subnode.__data__ = node.__data__;
        subgroup[i] = subnode;
      }
    }
  }
  return new Selection$1(subgroups, this._parents);
}

Comment 1

[Denis Palmeiro [:denispal]]

Also seen in TodoItem from TodoMVC-WebComponents:

Firefox: https://share.firefox.dev/4eTfoZq
Chrome: https://share.firefox.dev/4heug67

Comment 2

[Markus Stange [:mstange]]

In the Windows profiles from https://shell--speedometer-preview.netlify.app/?suite=Charts-observable-plot&iterationCount=100 , I don't see growSlots being called during selection_select at all: https://share.firefox.dev/4f7eMzw
I wonder why we call it on Android but not on Windows.

Comment 3

[Iain Ireland [:iain]]

Comparing the Android profile here to the Windows profile here, the Android profile is calling the add_property hook on a lot of different kinds of binding (SVG(Line|Path|Rect|Text|Title|Circle)Element_Binding), whereas the Windows profile only calls HTMLParagraphElement_Binding::_addProperty. I don't know why that would be the case, but it explains why we might not be seeing growSlots being called here: if HTMLParagraphElement_Binding already has an unused slot, there's no need to allocate. (Glancing at the code, I assume that this is all being done for subnode.__data__ = ..., so we're presumably only adding one property to any given object.)

So the interesting question is why we're seeing SVG stuff on Android, but HTMLParagraph on Windows.

Comment 4

[Markus Stange [:mstange]]

(In reply to Iain Ireland [:iain] from comment #3)

whereas the Windows profile only calls HTMLParagraphElement_Binding::_addProperty.

I'm not sure if we know this for sure - the compiler may have inline-code-folded multiple _addProperty methods. Actually I think that's pretty likely, because I don't think the test would be using paragraph elements in the chart.

Comment 5

[Markus Stange [:mstange]]

I see JSCLASS_HAS_RESERVED_SLOTS(DOM_INTERFACE_PROTO_SLOTS_BASE) in the bindings for these SVG elements: path, rect, circle, text, title
And DOM_INTERFACE_PROTO_SLOTS_BASE is zero.
So for those we would expect to see growSlots on Windows as well, I think.

Comment 6

[Iain Ireland [:iain]]

(In reply to Markus Stange [:mstange] from comment #4)

I'm not sure if we know this for sure - the compiler may have inline-code-folded multiple _addProperty methods. Actually I think that's pretty likely, because I don't think the test would be using paragraph elements in the chart.

That's a good point. It does make me wonder why we haven't been able to deduplicate identical functions in the same way for the Android build.

(In reply to Markus Stange [:mstange] from comment #5)

I see JSCLASS_HAS_RESERVED_SLOTS(DOM_INTERFACE_PROTO_SLOTS_BASE) in the bindings for these SVG elements: path, rect, circle, text, title
And DOM_INTERFACE_PROTO_SLOTS_BASE is zero.
So for those we would expect to see growSlots on Windows as well, I think.

I believe you're looking at the class definition for the prototype, not for the SVG wrapper objects themselves. I see 1 reserved slot for every SVG instance class. I think this searchfox query should find most of the DOM classes with a different number of reserved slots.

I would expect classes with one reserved slot to get AllocKind::OBJECT2, meaning that they would have one unused slot available. Adding two custom properties would require us to grow the slots.

It might be interesting to run this test with a patch that dumps obj->getClass()->name here to give us a sense of what kinds of objects we're actually seeing, and in what quantities.

Comment 7

[Iain Ireland [:iain]]

I took a quick look at this in a local build. I added a patch to dump a message in js::NativeObject::setShapeAndAddNewSlot indicating the class, whether we were storing into a fixed slot or a dynamic slot, and (in the case of dynamic slots), whether we had to reallocate the slots array:

      2 dynamic: WebExtensionPolicy grow
      4 fixed: WebExtensionPolicy
     20 dynamic: SVGSVGElement grow
     20 fixed: SVGSVGElement
   1078 fixed: SVGLineElement
   2761 fixed: SVGPathElement
   3961 fixed: SVGTextElement
   6060 fixed: SVGCircleElement
  13246 fixed: SVGRectElement
  19310 fixed: SVGTitleElement

We see tens of thousands of SVG elements adding a new fixed slot, which does not require reallocation. Across a run of charts-observable-plot, we only allocate slots 22 times.

After taking into account Markus's point about code deduplication, this information is consistent with the Windows profile analyzed above: we call the hook a lot, but we don't see any samples reallocating slots. But that's not what shows up in Denis's Android profile. Something weird is going on there.

Denis, can you take a look at what Android is doing here? It doesn't make sense for there to be samples in growSlots.

For reference, my hacky patch:

--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -1476,16 +1476,32 @@ bool js::AddSlotAndCallAddPropHook(JSCon
                                    HandleValue v, Handle<Shape*> newShape) {
   MOZ_ASSERT(obj->getClass()->getAddProperty());
   MOZ_ASSERT(newShape->asShared().lastProperty().isDataProperty());
 
   RootedId id(cx, newShape->asShared().lastProperty().key());
   MOZ_ASSERT(!id.isInt());
 
   uint32_t slot = newShape->asShared().lastProperty().slot();
+
+  char buf[100];
+  sprintf(buf, "/tmp/addprop-%d", getpid());
+  FILE* f = fopen(buf, "a");
+
+  uint32_t numFixed = newShape->asShared().numFixedSlots();
+  if (slot < numFixed) {
+    fprintf(f, "%s: fixed\n", obj->getClass()->name);
+  } else {
+    uint32_t dynamicSlotIndex = slot - numFixed;
+    fprintf(f, "%s: dynamic (%s)\n",
+            obj->getClass()->name,
+            dynamicSlotIndex >= obj->numDynamicSlots() ? "grow" : "fits");
+  }
+  fclose(f);
+
   if (!obj->setShapeAndAddNewSlot(cx, &newShape->asShared(), slot)) {
     return false;
   }
   obj->initSlot(slot, v);
 
   return CallAddPropertyHook(cx, obj, id, v);
 }

(Clearing js-perf-next for now because this is still just an interesting observation, not actionable.)

Comment 8

[Denis Palmeiro [:denispal]]

Results on Android:

     31 JSWindowActorChild: dynamic (fits)
     22 JSWindowActorChild: dynamic (grow)
      3 JSWindowActorChild: fixed
   6060 SVGCircleElement: fixed
   1077 SVGLineElement: fixed
   2760 SVGPathElement: fixed
  13244 SVGRectElement: fixed
     19 SVGSVGElement: dynamic (grow)
     19 SVGSVGElement: fixed
   3960 SVGTextElement: fixed
  19309 SVGTitleElement: fixed

Comment 9

[Denis Palmeiro [:denispal]]

The original profile has a lot of time spent in malloc lock contention. Here is an updated profile with the patch from bug 1913757. This reduces the difference down to 6x where we are spending most of our time in mozilla::dom::PreserveWrapper.

Comment 10

[Iain Ireland [:iain]]

I think it's safe to dupe this to bug 1844878, although since it does look like we're mostly only adding one property to a given wrapper, the patch currently attached to that bug won't solve the problem on its own.