Add a megamorphic SetElem stub

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
Posted patch PatchSplinter Review
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

2 years 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

2 years 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

2 years 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

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