Closed
Bug 856627
Opened 12 years ago
Closed 12 years ago
Baseline: Handle ListBase proxies in baseline ICs
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: djvj, Unassigned)
References
Details
Attachments
(1 file)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Slowing down DOM-access related things.
Reporter | ||
Comment 1•12 years ago
|
||
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)
Comment 2•12 years ago
|
||
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).
Reporter | ||
Comment 3•12 years ago
|
||
(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•12 years ago
|
||
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>
Reporter | ||
Comment 5•12 years ago
|
||
I'll test this out after the arguments object patch is done.
Comment 6•12 years ago
|
||
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+
Reporter | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•