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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Tom S [:evilpie]]

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

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?

Comment 1

[Tom S [:evilpie]]

Oh wow, this needs to be BooleanValue(true) of course!

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Tom S [:evilpie]]

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?

Comment 4

[Jan de Mooij [:jandem]]

(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 5

[Tom S [:evilpie]]

Attachment: v2 - Constant fold in/hasOwn with a definite slot or known unboxed offset

Oh yeah, that's quite obvious. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca1506f74878c357550b90cdd2e52c2524a38ca2&selectedJob=134891137

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1a430da8fe8
Constant fold in/hasOwn with a definite slot or known unboxed offset. r=jandem

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/d1a430da8fe8