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

Expose rb_fiber_raise and tidy up the internal implementation. #4649

Merged
merged 2 commits into from
Sep 20, 2021

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Jul 14, 2021

Expose rb_fiber_raise so it can be called from C extensions.

Rework the internal implementation to avoid all the indirection for resuming_fiber and fiber_ptr usage. This should generate slightly more efficient assembly.

@ioquatix
Copy link
Member Author

ioquatix commented Jul 14, 2021

@eregon @headius I would like to understand how much of a burden this is on the JVM implementations of Ruby.

It's advantageous for us to expose these functions from C because it avoids method lookup which can be expensive on the hot path. However, it's also different to use these functions vs something like rb_funcall which would handle users overriding those methods. I wonder if our implementation should handle this more "correctly", e.g.

VALUE rb_fiber_transfer(VALUE fiber) {
  if (rb_obj_is_fiber(fiber)) ... direct code path ...
  else ... rb_funcall(... id_transfer ...)
}

I generally feel a few more methods here allows us to reduce overhead by quite a lot in native extensions. But obviously it's more work for other implementations to pick up these changes and support them.

@eregon
Copy link
Member

eregon commented Jul 14, 2021

For TruffleRuby, if it's OK to implement rb_fiber_transfer() internally as a call to Fiber#transfer (what TruffleRuby does for maybe 80% of C API functions) then the overhead is negligible (3 lines of code).
rb_funcall() uses an inline cache on TruffleRuby, so there is not much lookup overhead.

In general I wish CRuby would do better inline caching for rb_funcall() if that's possible, so there is less need to have many C API functions when rb_funcall is already working.
It would be interesting to actually measure the difference on recent CRuby.

If you would like other implementations supporting the C API to support rb_fiber_transfer() then the best way is to add a spec under spec/ruby/optional/capi for it.

@ioquatix
Copy link
Member Author

@eregon Thanks for informing me those details.

I agree better MIC or PIC could be great, even if we had to allocate some local storage for it.

Making rb_funcall really fast would be better than C functions for the reasons I described like better polymorphism support.

I will try to introduce C API specs.

@ioquatix ioquatix force-pushed the expose_rb_fiber_raise branch 3 times, most recently from 3930ea6 to 7414040 Compare September 20, 2021 02:51
spec/ruby/optional/capi/ext/fiber_spec.c Outdated Show resolved Hide resolved
spec/ruby/optional/capi/fiber_spec.rb Show resolved Hide resolved
@nobu
Copy link
Member

nobu commented Sep 20, 2021

This rb_fiber_raise accepts argc and argv, then creates an exception instance.
Isn't it better to pass an exception and just raise it, as an C API?

@ioquatix ioquatix force-pushed the expose_rb_fiber_raise branch 3 times, most recently from 5dc1a53 to 63bbbc7 Compare September 20, 2021 03:39
@ioquatix
Copy link
Member Author

@nobu the goal is to have:

rb_fiber_raise(fiber, ...)

The same as:

rb_funcallv(fiber, ...)

As discussed on Slack.

@ioquatix ioquatix requested a review from nobu September 20, 2021 03:41
@ioquatix ioquatix merged commit 649c87b into ruby:master Sep 20, 2021
@ioquatix
Copy link
Member Author

cc @eregon there is a C extension spec, do we need to copy it to the ruby spec repo?

@ioquatix ioquatix deleted the expose_rb_fiber_raise branch September 20, 2021 06:31
@eregon
Copy link
Member

eregon commented Sep 21, 2021

spec/ruby is sync'd monthly, nothing extra needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants