1329195 - CacheIR: Update old stubs instead of attaching a new stub when a DOM proxy's generation changes

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

To avoid attaching a lot of stubs and then deoptimizing.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch gives the generation stub field its own StubField::Type. Then in stubDataEquals (renamed to stubDataEqualsMaybeUpdate), it's easy to check whether the generation is the *only* thing that chanced and if so we update the old stub data instead of attaching a new stub.

For this to work, I had to change the Ion codegen to read the generation from the IonICStub instead of baking it in in the code.

I'll now attach a microbenchmark and post perf numbers.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> whether the generation is the *only* thing that chanced

Ugh, *changed*, obviously.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Microbenchmark

Here's a simple microbenchmark for this. It has a <form> element and adds input elements to that so the generation changes.

Results (best of 5 runs):

Before:             161 ms
Before, no Ion:     168 ms

After:               10 ms
After, no Ion:       24 ms

Note that we're also doing much better than Aurora:

Aurora 52:           34 ms
Aurora 52, no Ion:  387 ms

This is because the old Baseline code only optimized the native-getter unshadowed case, and now we also handle plain data properties on the proto. Also, the old Ion IC code had to reset the stub chain and compile a new stub every time we attached an "unshadowed" stub and now we can just update the generation field. This avoids a lot of W^X toggling.

Comment 4

[Tom S. (please needinfo tschuster)]

Comment on attachment 8827394 [details] [diff] [review]
Patch

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

So without this we actually regressed performance quite a bit, correct? I think I remember a few other cases where we used to just update the stubs and these numbers have me a bit concerned.

::: js/src/jit/CacheIRCompiler.cpp
@@ +858,5 @@
>              continue;
>          }
>  
> +        if (field.asInt64() != *reinterpret_cast<const uint64_t*>(stubDataWords)) {
> +            if (field.type() != StubField::Type::DOMExpandoGeneration)

Using a custom type for this is clever!

Comment 5

[Tom S. (please needinfo tschuster)]

But otherwise the new numbers look very good +1

Comment 6

[Jan de Mooij [:jandem]]

Thanks for the review!

(In reply to Tom Schuster [:evilpie] from comment #4)
> I think I remember a few other cases where we used to just update the stubs
> and these numbers have me a bit concerned.

That was for shape guards on the prototype in some cases. It was added for the BaselineInspector, but the inspector now checks whether the shape still matches the holder's current shape. The Ion ICs also never did anything like that. We should investigate these things at some point, but I don't think it's very urgent.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It may be worth checking the bugs that motivated the addition of that "update the stub" stuff to start with and checking how we do on those testcases.

Comment 8

[Jan de Mooij [:jandem]]

Pulsebot isn't working, so:

https://hg.mozilla.org/integration/mozilla-inbound/rev/80e4fe7ff7cb78b5774caa19f9c340132a06202b

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/80e4fe7ff7cb