1365069 - Baldr: avoid wasm::Compartment::lookupCode on warm paths

Status: RESOLVED DUPLICATE of bug 1277562

(Core :: JavaScript Engine, enhancement)

Description

[Luke Wagner [:luke]]

Attachment: lookup-code-less

Assuming bug 1359027 adds a lock to lookupCode, it'd be nice to avoid calling this super-often.  After bug 1334504, we can now find a frame's code simply by fp->tls->instance->code so many of these uses of lookupCode() are now unnecessary.

Comment 1

[Lars T Hansen [:lth]]

Comment on attachment 8867906 [details] [diff] [review]
lookup-code-less

Review of attachment 8867906 [details] [diff] [review]:
-----------------------------------------------------------------

Generally this seems fine, but if I hesitate it is because the assertion in the patch comment is not (or will not be) correct, you *cannot* "find a frame's code simply by fp->tls->instance->code", because that instance does not have a code hanging off it, it has something that has potentially two Code objects, one per tier.  Really, it is a lookup-by-pc operation on instance->code that yields (or will yield) "the" Code for the frame.

I wonder if we can delay this until after bug 1334504, at least, so that we have a better idea about what the data structures might look like and what kind of locking we might need.

Comment 2

[Lars T Hansen [:lth]]

Of course I meant to write that we should delay until after bug 1359027.

Comment 3

[Luke Wagner [:luke]]

Sure, happy to hold off for bug 1334504.  Mostly, I just wanted to convince myself that we wouldn't be adding a locked lookup to any warmish paths like profiling's stack-walking.

FWIW, I think with tiering the tweak to this patch would be that you'd write fp->tls->instance->code(pc), passing the 'pc' so the instance could do lock-free query of whether pc was in each of its codes.  (Lock free b/c even if a tier can be added asynchronously, as long as each tier is stored in an Atomic, the pc you have is for a halted thread so the tier containing pc will definitely be set.)

Comment 4

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #3)

> FWIW, I think with tiering the tweak to this patch would be that you'd write
> fp->tls->instance->code(pc), passing the 'pc' so the instance could do
> lock-free query of whether pc was in each of its codes.  (Lock free b/c even
> if a tier can be added asynchronously, as long as each tier is stored in an
> Atomic, the pc you have is for a halted thread so the tier containing pc
> will definitely be set.)

Yup, makes sense.

Comment 5

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #4)
> (In reply to Luke Wagner [:luke] from comment #3)
> 
> > FWIW, I think with tiering the tweak to this patch would be that you'd write
> > fp->tls->instance->code(pc), passing the 'pc' so the instance could do
> > lock-free query of whether pc was in each of its codes.  (Lock free b/c even
> > if a tier can be added asynchronously, as long as each tier is stored in an
> > Atomic, the pc you have is for a halted thread so the tier containing pc
> > will definitely be set.)
> 
> Yup, makes sense.

Indeed the patch I just uploaded to bug 1277562 contains several such accessors on Code.

Comment 6

[Luke Wagner [:luke]]

(Want me to rebase, r? and land this independently or will you fold this into your patch stack?)

Comment 7

[Lars T Hansen [:lth]]

I'll claim it and insert it into my queue.

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1365069-cache-code-object.patch

Rebased to sit on top of the patch queue in bug 1277562.  Will ask for review on this one as soon as the patches on that one are approved.

Comment 9

[Lars T Hansen [:lth]]

Patch being merged into 1277562 and landed with that.