1789457 - Tune the megamorphic cache and associated code

Status: RESOLVED FIXED

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

Description

[Alex Thayer [:alexical] (she/her)]

This is a bit of grab-bag of proposed changes to the megamorphic cache. In summary:

1. Slightly change the hashing of the shape to help avoid hitting pathological cases.
2. Remove the branchIfNonNativeObject bailout path in the places which call GetNativeDataPropertyPure and friends until after the cache lookup, since we'll never get a cache hit on a non-native object anyway.
3. Inline the cache lookup.

Comment 1

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1789457 - Shift out alignment bits of Shape pointer r?jandem

The first three bits of the Shape pointer will always be 0, so we lose out on
some entropy here. I initially tried a "real" hashing algorithm, but noticed as
you likely already did that this can be incredibly hot and even adding a single
multiply in meaningfully degrades the performance of this. However, adding this
shift in just adds one mov and one shr, and I couldn't get it to regress perf
for a microbenchmark that tried to make it irrelevant. However, it did speed up
a microbenchmark that used the same key over and over again against 100
different shapes by a factor of about 25%, so I think it's worth it to avoid
potential pathological cases.

Further observations: we do hit this issue reasonably frequently in the matrix
react bench as well as in speedometer - this patch reduces cache misses where
only the shape was different quite substantially. However, those misses are
a small enough portion of overall cache misses that this patch doesn't seem to
observably affect those benchmarks' overall numbers.

Lastly, I wish I could have computed these shifts instead of just leaving a
comment explaining how they were derived. However, our tools for doing this in
C++ are not constexpr-friendly it seems. Also, I made these public because
they will be used from the MacroAssembler in later patches.

Comment 2

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1789457 - Move NativeObject check after cache lookup r?jandem

Initially we explored caching this check on the shape, which still seems like
a decent idea. However, we can also just sidestep the problem for the most part
be moving the check after the cache lookup, since we can't get a cache hit if
it's not a NativeObject anyway, and we're hoping that a cache hit should be the
dominant path in most cases.

Depends on D156613

Comment 3

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1789457 - Inline megamorphic cache lookup in Ion r?jandem

This inlines the megamorphic cache lookup, netting us about a 30-40%
improvement for the microbenchmark on top of the prior optimizations. There is
no measured performance losses for speedometer or matrix-react, and maybe a
1% win overall, though I need to do more runs with the cleaned up patch to be
confident.

We could do similar inlining for the CacheIRCompiler version and for
MegamorphicLoadSlotByValue - however, we wouldn't be able to include the id and
id hash as immediates, which would mean we'd need another register and several
more instructions to make it work, and it would be awkward to share the masm
code because of the different register requirements.

Depends on D156614

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ff12c46bc20
Shift out alignment bits of Shape pointer r=jandem
https://hg.mozilla.org/integration/autoland/rev/080d321a6baf
Move NativeObject check after cache lookup r=jandem
https://hg.mozilla.org/integration/autoland/rev/da8bb74f3daa
Inline megamorphic cache lookup in Ion r=jandem

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/5ff12c46bc20
https://hg.mozilla.org/mozilla-central/rev/080d321a6baf
https://hg.mozilla.org/mozilla-central/rev/da8bb74f3daa