Generalize Baseline type update stubs

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
26 days ago
4 days ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf])

Attachments

(3 attachments)

(Assignee)

Description

26 days ago
Created attachment 8872390 [details]
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.
(Assignee)

Comment 1

26 days ago
Created attachment 8872391 [details] [diff] [review]
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.
(Assignee)

Updated

26 days ago
Attachment #8872391 - Flags: review?(tcampbell)
(Assignee)

Comment 2

26 days ago
Created attachment 8872393 [details] [diff] [review]
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.
Attachment #8872393 - Flags: review?(tcampbell)
Comment on attachment 8872391 [details] [diff] [review]
Part 1 - Some DoTypeUpdateFallback cleanup

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

Good cleanup.
Attachment #8872391 - Flags: review?(tcampbell) → review+
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.
Attachment #8872393 - Flags: review?(tcampbell) → review+
Attachment #8872390 - Attachment mime type: application/x-javascript → text/plain

Comment 5

24 days ago
Pushed by jandemooij@gmail.com:
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

24 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7d0946744f27
https://hg.mozilla.org/mozilla-central/rev/ba5cee986655
Status: ASSIGNED → RESOLVED
Last Resolved: 24 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1369774
Depends on: 1373094
You need to log in before you can comment on or make changes to this bug.