918593 - IonMonkey: NameIC chokes on prototypically inherited slotful scope properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

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.

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 0: Remove unused name parameter to GenerateReadSlot()

Comment 2

[Eric Faust [:efaust]]

Attachment: Allow protypical slotful name lookups on globals

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.

Comment 3

[Kannan Vijayan [:djvj]]

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.

Comment 4

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf3c64cfd28
https://hg.mozilla.org/integration/mozilla-inbound/rev/abb4ec445967

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3cf3c64cfd28
https://hg.mozilla.org/mozilla-central/rev/abb4ec445967