856627 - Baseline: Handle ListBase proxies in baseline ICs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Slowing down DOM-access related things.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Add ListBase proxy stubs

This adds stubs for GetProps on ListBase proxies that call JSNative getters.

That's really the only case that was hitting in dromaeo, so it doesn't seem worthwhile to spend immediate effort handling slot reads and scalling scripted getters.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I would expect there to be two relevant cases:

1)  The ".length" case: an accesor property on the prototype which has a JSNative getter/setter pair.

2)  The ".item" case: a value property on the prototype whose value is a Function object (typically used via CALLPROP).

I wouldn't be surprised if right now dromaeo only hits #1, but once we land HTMLDocument it'll be hitting #2 as well (though those parts of dromaeo may be ion-compiled, of course).

Comment 3

[Kannan Vijayan [:djvj]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> I would expect there to be two relevant cases:
> 
> 1)  The ".length" case: an accesor property on the prototype which has a
> JSNative getter/setter pair.
> 
> 2)  The ".item" case: a value property on the prototype whose value is a
> Function object (typically used via CALLPROP).
> 
> I wouldn't be surprised if right now dromaeo only hits #1, but once we land
> HTMLDocument it'll be hitting #2 as well (though those parts of dromaeo may
> be ion-compiled, of course).

I instrumented this with prints when i was in the middle of working on it, and I didn't see any hits for read properties when running through the DOM Core tests.

I was surprised too as I was expecting to see them.  Maybe my print instrumentation was wrong..

Comment 4

[Boris Zbarsky [:bzbarsky]]

Just to check, does this testcase hit your prints?

  <script>
    var l = document.getElementsByTagName("*");
    for (var i = 0; i < 100000; ++i)
      l.item(0);
  </script>

Comment 5

[Kannan Vijayan [:djvj]]

I'll test this out after the arguments object patch is done.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 735562 [details] [diff] [review]
Add ListBase proxy stubs

Review of attachment 735562 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! r=me with comments addressed.

(I still wish we could create some sort of ListBase proxies in the shell, but that's not for this bug to fix, of course.)

::: js/src/ion/BaselineIC.cpp
@@ +3066,5 @@
> +    // the shape matches.
> +    masm.branchTestObject(Assembler::NotEqual, tempVal, &failListBaseCheck);
> +    Register objReg = masm.extractObject(tempVal, tempVal.scratchReg());
> +    masm.branchPtr(Assembler::Equal, Address(objReg, JSObject::offsetOfShape()), scratch,
> +                   &listBaseOk);

Nit: masm.branchTestObjShape(Assembler::Equal, objReg, scratch, &listBaseOk);

@@ +5469,5 @@
> +    }
> +
> +    // Shape guard.
> +    masm.loadPtr(Address(BaselineStubReg, ICGetProp_CallListBaseNative::offsetOfShape()), scratch);
> +    masm.branchTestObjShape(Assembler::NotEqual, objReg, scratch, &failure);

This should be moved before the GenerateListBaseChecks call. The code generated by GenerateListBaseChecks assumes things about the object class/layout/slots and the generated code could crash if it sees an entirely different object.

@@ +5490,5 @@
> +    // Push args for vm call.
> +    masm.push(objReg);
> +    masm.push(callee);
> +
> +    // Don't to preserve R0 anymore.

Nit: missing "have"

::: js/src/ion/BaselineIC.h
@@ +4217,5 @@
> +
> +    // PC offset of call
> +    uint32_t pcOffset_;
> +
> +    ICGetProp_CallListBaseNative(IonCode *stubCode, ICStub *firstMonitorStub,

I think we have to move this constructor to BaselineIC.cpp, to avoid annoying "used but never defined" warnings for the HeapPtr* members with GCC 4.7. Please make sure it doesn't introduce new warnings, see also bug 859446.

Comment 7

[Kannan Vijayan [:djvj]]

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/62e3246ae340

Note:
I put the constructor in the main header file since when I pushed, the patch for bug 859446 had been backed out, so the constructors for the other ICStubs were back in the header file.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62e3246ae340