Rework inline cache handling #609
Conversation
Don't store the update flag in the IC because that's a) an out-of-band signalling mechanism, and b) makes JSInlineCache bigger than it needs to be. One is allocated per function so it adds up. Another reason for making this change is that it makes visible what I strongly suspect are bugs in the original implementation.
| static int32_t get_ic_prop_offset(JSInlineCache *ic, uint32_t cache_offset, | ||
| JSShape *shape) | ||
| static uint32_t get_ic_prop_offset(const JSInlineCacheUpdate *icu, |
There was a problem hiding this comment.
Offsets are stored as uint32_t so this should have been uint32_t from the start. Not that it matters in a practical sense, 2^31-1 is large enough by a wide margin. Still, bigger is better according to my girlfriend and 2^32 > 2^31.
| atom = get_u32(pc); | ||
| pc += 4; | ||
| sf->cur_pc = pc; | ||
| val = JS_GetPropertyInternal2(ctx, sp[-1], atom, sp[-1], NULL, FALSE); |
There was a problem hiding this comment.
Note how this was passing NULL instead of ic. Looks like another bug to me because a few lines below it is acting on ic->updated.
There was a problem hiding this comment.
Is there a way to have a test for that?
There was a problem hiding this comment.
Not at the moment (or not easily anyway) but I'm thinking about adding a debug module that would make it easier to inspect the textual representation of the bytecode (basically by using the existing bytecode printer to dump to a string.)
You can technically do that already with bjson.write and grubbing through the binary output but that's super fragile.
There was a problem hiding this comment.
A la the dis module in Python? That's cool!
Don't store the update flag in the IC because that's a) an out-of-band
signalling mechanism, and b) makes JSInlineCache bigger than it needs
to be. One is allocated per function so it adds up.
Another reason for making this change is that it makes visible what
I strongly suspect are bugs in the original implementation.
A third reason is to pave the way for lazy and preinit/second-chance ICs:
lazy: allocate on first use, not during codegen
preinit/second-chance: allocate on second use, based on the observation that a lot of code (maybe even most) only runs once, ergo, any work to set up ICs is actually a deoptimization
The second commit makes the functional change of updating IC offsets because... why didn't it do so in the first place? One of those aforementioned bugs, AFAICT- edit: replaced with asserts that check the offsets don't changeWith these changes, microbench.js finishes in 32s instead of 34s on my machine (good) although the numbers for individual benchmarks don't materially change.