ZJIT: Avoid compiling failed ISEQs repeatedly #14195
Conversation
| // If we already know we can't compile the ISEQ, fail early without cb.mark_all_executable(). | ||
| let cb = ZJITState::get_code_block(); | ||
| let payload = get_or_create_iseq_payload(iseq); | ||
| if cb.has_dropped_bytes() || payload.status == IseqStatus::CantCompile { |
There was a problem hiding this comment.
I think the payload status part of this check can happen without the VM lock, since the whole code path can be made read-only. But you still need the check as is while holding the VM lock in any case. Ractor concerns are far out, though. Could you leave a TODO?
There was a problem hiding this comment.
Left a TODO, quoting your comment as is 28f9f3c.
TBH I didn't understand what you said though. I interpreted your comment like "you can check payload.status == IseqStatus::CantCompile without the VM lock, but you still need the check with the VM lock". Can you elaborate more on when you can check it without the VM lock and when it needs to be checked with the VM lock?
Also, as explained above fn function_stub_hit, the primary use of this path is for cb.has_dropped_bytes() like YJIT (once we implement side exits for unsupported instructions). To get cb, we'd still need the VM lock (we could have an atomic global variable separately from cb if needed though).
There was a problem hiding this comment.
Both checks are necessary. If you find payload.status == IseqStatus::CantCompile outside the lock, you can exit without taking the lock. But if you do contend for the lock, by the time you get it you might've lost the race so you repeat the same check once you have the lock.
There was a problem hiding this comment.
Ah, that makes sense. Thanks.
|
Does this speed up some of the issues on yjit-bench? |
❌ Tests Failed✖️no tests failed ✔️62282 tests passed(1 flake) |
Oh yeah they're waaay faster. I'll update the issue. |
This PR lets
function_stub_hitexit quickly when the ISEQ has failed to compile before, instead of attempting it repeatedly.I updated
start_ptr: Option<CodePtr>tostatus: IseqStatusto avoid consuming more memory. We should also fix the compiler to emit a side-exit when there's an unsupported instruction so that we exit more efficiently without calling the Rust function, but it's for another PR.