1388388 - Add a megamorphic SetElem stub

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Megamorphic SETELEMs are pretty common on Speedometer but hard to optimize with IC stubs, especially when adding new properties. However the least we could do is attach a stub that just calls js::SetObjectElement.

The attached patch does this and it improves the micro-benchmark below from 485-500 to 430-450 ms.

function f() {
    var o = {};
    var props = [];
    for (var i = 0; i < 50; i++) {
        props.push("foo" + i);
    }
    var t = new Date;
    for (var i = 0; i < 100000; i++) {
        for (var j = 0; j < 50; j++) {
            o[props[j]] = j;
        }
    }
    print(new Date - t);
}
f();

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 8894907 [details] [diff] [review]
Patch

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

::: js/src/jit/VMFunctions.h
@@ +892,5 @@
>  bool
>  GetPrototypeOf(JSContext* cx, HandleObject target, MutableHandleValue rval);
>  
> +typedef bool (*SetObjectElementFn)(JSContext*, HandleObject, HandleValue,
> +                                   HandleValue, HandleValue, bool);

I should move this to VMFunctions.cpp

(The reason for having this in VMFunctions.h/cpp is to avoid generating 4 copies of the same VM wrapper for this C++ function. I think at some point we should do this for more VM functions.)

Comment 2

[Tom S [:evilpie]]

Comment on attachment 8894907 [details] [diff] [review]
Patch

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

::: js/src/jit/CacheIR.cpp
@@ +3588,5 @@
> +
> +    // The generic proxy stubs are faster.
> +    if (obj->is<ProxyObject>())
> +        return false;
> +

Should we add something like writer.guardIsNotProxy here?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d85d37d083c
Add a megamorphic SetElement stub. r=evilpie

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #2)
> Should we add something like writer.guardIsNotProxy here?

Good question. I decided not to add it because if a SETELEM is megamorphic and both proxies and non-proxies show up, it's probably okay for all objects to go through SetObjectElement.

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/4d85d37d083c