ZJIT: Support invalidating constant patch points #13998
Conversation
| @@ -148,6 +148,7 @@ rb_clear_constant_cache_for_id(ID id) | |||
| } | |||
|
|
|||
| rb_yjit_constant_state_changed(id); | |||
| rb_zjit_constant_state_changed(id); | |||
There was a problem hiding this comment.
If this is only keyed on one ID, does that mean we will invalidate A::B::C if D::E::C changes (because they both share C)?
There was a problem hiding this comment.
Yes when C is redefined the current implementation invalidates all jumps associated with it (confirmed with debugger and print messages). But since the callback only receives one ID, I'm not sure if there's a way to reconstruct the full constant path based on it.
It looks like YJIT behaves the same way?
@k0kubun is this correct?
There was a problem hiding this comment.
Yeah, it does seem like it. rb_clear_constant_cache_for_id(ID id) is called when the id is used in the scope of a module. So if you invalidate constants with an id, that would invalidate constants with the same name under different modules.
I do think we need to do that anyway though. If the ISEQ is on a module, any random class could include it and use the JIT code. We can't really scope the invalidation into specific classes/modules unless you scan every class/module that could use it, which seems unrealistic.
There was a problem hiding this comment.
Thanks for the clarification!
|
YJIT also invalidates constants on |
|
lgtm after addressing ^ |
|
So, for correctness, Since ZJIT currently only discards invalid code and never recompiles them, we probably don't need the hook yet. Let's merge this as is for now. Thanks! |
No description provided.