Shrink ICEntry and ICFallbackStub
Categories
(Core :: JavaScript Engine: JIT, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox90 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(3 files)
Bug 1673553 (TI removal and follow-ups) simplified the Baseline IC code and data structures a lot but there's still more to explore:
-
We still have a C++ class for each fallback stub kind. I think we can get rid of this and have just
ICFallbackStub(the current base class) similar toICCacheIRStub. Some fallback stubs (NewArray, NewObject, Rest) store a template object, but that will likely change in the future. -
We then have the following for each fallback stub:
* ICEntry (2 words) [array in JitScript]
firstStub
pcOffset
* ICFallbackStub (4 words) [allocated in LifoAlloc]
code
enteredCount + isFallback
icEntry
state
This isn't very efficient, for example ICEntry has 4 padding bytes. It might be better to store a pointer to the fallback stub in the ICEntry and store the pcOffset in the fallback stub. This would get us down to 5 words.
Another option is to merge ICEntry and ICFallbackStub like this:
* ICFallbackStub (4 words) [array in JitScript]
code
enteredCount + isFallback
firstStub
pcOffset + state
This would let us speed up JitScript allocation because we no longer need the LifoAlloc for fallback stubs, we just allocate the fixed-size array as part of JitScript (similar to current ICEntry[]) and fill it in.
|
Assignee | |
Comment 1•4 years ago
|
||
This should make JitScript/ICScript allocation even cheaper which is nice. It does double sizeof(ICEntry) (although ICEntry no longer exists directly) so an alternative is to keep ICEntry as separate array of firstStub pointers:
* ICEntry (1 word) [array]
firstStub
* ICFallbackStub (3 words) [array]
code
enteredCount + isFallback
pcOffset + state
Because both are allocated as array, it should be possible to go from ICFallbackStub to the corresponding ICEntry with some pointer arithmetic (when attaching a new stub).
|
||
Comment 3•4 years ago
|
||
I like the version in comment #1, both for the cache locality aspect and for the possibility of using BaseIndex more in blinterp.
|
|
Assignee | |
Comment 4•4 years ago
|
||
Updating the bug title to reflect the comment 1 approach...
|
|
Assignee | |
Comment 5•3 years ago
|
||
There was just one caller left, the CacheIR health report code, and it can be
changed to iterate over the JitScript's ICEntries instead.
|
||
Updated•3 years ago
|
|
|
Assignee | |
Comment 6•3 years ago
|
||
These iterators are fairly complicated (the patch deletes > 130 lines of code) and
they were only used in a few places.
This simplifies the next patch a bit.
Depends on D114721
|
|
Assignee | |
Comment 7•3 years ago
|
||
This moves the pcOffset field from ICEntry to ICFallbackStub, and removes the
ICEntry* pointer from ICFallbackStub. Both of these classes are now one word smaller.
Because ICEntry and ICFallbackStubs are now both stored as ICScript arrays, we can
go from one to the other with some pointer arithmetic. This is pretty fast and
only has to happen on slower paths.
Another option would be to merge ICEntry and ICFallbackStub, but that complicates
the Baseline Interpreter code, especially on 32-bit platforms where the combined
size is not a nice power-of-two. With this patch we can use BaseIndex there.
Depends on D114722
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/969df17a7641 part 1 - Remove maybeICEntryFromPCOffset. r=iain https://hg.mozilla.org/integration/autoland/rev/33e0adcebde8 part 2 - Remove ICStubIterator and ICStubConstIterator. r=iain https://hg.mozilla.org/integration/autoland/rev/7e483465658a part 3 - Shrink ICEntry and ICFallbackStub. r=iain
|
||
Comment 9•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/969df17a7641
https://hg.mozilla.org/mozilla-central/rev/33e0adcebde8
https://hg.mozilla.org/mozilla-central/rev/7e483465658a
Description
•