1811310 - Try caching `isFixedSlot` in the megamorphic cache

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

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

From Jan's comment:

I'm wondering if it makes sense to have an isFixedSlot flag in the megamorphic cache entries if we have the space for it.. Then we could also pre-subtract numFixed for dynamic slots and store an offset rather than slot index, when storing the entry in the table... Not sure how much it matters.

It probably won't move the needle a ton, but it's simple and seems like a strict improvement over what we have.

Comment 1

[Jan de Mooij [:jandem]]

NI myself to do a quick experiment this week to see if it matters.

Comment 2

[Jan de Mooij [:jandem]]

I'm looking into this at the moment. I think we can get rid of some instructions and have more compact code, but I haven't measured perf impact yet.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1811310 - Cache isFixedSlot in MegamorphicCacheEntry. r?dthayer!

This makes our JIT code for the megamorphic cache lookup a bit more compact by
caching the slot's offset + isFixedSlot flag in a single 32-bit value.

This improves some local micro-benchmarks a bit, but it's hard to see an improvement
on Speedometer. Maybe it improves some of the subtests a tiny bit and combined with
the shorter and simpler JIT code that might be good enough.

It's possible to go further and combine the load instruction for fixed vs dynamic,
and to replace the isFixedSlot branch with a CMOV instruction, but that seemed to
be a bit harder on the CPU (the branch is probably quite predictable in practice)
so this is the more conservative approach.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/535b8e8d4e77
Cache isFixedSlot in MegamorphicCacheEntry. r=dthayer

Comment 5

[Jan de Mooij [:jandem]]

Leaving this open to do the same for the setprop cache.

Comment 6

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/535b8e8d4e77

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1811310 part 2 - Use TaggedSlotOffset for megamorphic setprop cache too. r?dthayer!

Probably either neutral on Speedometer or a tiny improvement on some subtests.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8361daa68ad
part 2 - Use TaggedSlotOffset for megamorphic setprop cache too. r=dthayer

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b8361daa68ad