Feature #20750
openAllow rb_thread_call_with_gvl to work when thread already has GVL
Description
Hello All,
I'm hoping we can make ruby_thread_has_gvl_p
a public method and no longer experimental.
I saw the following code in a gem that was not compiling with ruby 3.3:
// int ruby_thread_has_gvl_p(void); // <== line of concern
if (ruby_thread_has_gvl_p()) {
method_call(&context);
}
else {
rb_thread_call_with_gvl(method_call, &context);
}
400 unique projects on github added the line listed above to fix compilation. 1
Some of the projects detected the method first in extconf.rb
, but a majority just referenced it.
ffi
used to have the detection code but dropped it since the method was in all supported ruby versions.
It feels like this method is now part of the undocumented public interface.
My suggestion is to add the method to the real public interface in ruby/thread.h
.
PR with possible solution: https://github.com/ruby/ruby/pull/11619
Also included a patch if that is easier
Thank you for your consideration,
Keenan
Timeline:
- Dec 30 2008 -
rb_thread_call_with_gvl
introduced via 9c04a06 2 - Jan 12 2009 -
ruby_thread_has_gvl_p
introduced via 6f09fc2 4 - Jul 18 2012 -
rb_thread_call_with_gvl
made public via e9a91d2 3
Current references to code: rb_thread_call_with_gvl
5 ruby_thread_has_gvl_p
6:
Files
Updated by Eregon (Benoit Daloze) about 2 months ago
Note that the majority of the search results are just CRuby using it, and GitHub not detecting it's effectively copies of CRuby.
This seems like a strange pattern, why not just use rb_thread_call_with_gvl
and remove the condition?
That should be as efficient and not need to rely on private APIs.
Updated by kbrock (Keenan Brock) about 2 months ago
Hello Benoit,
Thank you for your question.
Eregon (Benoit Daloze) wrote in #note-1:
This seems like a strange pattern, why not just use
rb_thread_call_with_gvl
and remove the condition?
That should be as efficient and not need to rely on private APIs.
Yes, agreed. The pattern seems overly verbose and complicated.
It is part of a gem's PR to get ruby 3.3 working 1
To determine if this is strange, I went looking for how other code calls rb_thread_call_with_gvl
.
Patterns¶
I searched for rb_thread_call_with_gvl
to see the various calling patterns. 2
ffi¶
In ffi, they make the ruby_thread_has_gvl_p
check in the 1 method call to rb_thread_call_with_gvl
3
ruby using rb_thread_call_with_gvl
In ruby, it seems checking ruby_thread_has_gvl_p
before calling rb_thread_call_with_gvl
is a very common use case. 4 5 6
Of note, ruby has one other pattern:
Define methods specific to the case that the gvl is not acquired. (Though some of these examples like 7 may just be asserting the gvl is obtained)
ruby using ruby_thread_has_gvl_p
It is almost as if ruby_thread_has_gvl_p
is only used for 2 purposes:
- Decide to use or not use
rb_thread_call_with_gvl
(our use case). -
VM_ASSERT(ruby_thread_has_gvl_p())
- an extension of specific methods for gvl acquired.
conclusion¶
To me, it seems this pattern is valid.
Agreed that it would be nice if the code were organized better and it partitioned whether the gvl has already been acquired.
Are there other patterns I missed?
Keenan
Updated by Eregon (Benoit Daloze) about 2 months ago
So what is the concrete issue if the pattern above is replaced by rb_thread_call_with_gvl(method_call, &context);
?
Does rb_thread_call_with_gvl()
fail if the GVL is already acquired?
If so, I think that would be a bug of rb_thread_call_with_gvl()
and it should be fixed.
From the name it seems pretty clear it should just call the callback if the GVL is already acquired.
Updated by Eregon (Benoit Daloze) about 2 months ago
OK the issue/bug is
https://github.com/ruby/ruby/blob/4797b0704ae49fb42c8ad9a45028efbe2298b5f5/thread.c#L1899
if (brb == 0) {
rb_bug("rb_thread_call_with_gvl: called by a thread which has GVL.");
}
@ko1 (Koichi Sasada) I think that's a bug, I think rb_thread_call_with_gvl() should be allowed if the GVL is already acquired, there seems to be no value to prevent it.
TruffleRuby already implements those semantics BTW.
How do you want to solve this issue?
Updated by kbrock (Keenan Brock) about 2 months ago
Benoit,
Thank you.
Maybe I should have run the more complete pattern that I see in the ruby code base:
if(!ruby_native_thread_p()) { // (th = ruby_thread_from_native()) == 0
//issues
else if(ruby_thread_has_gvl_p()) { // (brb = th->blocking_region_buffer) == 0
func(params);
} else {
rb_thread_call_with_gvl(func, params);
}
I had not wanted to be intrusive, but I think this solution makes much more sense.
Updated by kbrock (Keenan Brock) about 2 months ago
- File 11649-lenent-rb_thread_call_with_gvl.patch 11649-lenent-rb_thread_call_with_gvl.patch added
- File 11649b-additional-changes.patch 11649b-additional-changes.patch added
Hello,
Here is the new proposed patch for your review.
https://github.com/ruby/ruby/pull/11649
I do not know if we try and keep changes as small as possible (to avoid introducing bugs)
Or if we refactor along the way.
Updated by kbrock (Keenan Brock) about 2 months ago
- Subject changed from Expose ruby_thread_has_gvl_p in ruby/thread.h to Allow rb_thread_call_with_gvl to work when thread already has GVL
Updated by matz (Yukihiro Matsumoto) about 1 month ago
OK, I accept to make rb_thread_call_with_gvl
to acquire GVL only when needed. @ko1 (Koichi Sasada) worried it may encourage bad design pattern, but not allowing rb_thread_call_with_gvl
with GVL does not improve the situation.
So I accept it, you handle it with care.
Matz.