1368509 - Generalize Baseline type update stubs

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Micro-benchmark

Bug 1363054 did this for the type monitor stubs and we might as well do something similar for the type update stubs while we're at it.

Attaching a simple micro-benchmark that improves from 65 ms to 20 ms with --no-ion with the patches I'll attach. It's a stupid benchmark but this kind of thing shows up in polymorphic code - when I load Gmail.com the number of DoTypeUpdateFallback calls improves from more than 23k to less than 8k.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Some DoTypeUpdateFallback cleanup

This cleans up DoTypeUpdateFallback a bit. In particular, all type update stubs have kind CacheIR_Updated now, so we can remove the switch statement and simplify the code a little.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Handle unknown/unknownObject type sets better

This is very similar to the type monitor patches: I added TypeUpdate_AnyValue and the primitive set stub can now be used for the any-object case.

Comment 3

[Ted Campbell [:tcampbell]]

Comment on attachment 8872391 [details] [diff] [review]
Part 1 - Some DoTypeUpdateFallback cleanup

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

Good cleanup.

Comment 4

[Ted Campbell [:tcampbell]]

Comment on attachment 8872393 [details] [diff] [review]
Part 2 - Handle unknown/unknownObject type sets better

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

::: js/src/jit/BaselineIC.cpp
@@ +442,5 @@
>  
> +bool
> +ICTypeUpdate_AnyValue::Compiler::generateStubCode(MacroAssembler& masm)
> +{
> +    masm.mov(ImmWord(1), R1.scratchReg());

I'd prefer a simple comment stating the obvious that AnyValue always matches so return true.

::: js/src/jit/SharedIC.cpp
@@ +431,5 @@
> +void
> +ICUpdatedStub::resetUpdateStubChain(Zone* zone)
> +{
> +    while (!firstUpdateStub_->isTypeUpdate_Fallback()) {
> +        if (zone->needsIncrementalBarrier()) {

Good point. I forget these sort of things still need barriers.

@@ +2559,5 @@
> +        types = obj->group()->maybeGetProperty(id);
> +        MOZ_ASSERT(types);
> +    }
> +
> +    // Don't attach too many SingleObject/ObjectGroup stubs. If the value is a

This comment + if statement took me a few reads to understand. Perhaps rephrase.

// Don't attach too many SingleObject/ObjectGroup stubs unless we can replace them with a single PrimitiveSet or AnyValue stub.

@@ +2621,5 @@
>      } else if (val.toObject().isSingleton()) {
>          RootedObject obj(cx, &val.toObject());
>  
> +#ifdef DEBUG
> +        // We should not have a stub for this object.

Good find. This code seems to be legacy from Bug 836064 and no longer appropriate.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0946744f27
part 1 - Clean up DoTypeUpdateFallback a bit. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5cee986655
part 2 - Generalize Baseline unknown/unknownObject type update stubs. r=tcampbell

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d0946744f27
https://hg.mozilla.org/mozilla-central/rev/ba5cee986655