1682504 - Shrink ICEntry and ICFallbackStub

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P3)

Description

[Jan de Mooij [:jandem]]

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 to ICCacheIRStub. 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.

Comment 1

[Jan de Mooij [:jandem]]

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

[Iain Ireland [:iain]]

I like the version in comment #1, both for the cache locality aspect and for the possibility of using BaseIndex more in blinterp.

Comment 4

[Jan de Mooij [:jandem]]

Updating the bug title to reflect the comment 1 approach...

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1682504 part 1 - Remove maybeICEntryFromPCOffset. r?iain!

There was just one caller left, the CacheIR health report code, and it can be
changed to iterate over the JitScript's ICEntries instead.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1682504 part 2 - Remove ICStubIterator and ICStubConstIterator. r?iain!

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

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1682504 part 3 - Shrink ICEntry and ICFallbackStub. r?iain!

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/969df17a7641
https://hg.mozilla.org/mozilla-central/rev/33e0adcebde8
https://hg.mozilla.org/mozilla-central/rev/7e483465658a