Add a megamorphic SetElem stub

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
Created attachment 8894907 [details] [diff] [review]
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();
Attachment #8894907 - Flags: review?(evilpies)
(Assignee)

Comment 1

10 months ago
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 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?
Attachment #8894907 - Flags: review?(evilpies) → review+

Comment 3

9 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d85d37d083c
Add a megamorphic SetElement stub. r=evilpie
(Assignee)

Comment 4

9 months ago
(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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d85d37d083c
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.