Closed
Bug 1368509
Opened 7 years ago
Closed 7 years ago
Generalize Baseline type update stubs
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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•7 years ago
|
Attachment #8872391 -
Flags: review?(tcampbell)
Assignee | ||
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
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 4•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8872390 -
Attachment mime type: application/x-javascript → text/plain
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7d0946744f27 https://hg.mozilla.org/mozilla-central/rev/ba5cee986655
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•2 years ago
|
Performance Impact: --- → ?
Whiteboard: [qf]
You need to log in
before you can comment on or make changes to this bug.
Description
•