Issue #20750 has been updated by kbrock (Keenan Brock). 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 [1]: https://github.com/oVirt/ovirt-engine-sdk-ruby/pull/10 [2]: https://github.com/search?q=rb_thread_call_with_gvl+language%3AC++NOT+is%3Afork++NOT+is%3Aarchived+NOT+path%3A**%2Fthread.c+NOT+path%3A*.h+NOT+path%3A**%2FFunction.c++NOT+path%3A**%2Fio.c&type=code [3]: https://github.com/ffi/ffi/blob/85d0fabce6ab5ceb3848f7e09a265341672a272d/ext... [4]: https://github.com/ruby/ruby/blob/5307c65c76774f8a5964ccdb8ed94412962b5eaa/g... [5]: https://github.com/ruby/ruby/blob/5307c65c76774f8a5964ccdb8ed94412962b5eaa/g... [6]: https://github.com/ruby/ruby/blob/5307c65c76774f8a5964ccdb8ed94412962b5eaa/e... [7]: https://github.com/ruby/ruby/blob/5307c65c76774f8a5964ccdb8ed94412962b5eaa/g... ---------------------------------------- Feature #20750: Expose ruby_thread_has_gvl_p in ruby/thread.h https://bugs.ruby-lang.org/issues/20750#change-109823 * Author: kbrock (Keenan Brock) * Status: Open ---------------------------------------- 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: ``` c // 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]: [1]: https://github.com/search?q=ruby_thread_has_gvl_p+language%3AC++NOT+is%3Afork++NOT+is%3Aarchived&type=code [2]: https://github.com/ruby/ruby/commit/9c04a0647fb99f7554cb2e04385ddbb970631e02 [3]: https://github.com/ruby/ruby/commit/e9a91d2c95dfe22ad0487952f7a1053ef9a5fd16 [4]: https://github.com/ruby/ruby/commit/6f09fc2efd1915c4337ce6dded52a8a8771c3222 [5]: https://github.com/ruby/ruby/blob/master/thread.c#L1878 [6]: https://github.com/ruby/ruby/blob/master/thread.c#L1923 ---Files-------------------------------- 11618.patch (3.18 KB) -- https://bugs.ruby-lang.org/