Skip to content
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

YJIT: Introduce no_gc attribute #7511

Merged
merged 1 commit into from Mar 14, 2023
Merged

YJIT: Introduce no_gc attribute #7511

merged 1 commit into from Mar 14, 2023

Conversation

k0kubun
Copy link
Member

@k0kubun k0kubun commented Mar 13, 2023

  • Introduce no_gc attribute to skip that jit_prepare_routine_call when possible
    • If a method doesn't allocate, we should be able to skip jit_prepare_routine_call.

Kernel#class is one of such methods that are called often on railsbench Shopify/yjit-bench#168.

@matzbot matzbot requested a review from a team March 13, 2023 20:30
@XrXr
Copy link
Member

XrXr commented Mar 13, 2023

If leaf, it doesn't raise either. PC should be irrelevant.

It's relevant for allocation tracing, and we've seen that not updating PC can cause SEGV with stackprof. Also, since GC contends for the VM lock, another thread might have triggered global invalidation while it's waiting. In which case we need the patch point recording in jit_prepare_routine_call to exit as soon as possible.

By the way, allocation implies raising because of NoMemoryError

@k0kubun
Copy link
Member Author

k0kubun commented Mar 13, 2023

It's relevant for allocation tracing, and we've seen that not updating PC can cause SEGV with stackprof. Also, since GC contents for the VM lock, when it gets it another thread might have triggered global invalidation. In which case we need the patch point recording in jit_prepare_routine_call to exit as soon as possible.

Ah, I see. I'll keep the original jit_prepare_routine_call while still eliminating this for no_gc.

By the way, this means that we should fix #7466 too. We paired on this during your vacation, I wrote jit_prepare_routine_call first, but ended up using gen_save_sp because I couldn't explain why we'd need to update PC.

By the way, allocation implies raising because of NoMemoryError

I discussed this on ruby-lang Slack before. Koichi's take was that NoMemoryError virtually doesn't exist on Linux because it gets killed by OOM. Anyway, as long as we'll call jit_prepare_routine_call, this is no longer relevant.

@k0kubun
Copy link
Member Author

k0kubun commented Mar 13, 2023

Alas, CI notices String#getbyte wasn't actually leaf sometimes. I think we need something like leaf_if_arg_is: Fixnum. Anyway, for this PR, I'll try to find another example.

@k0kubun
Copy link
Member Author

k0kubun commented Mar 13, 2023

I added this to Kernel#class instead. It's as important as String#getbyte on railsbench.

@k0kubun k0kubun force-pushed the yjit-no-gc branch 2 times, most recently from b5619be to b510bf8 Compare March 13, 2023 21:38
@k0kubun k0kubun merged commit 70ba310 into ruby:master Mar 14, 2023
98 checks passed
@k0kubun k0kubun deleted the yjit-no-gc branch March 14, 2023 22:39
k0kubun added a commit that referenced this pull request Mar 15, 2023
to see if it stabilizes Cirrus CI.
k0kubun added a commit to Shopify/ruby that referenced this pull request Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants