1323099 - Ion GetProperty IC should be used when the input is a Value

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Jan de Mooij [:jandem]]

Ion ICs are currently only used when the input is an object. This covers the majority of cases, but we have seen benchmarks (Octane-crypto is one of them, IIRC) that do something like x.length/x.toString()/x[i] where x is {string, object}.

Once bug 1322093 is fixed, it will be easy to fix this: we can upgrade the object-Register in IonGetPropertyIC to a TypedOrValueRegister and make the C++ fallback function take a Value, and then we should automatically emit the right guards and regalloc stuff.

Filing this now so I don't forget about it.

Comment 1

[Nicolas B. Pierron [:nbp]]

If I recall correctly, this case appears in react code as well. (see Bug 1301690)

Comment 2

[Jan de Mooij [:jandem]]

I had some WIP patches for this that worked fine. I'll try to get them landed this cycle.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 1 - GetProp

This patch changes the GetPropertyIC to take a TypedOrValueRegister as input, so now it can be used for any JSOP_GETPROP. We no longer need the shared stub or MCallGetProperty (there are some other places that use this so we can't remove it).

Later we can do something similar for JSOP_GETELEM.

Comment 4

[Tom S [:evilpie]]

For register usage wouldn't it be better to have an object and a value version of GetPropertyCache?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #4)
> For register usage wouldn't it be better to have an object and a value
> version of GetPropertyCache?

We use a TypedOrValueRegister, so it's either an object register (as before) or a boxed Value.

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8829782 [details] [diff] [review]
Part 1 - GetProp

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

Nice!

::: js/src/jit/Lowering.cpp
@@ +3656,5 @@
>          defineBox(lir, ins);
>          assignSafepoint(lir, ins);
>      } else {
>          LGetPropertyCacheT* lir =
> +            new(alloc()) LGetPropertyCacheT(useBoxOrTypedOrConstant(value, /* useConst = */ false),

Shouldn't we introduce "useBoxOrTyped" for this?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb0de976a39e
part 1 - Use Ion GetPropertyIC also when the input is a boxed Value. r=h4writer

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #6)
> Shouldn't we introduce "useBoxOrTyped" for this?

Good idea. I added useBoxOrTyped and changed useBoxOrTypedOrConstant to call that in the not-a-constant case.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Jan de Mooij [:jandem]]

There's more to do here (for JSOP_GETELEM) but let's close this one to make it easier to track this.