Closed
Bug 918593
Opened 11 years ago
Closed 11 years ago
IonMonkey: NameIC chokes on prototypically inherited slotful scope properties
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: efaust, Assigned: efaust)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
(deleted),
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
There's a performance cliff if the name you want was on the prototype chain of one of the scope objects instead of the object itself. This is inexcusable.
This is neatly demonstrated by:
<script>
var count = 100000;
var start = new Date;
window.__proto__.foo = 5;
for (var i = 0; i < count; i++)
foo;
var end = new Date;
document.write((end - start) / count * 1e6);
</script>
This takes 600ns per iteration, per bz's reports, while setting foo as |window.foo = 5;| takes 10ns per iteration.
The name ICs just aren't capable of walking proto chains on scope objects.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #808059 -
Flags: review?(kvijayan)
Assignee | ||
Comment 2•11 years ago
|
||
Patch looks like it brings us down to about 30ns per iteration on simple microbench:
<script>
var count = 100000;
var start = new Date;
window.__proto__.foo = 5;
for (var i = 0; i < count; i++)
foo;
var end = new Date;
document.write((end - start) / count * 1e6);
</script>
Where bz reports changing it to window.foo = 5 brings us to similar speeds.
Attachment #808060 -
Flags: review?(kvijayan)
Updated•11 years ago
|
Attachment #808059 -
Flags: review?(kvijayan) → review+
Comment 3•11 years ago
|
||
Comment on attachment 808060 [details] [diff] [review]
Allow protypical slotful name lookups on globals
Review of attachment 808060 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/IonCaches.cpp
@@ +3659,5 @@
> }
>
> static void
> GenerateScopeChainGuards(MacroAssembler &masm, JSObject *scopeChain, JSObject *holder,
> + Register outputReg, Label *failures, bool guardLast = true)
Minor style nit: It's generally preferrable to have default arguments default to "false", mainly to promote the notion that "if you don't pass an argument, it's like passing false for it".
Instead of |guardLast=true|, maybe |skipLastGuard=false| as the parameter (and corresponding changes in other parts of the code) would fit with that philosophy.
@@ +3675,2 @@
> GenerateScopeChainGuard(masm, tobj, outputReg, NULL, failures);
> +
Nit: whitespace on blank line above.
Attachment #808060 -
Flags: review?(kvijayan) → review+
Assignee | ||
Comment 4•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3cf3c64cfd28
https://hg.mozilla.org/mozilla-central/rev/abb4ec445967
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•