Closed Bug 1404659 Opened 2 years ago Closed 2 years ago

Constant fold in/hasOwn with a definite slot or known unboxed offset

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(1 file, 1 obsolete file)

getProp/setProp have a _lot_ of different optimization strategies. At least getPropTryDefiniteSlot and getPropTryUnboxed are easy to port. Both of these AFAIK only look at own properties of the object, so this is applicable to hasOwn and in. Maybe it would make sense to write custom code for this, to avoid some of the unnecessary work, but that seems minimal to me.

This was mostly motivated by the self-hosted Object/Reflect defineProperty code, which uses |in| quite a bit and for simple, but probably common uses like Object.defineProperty(x, y, {value: 12, writable: false}), we would already fold away most stuff, but not the |"value" in ...| branch. During testing I noticed that we don't inline ObjectOrReflectDefineProperty into ObjectDefineProperty?
Attachment #8914017 - Flags: feedback?(jdemooij)
Oh wow, this needs to be BooleanValue(true) of course!
Comment on attachment 8914017 [details] [diff] [review]
Constant fold in/hasOwn with a definite slot or known unboxed offset

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

Good idea!

::: js/src/jit/IonBuilder.cpp
@@ +12841,5 @@
> +        return Ok();
> +
> +    JSAtom* atom = &str->asAtom();
> +    uint32_t dummy;
> +    if (atom->isIndex(&dummy))

It might be nice to change getDefiniteSlot and getUnboxedOffset to take a jsid argument instead of PropertyName*. getDefiniteSlot does NameToId(name) so there should be no change in behavior.
Attachment #8914017 - Flags: feedback?(jdemooij) → feedback+
After removing all checks and using jsid directly I run into various failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc74f2ce5e26652876400b603bc925be239d3266

I think this mostly comes from int-jsids. So maybe we should check for those. Do we still need to check of isIndex, too?
(In reply to Tom Schuster [:evilpie] from comment #3)
> I think this mostly comes from int-jsids. So maybe we should check for
> those. Do we still need to check of isIndex, too?

I think you can do something like this: http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/js/src/jit/IonBuilder.cpp#12801-12802 ?
Comment on attachment 8915109 [details] [diff] [review]
v2 - Constant fold in/hasOwn with a definite slot or known unboxed offset

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

Excellent.
Attachment #8915109 - Flags: review?(jdemooij) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1a430da8fe8
Constant fold in/hasOwn with a definite slot or known unboxed offset. r=jandem
https://hg.mozilla.org/mozilla-central/rev/d1a430da8fe8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.