-
Notifications
You must be signed in to change notification settings - Fork 5.4k
ZJIT: Write a callee frame on JIT-to-JIT calls #13579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After our conversation, overall it looks good.
I look forward to making this much lazier in the future.
I feel like we should have more tests, and more rigorous tests, for this new feature. Can we add some more to exercise this?
Co-authored-by: Max Bernstein <tekknolagi@gmail.com>
Co-authored-by: Max Bernstein <tekknolagi@gmail.com>
I added tests that dump non-argument locals on side exits 5b621b1 and one that checks Another test target would be the content of dumped locals. But to test the content of locals from a callee, we'd need to load a C extension in Do you have other things you want to be tested in mind? We could just "have more tests" as you said, but I generally try to avoid bloating the number of tests by adding meaningless/duplicated test cases. |
❌ Tests Failed✖️no tests failed ✔️61994 tests passed(1 flake) |
I don't mean to bloat the tests but did not (at the time of comment) have a good set of recommendations for things to test. 2 tests for a bunch of new functionality just seemed a bit light. I can imagine having a bunch of dark untested corner cases |
I think we should aim for enabling This project should have got the test setup much earlier, but we were so focused on keeping generated code optimal that we never had the "rigorous tests" we could have got for free. This PR should bring us much closer to having it. |
You approved the PR and the CI passed, so I'll merge this PR to unblock your stacked PR. We can add more tests as we come up with untested scenarios that are worth testing. |
This PR changes JIT-to-JIT calls to actually write a callee frame that is about to be pushed.
When JIT code makes a non-leaf call, it may dispatch an arbitrary method that looks at caller frames. When that happens, the method may read the locals, PC, ISEQ, and other things of the frame. Because that happens before the frame side-exits, we need to somehow make them available when needed.
As a first implementation to make that work, this PR writes them when a JIT frame is pushed. ZJIT should ideally optimize away the overhead, but I think it's nice to make things work first as we're onboarding more people to work on ZJIT.