Closed Bug 856627 Opened 7 years ago Closed 7 years ago

Baseline: Handle ListBase proxies in baseline ICs

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: djvj, Unassigned)

References

Details

Attachments

(1 file)

Slowing down DOM-access related things.
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.
Attachment #735562 - Flags: review?(jdemooij)
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).
(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..
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>
I'll test this out after the arguments object patch is done.
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.
Attachment #735562 - Flags: review?(jdemooij) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/62e3246ae340
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.