Closed
Bug 1479603
Opened 3 years ago
Closed 3 years ago
Inline Caching Cleanup
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: mgaudet, Assigned: mgaudet)
References
Details
Attachments
(12 files, 1 obsolete file)
29.12 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
10.77 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
2.88 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
18.09 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
7.26 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
46.49 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
29.12 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
8.45 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
61.08 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
108.40 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
71.02 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
7.59 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
As more pieces of the IC system get converted to CacheIR, there's a number opportunity to generally cleanup the Inline Caching story: * There's a good number of functions in the wrong files (ie, things in SharedIC.h that are only ever referred to in CacheIRCompiler files). * Once Bugs 1478126 and 1341261 land some IC Machinery (ICMultiStubCompiler) goes dead. There's also an opportunity to split apart SharedIC.h into some files with some more structure. I was thinking something like: ICCommon.{h,cpp}: Basic IC definitions; ICEntry, ICStub{Const}Iterator, ICStub, ICFallback Stub TypeIC.{h,cpp}: The type IC mechanisms; TypeCheckPrimitiveSetStub and subclasses. SharedIC.{h,cpp}: Kept as homes for things using the Shared stubs machinery; ICNewObject_Fallback etc.
Assignee | ||
Updated•3 years ago
|
Assignee: nobody → mgaudet
Comment 1•3 years ago
|
||
Eventually the shared stubs machinery should just be removed. Once it's only used for ICNew{Object,Array} that's probably not enough justification to keep all that code. IIRC these New* stubs were ported to shared stubs "because we can" and I'm not sure having them in Ion is really necessary for perf; I might be wrong on this, worth checking the original bugs that made them shared stubs. So then everything in {Baseline,Shared}IC.{h,cpp} will be Baseline specific and I like using BaselineIC.{h,cpp} for them (also for consistency with IonIC.{h,cpp}). We could still split the Type Monitor/Update stubs out to BaselineTypeIC.{h,cpp} or something. WDYT?
Assignee | ||
Comment 2•3 years ago
|
||
I definitely like the idea of keeping baseline-with-baseline in files named Baseline. I do still a common base file may make sense (though, I haven't done the work to tease it out yet, so could be more tightly woven than I'd like) I definitely would love to get rid of the last vestiges of the SharedIC stuff, but an experimental try push [1] with just the NewArray and NewObject Ion ICs disabled suggest that on some workloads they're still providing value ("tp6_youtube opt 1_thread e10s stylo" shows a pretty big degradation; though, "tp6_youtube opt e10s stylo" does not -- curious). If we don't get rid of it, I'd still say segregating them into SharedIC.{cpp,h} makes sense (at least to draw a target on their back for CacheIR conversion (planning on investigating that later today anyhow). [1]: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=ddf5d6bd6e6c5f9dfe638e0e413bcad458f854f0&framework=1&selectedTimeRange=172800
Comment 3•3 years ago
|
||
(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #2) > I do still a common base file may make sense (though, I haven't done the > work to tease it out yet, so could be more tightly woven than I'd like) But common in what sense? If Baseline and Ion, their IC code is basically unshared except for the CacheIR code. If BaselineIC and SharedIC, I think that's not worth the trouble if SharedIC is dying soon anyway.
Updated•3 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•3 years ago
|
||
- Move SimpleTypeDescrKey functions into CacheIR.h. This matches the set of callers - Move LoadTypedThingData into CacheIRCompiler, matching usage - Push definition of StripPreliminaryObjectStubs up in file, and remove header declaration - Consolidate LoadTypedThingLength into CacheIR files - Remove now dead ICMultiStubCompiler - Remove BinarySharedStub classes. Can do this now that Compare and BinaryArith ICs have been converted to CacheIR - Rename comapreSharedStub, and use correct flag to disable it. - Remove SharedStubInfo - Collapse SharedICList.h back into SharedIC.h - Remove unimplemented declaration (This used to be a bunch of patches, but I folded them for review ease; can unfold if desired)
Attachment #9001475 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•3 years ago
|
||
Attachment #9001476 -
Flags: review?(jdemooij)
Assignee | ||
Comment 6•3 years ago
|
||
Attachment #9001477 -
Flags: review?(jdemooij)
Assignee | ||
Comment 7•3 years ago
|
||
PS: I haven't written this patch yet, but what do you think about renaming the last of the 'shared stubs' machinery to 'fallback stub' machinery. (I am thinking specifically of [1] and [2]) [1]: https://searchfox.org/mozilla-central/rev/5dbfd833bbb114afe758db4d4bdbc5b13bcc33ef/js/src/jit/CodeGenerator.cpp#10393 [2]: https://searchfox.org/mozilla-central/rev/5dbfd833bbb114afe758db4d4bdbc5b13bcc33ef/js/src/jit/CodeGenerator.h#293-303
Comment 8•3 years ago
|
||
Comment on attachment 9001475 [details] [diff] [review] [Part 1] Cleanup IC code as we show SharedICs the door Review of attachment 9001475 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineIC.cpp @@ +547,5 @@ > > return tailCallVM(DoToNumberFallbackInfo, masm); > } > > +void Nice, we can now use "static void" here because it's only used in BaselineIC.cpp right? ::: js/src/jit/CacheIRCompiler.h @@ +905,5 @@ > +void > +LoadTypedThingData(MacroAssembler& masm, TypedThingLayout layout, Register obj, Register result); > + > +void > +LoadTypedThingLength(MacroAssembler& masm, TypedThingLayout layout, Register obj, Register result); Maybe these two could now be protected methods (loadTypedThingData, loadTypedThingLength) of the CacheIRCompiler class. Follow-up patch? :) ::: js/src/jit/SharedIC.h @@ +193,5 @@ > > class ICStub; > class ICFallbackStub; > > +// List of IC stub kinds that can run in Baseline and in IonMonkey This list can now be merged with BaselineICList right?
Attachment #9001475 -
Flags: review?(jdemooij) → review+
Comment 9•3 years ago
|
||
Comment on attachment 9001476 [details] [diff] [review] [Part 2] Cleanup Ion SharedStubs code Review of attachment 9001476 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Attachment #9001476 -
Flags: review?(jdemooij) → review+
Comment 10•3 years ago
|
||
Comment on attachment 9001477 [details] [diff] [review] [Part 3] Remove Shared Stubs option Review of attachment 9001477 [details] [diff] [review]: ----------------------------------------------------------------- We can also remove this option from this test file: jit-test/tests/basic/write-frozen-dense-strict-inlinecache.js
Attachment #9001477 -
Flags: review?(jdemooij) → review+
Comment 11•3 years ago
|
||
(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #7) > PS: I haven't written this patch yet, but what do you think about renaming > the last of the 'shared stubs' machinery to 'fallback stub' machinery. (I am > thinking specifically of [1] and [2]) I think on top of your other patches, this is all dead code :) We can also remove ICStubEngine::IonSharedIC then (and probably remove ICStubEngine from a lot of places where it's used).
Assignee | ||
Comment 12•3 years ago
|
||
Assignee | ||
Comment 13•3 years ago
|
||
Comment on attachment 9001723 [details] [diff] [review] [Part 4] Remove SharedIC support from Ion You're 100% correct. :D
Attachment #9001723 -
Flags: review?(jdemooij)
Comment 14•3 years ago
|
||
Comment on attachment 9001723 [details] [diff] [review] [Part 4] Remove SharedIC support from Ion Review of attachment 9001723 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ -10561,5 @@ > - // Patch shared stub IC loads using IC entries > - for (size_t i = 0; i < sharedStubs_.length(); i++) { > - CodeOffset label = sharedStubs_[i].label; > - > - IonICEntry& entry = ionScript->sharedStubList()[i]; IonICEntry can also be removed now and we can then merge ICEntry and BaselineICEntry \o/ ::: js/src/jit/IonCode.h @@ +545,5 @@ > recompiling_ = false; > } > > FallbackICStubSpace* fallbackStubSpace() { > return &fallbackStubSpace_; All this can also go away.
Attachment #9001723 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 16•3 years ago
|
||
Once the old shared stub kind list is folded back into the baseline list where appropriate, that leaves behind the CacheIR stub kinds, which don't feel like they belong. So I was thinking I'd keep them separate, but name them as such.
Attachment #9002453 -
Flags: review?(jdemooij)
Assignee | ||
Comment 17•3 years ago
|
||
Attachment #9002512 -
Flags: review?(jdemooij)
Assignee | ||
Comment 18•3 years ago
|
||
Attachment #9002513 -
Flags: review?(jdemooij)
Assignee | ||
Comment 19•3 years ago
|
||
Attachment #9002514 -
Flags: review?(jdemooij)
Assignee | ||
Comment 20•3 years ago
|
||
Attachment #9002515 -
Flags: review?(jdemooij)
Comment 21•3 years ago
|
||
Comment on attachment 9002453 [details] [diff] [review] [Part 1 Fixup] Rename SHARED_STUB_KIND_LIST to CACHEIR_STUB_KIND_LIST Review of attachment 9002453 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/SharedIC.h @@ +193,5 @@ > > class ICStub; > class ICFallbackStub; > > +// Cache IR stubs can be generated for both Baseline and IonMonkey CacheIR works for both Baseline and Ion ICs, but this is the Baseline specific part and these stub kinds are only used for Baseline. I think we should just add them to the Baseline list.
Attachment #9002453 -
Flags: review?(jdemooij)
Updated•3 years ago
|
Attachment #9002512 -
Flags: review?(jdemooij) → review+
Comment 22•3 years ago
|
||
Comment on attachment 9002513 [details] [diff] [review] [Part 6] Reduce redundant usage of ICStubEngine Review of attachment 9002513 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineIC.h @@ +542,5 @@ > } > > public: > + explicit Compiler(JSContext* cx, bool hasReceiver = false) > + : ICStubCompiler(cx, ICStub::GetProp_Fallback, Engine::Baseline), We can also remove the engine field from ICStubCompiler :)
Attachment #9002513 -
Flags: review?(jdemooij) → review+
Comment 23•3 years ago
|
||
Comment on attachment 9002514 [details] [diff] [review] [Part 7] Collapse IonICEntry and BaselineICEntry into ICEntry Review of attachment 9002514 [details] [diff] [review]: ----------------------------------------------------------------- Excellent.
Attachment #9002514 -
Flags: review?(jdemooij) → review+
Comment 24•3 years ago
|
||
Comment on attachment 9002515 [details] [diff] [review] [Part 8] Remove Ion Fallback stub space Review of attachment 9002515 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/SharedIC.h @@ +1109,5 @@ > > public: > virtual ICStub* getStub(ICStubSpace* space) = 0; > > static ICStubSpace* StubSpaceForStub(bool makesGCCalls, JSScript* outerScript, Engine engine) { We can remove the engine argument here too :)
Attachment #9002515 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 25•3 years ago
|
||
Attachment #9002783 -
Flags: review?(jdemooij)
Assignee | ||
Comment 26•3 years ago
|
||
Attachment #9002784 -
Flags: review?(jdemooij)
Assignee | ||
Comment 27•3 years ago
|
||
(this cleanup is broader and farther reaching than I had foreseen!)
Comment 28•3 years ago
|
||
Comment on attachment 9002783 [details] [diff] [review] [Part 9] Remove ICStubCompiler::Engine Review of attachment 9002783 [details] [diff] [review]: ----------------------------------------------------------------- Nice, r=me with nits addressed. ::: js/src/jit/BaselineIC.h @@ +302,5 @@ > bool hasReceiver_; > MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override; > > virtual int32_t getKey() const override { > + return static_cast<int32_t>(kind) << 1 | We can remove the |<< 1| now and change the << 17 below to << 16 @@ +534,5 @@ > MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override; > void postGenerateStubCode(MacroAssembler& masm, Handle<JitCode*> code) override; > > virtual int32_t getKey() const override { > + return static_cast<int32_t>(kind) << 1 | And here. @@ +666,5 @@ > MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override; > void postGenerateStubCode(MacroAssembler& masm, Handle<JitCode*> code) override; > > virtual int32_t getKey() const override { > + return static_cast<int32_t>(kind) << 1 | And here. @@ +755,5 @@ > uint32_t pcOffset_; > MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override; > > virtual int32_t getKey() const override { > + return static_cast<int32_t>(kind) << 1 | Same here (also please keep the parentheses around the <<) @@ +852,5 @@ > uint32_t pcOffset_; > MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override; > > virtual int32_t getKey() const override { > + return static_cast<int32_t>(kind) << 1 | And here. @@ +932,5 @@ > uint32_t pcOffset_; > MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override; > > virtual int32_t getKey() const override { > + return static_cast<int32_t>(kind) << 1 | And here. ::: js/src/jit/SharedIC.cpp @@ -536,5 @@ > - if (engine_ == Engine::Baseline) { > - EmitBaselineTailCallVM(code, masm, argSize); > - } else { > - uint32_t stackSize = argSize + fun.extraValuesToPop * sizeof(Value); > - EmitIonTailCallVM(code, masm, stackSize); EmitIonTailCallVM, more dead code :) @@ +584,5 @@ > ICStubCompiler::leaveStubFrame(MacroAssembler& masm, bool calledIntoIon) > { > MOZ_ASSERT(entersStubFrame_ && inStubFrame_); > inStubFrame_ = false; > + Remove trailing whitespace. ::: js/src/jit/SharedIC.h @@ +1011,5 @@ > #endif > > // By default the stubcode key is just the kind. > virtual int32_t getKey() const { > + return (static_cast<int32_t>(kind) << 1); return static_cast<int32_t>(kind); @@ +1195,5 @@ > TypeCheckPrimitiveSetStub* existingStub_; > uint16_t flags_; > > virtual int32_t getKey() const override { > + return static_cast<int32_t>(kind) << 1 | Can remove the << 1 and s/17/16/
Attachment #9002783 -
Flags: review?(jdemooij) → review+
Comment 29•3 years ago
|
||
Comment on attachment 9002784 [details] [diff] [review] [Part 10] Relocate the remaining Baseline-only stub code to BaselineIC files Review of attachment 9002784 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/SharedIC.h @@ -688,4 @@ > // that these do not get purged, all stubs that can make calls are allocated > // in the fallback stub space. > bool allocatedInFallbackSpace() const { > MOZ_ASSERT(next()); I think all of this other code (ICStub etc) is also Baseline specific.
Attachment #9002784 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 30•3 years ago
|
||
Woo! You're totally right. I attempted to verify the new headers with a FILES_PER_UNIFIED_FILE = 1 local build, and this try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42e47dda9873d02aadbbb44f19c8cb549ae42761 Hopefully that'll be enough to avoid breaking non-unified builds.
Attachment #9003167 -
Flags: review?(jdemooij)
Assignee | ||
Updated•3 years ago
|
Attachment #9002453 -
Attachment is obsolete: true
Assignee | ||
Comment 31•3 years ago
|
||
Sorry for the extra patch, but I had some trouble trying to get this change into the stack with minimal fixups, so it was just much easier to toss it on top. Appreciate it :)
Attachment #9003241 -
Flags: review?(jdemooij)
Comment 32•3 years ago
|
||
Comment on attachment 9003167 [details] [diff] [review] [Part 11] Remove SharedIC.h,cpp Review of attachment 9003167 [details] [diff] [review]: ----------------------------------------------------------------- \o/ This is awesome. ::: js/src/jit/BaselineIC.h @@ +8,5 @@ > #define jit_BaselineIC_h > > #include "mozilla/Assertions.h" > > + Nit: remove the extra blank line ::: js/src/jit/CacheIR.h @@ +13,5 @@ > > #include "gc/Rooting.h" > #include "jit/CompactBuffer.h" > #include "jit/ICState.h" > +#include "jit/MacroAssembler.h" Does it compile without this #include? I don't think CacheIR.h itself depends on masm.
Attachment #9003167 -
Flags: review?(jdemooij) → review+
Comment 33•3 years ago
|
||
Comment on attachment 9003241 [details] [diff] [review] [Part 12] Simplify getKey computations Review of attachment 9003241 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #9003241 -
Flags: review?(jdemooij) → review+
Comment 34•3 years ago
|
||
Pushed by mgaudet@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7518b4bbc6c7 [Part 1] Cleanup IC code as we show SharedICs the door. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/402c68b550d1 [Part 2] Cleanup Ion SharedStubs code r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/9a64325eeacb [Part 3] Remove Shared Stubs option r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/92b4cab82784 [Part 4] Remove SharedIC support from Ion r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/68b7d42b931d [Part 5] Relocate Baseline fallback stubs to BaselineIC.h r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/4bdec5f4de88 [Part 6] Reduce redundant usage of ICStubEngine r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/652d157c6c62 [Part 7] Collapse IonICEntry and BaselineICEntry into ICEntry r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/c594e6a34377 [Part 8] Remove Ion Fallback stub space r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/b733b2488363 [Part 9] Remove ICStubCompiler::Engine r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/370a45e8ea65 [Part 10] Relocate the remaining Baseline-only stub code to BaselineIC files r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/5a2381311ced [Part 11] Remove SharedIC.h,cpp r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0c2d31df1e [Part 12] Simplify getKey computations r=jandem
Comment 35•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7518b4bbc6c7 https://hg.mozilla.org/mozilla-central/rev/402c68b550d1 https://hg.mozilla.org/mozilla-central/rev/9a64325eeacb https://hg.mozilla.org/mozilla-central/rev/92b4cab82784 https://hg.mozilla.org/mozilla-central/rev/68b7d42b931d https://hg.mozilla.org/mozilla-central/rev/4bdec5f4de88 https://hg.mozilla.org/mozilla-central/rev/652d157c6c62 https://hg.mozilla.org/mozilla-central/rev/c594e6a34377 https://hg.mozilla.org/mozilla-central/rev/b733b2488363 https://hg.mozilla.org/mozilla-central/rev/370a45e8ea65 https://hg.mozilla.org/mozilla-central/rev/5a2381311ced https://hg.mozilla.org/mozilla-central/rev/1d0c2d31df1e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•