* [ruby-core:119239] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h
@ 2024-09-17 16:52 kbrock (Keenan Brock) via ruby-core
2024-09-17 20:03 ` [ruby-core:119240] " Eregon (Benoit Daloze) via ruby-core
` (6 more replies)
0 siblings, 7 replies; 8+ messages in thread
From: kbrock (Keenan Brock) via ruby-core @ 2024-09-17 16:52 UTC (permalink / raw)
To: ruby-core; +Cc: kbrock (Keenan Brock)
Issue #20750 has been reported by kbrock (Keenan Brock).
----------------------------------------
Feature #20750: Expose ruby_thread_has_gvl_p in ruby/thread.h
https://bugs.ruby-lang.org/issues/20750
* 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/
______________________________________________
ruby-core mailing list -- ruby-core@ml.ruby-lang.org
To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/
^ permalink raw reply [flat|nested] 8+ messages in thread
* [ruby-core:119240] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h
2024-09-17 16:52 [ruby-core:119239] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h kbrock (Keenan Brock) via ruby-core
@ 2024-09-17 20:03 ` Eregon (Benoit Daloze) via ruby-core
2024-09-17 22:34 ` [ruby-core:119241] " kbrock (Keenan Brock) via ruby-core
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Eregon (Benoit Daloze) via ruby-core @ 2024-09-17 20:03 UTC (permalink / raw)
To: ruby-core; +Cc: Eregon (Benoit Daloze)
Issue #20750 has been updated by Eregon (Benoit Daloze).
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.
----------------------------------------
Feature #20750: Expose ruby_thread_has_gvl_p in ruby/thread.h
https://bugs.ruby-lang.org/issues/20750#change-109822
* 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/
______________________________________________
ruby-core mailing list -- ruby-core@ml.ruby-lang.org
To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/
^ permalink raw reply [flat|nested] 8+ messages in thread
* [ruby-core:119241] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h
2024-09-17 16:52 [ruby-core:119239] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h kbrock (Keenan Brock) via ruby-core
2024-09-17 20:03 ` [ruby-core:119240] " Eregon (Benoit Daloze) via ruby-core
@ 2024-09-17 22:34 ` kbrock (Keenan Brock) via ruby-core
2024-09-18 11:56 ` [ruby-core:119253] " Eregon (Benoit Daloze) via ruby-core
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: kbrock (Keenan Brock) via ruby-core @ 2024-09-17 22:34 UTC (permalink / raw)
To: ruby-core; +Cc: kbrock (Keenan Brock)
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/ffi_c/Function.c#L603-L608
[4]: https://github.com/ruby/ruby/blob/5307c65c76774f8a5964ccdb8ed94412962b5eaa/gc.c#L4044
[5]: https://github.com/ruby/ruby/blob/5307c65c76774f8a5964ccdb8ed94412962b5eaa/gc.c#L4081
[6]: https://github.com/ruby/ruby/blob/5307c65c76774f8a5964ccdb8ed94412962b5eaa/ext/fiddle/closure.c#L229
[7]: https://github.com/ruby/ruby/blob/5307c65c76774f8a5964ccdb8ed94412962b5eaa/gc/default.c#L6789-L6803
----------------------------------------
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/
______________________________________________
ruby-core mailing list -- ruby-core@ml.ruby-lang.org
To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/
^ permalink raw reply [flat|nested] 8+ messages in thread
* [ruby-core:119253] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h
2024-09-17 16:52 [ruby-core:119239] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h kbrock (Keenan Brock) via ruby-core
2024-09-17 20:03 ` [ruby-core:119240] " Eregon (Benoit Daloze) via ruby-core
2024-09-17 22:34 ` [ruby-core:119241] " kbrock (Keenan Brock) via ruby-core
@ 2024-09-18 11:56 ` Eregon (Benoit Daloze) via ruby-core
2024-09-18 12:00 ` [ruby-core:119254] " Eregon (Benoit Daloze) via ruby-core
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Eregon (Benoit Daloze) via ruby-core @ 2024-09-18 11:56 UTC (permalink / raw)
To: ruby-core; +Cc: Eregon (Benoit Daloze)
Issue #20750 has been updated by Eregon (Benoit Daloze).
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.
----------------------------------------
Feature #20750: Expose ruby_thread_has_gvl_p in ruby/thread.h
https://bugs.ruby-lang.org/issues/20750#change-109833
* 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/
______________________________________________
ruby-core mailing list -- ruby-core@ml.ruby-lang.org
To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/
^ permalink raw reply [flat|nested] 8+ messages in thread
* [ruby-core:119254] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h
2024-09-17 16:52 [ruby-core:119239] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h kbrock (Keenan Brock) via ruby-core
` (2 preceding siblings ...)
2024-09-18 11:56 ` [ruby-core:119253] " Eregon (Benoit Daloze) via ruby-core
@ 2024-09-18 12:00 ` Eregon (Benoit Daloze) via ruby-core
2024-09-19 15:55 ` [ruby-core:119260] " kbrock (Keenan Brock) via ruby-core
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Eregon (Benoit Daloze) via ruby-core @ 2024-09-18 12:00 UTC (permalink / raw)
To: ruby-core; +Cc: Eregon (Benoit Daloze)
Issue #20750 has been updated by Eregon (Benoit Daloze).
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 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?
----------------------------------------
Feature #20750: Expose ruby_thread_has_gvl_p in ruby/thread.h
https://bugs.ruby-lang.org/issues/20750#change-109834
* 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/
______________________________________________
ruby-core mailing list -- ruby-core@ml.ruby-lang.org
To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/
^ permalink raw reply [flat|nested] 8+ messages in thread
* [ruby-core:119260] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h
2024-09-17 16:52 [ruby-core:119239] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h kbrock (Keenan Brock) via ruby-core
` (3 preceding siblings ...)
2024-09-18 12:00 ` [ruby-core:119254] " Eregon (Benoit Daloze) via ruby-core
@ 2024-09-19 15:55 ` kbrock (Keenan Brock) via ruby-core
2024-09-19 16:39 ` [ruby-core:119261] " kbrock (Keenan Brock) via ruby-core
2024-10-03 4:43 ` [ruby-core:119416] [Ruby master Feature#20750] Allow rb_thread_call_with_gvl to work when thread already has GVL matz (Yukihiro Matsumoto) via ruby-core
6 siblings, 0 replies; 8+ messages in thread
From: kbrock (Keenan Brock) via ruby-core @ 2024-09-19 15:55 UTC (permalink / raw)
To: ruby-core; +Cc: kbrock (Keenan Brock)
Issue #20750 has been updated by kbrock (Keenan Brock).
Benoit,
Thank you.
Maybe I should have run the more complete pattern that I see in the ruby code base:
```c
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.
----------------------------------------
Feature #20750: Expose ruby_thread_has_gvl_p in ruby/thread.h
https://bugs.ruby-lang.org/issues/20750#change-109849
* 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/
______________________________________________
ruby-core mailing list -- ruby-core@ml.ruby-lang.org
To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/
^ permalink raw reply [flat|nested] 8+ messages in thread
* [ruby-core:119261] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h
2024-09-17 16:52 [ruby-core:119239] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h kbrock (Keenan Brock) via ruby-core
` (4 preceding siblings ...)
2024-09-19 15:55 ` [ruby-core:119260] " kbrock (Keenan Brock) via ruby-core
@ 2024-09-19 16:39 ` kbrock (Keenan Brock) via ruby-core
2024-10-03 4:43 ` [ruby-core:119416] [Ruby master Feature#20750] Allow rb_thread_call_with_gvl to work when thread already has GVL matz (Yukihiro Matsumoto) via ruby-core
6 siblings, 0 replies; 8+ messages in thread
From: kbrock (Keenan Brock) via ruby-core @ 2024-09-19 16:39 UTC (permalink / raw)
To: ruby-core; +Cc: kbrock (Keenan Brock)
Issue #20750 has been updated by kbrock (Keenan Brock).
File 0001-rb_thread_call_with_gvl-is-lenient-when-it-has-alrea.patch added
File 0002-Don-t-check-GVL-before-calling-into-with_gvl_callbac.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.
----------------------------------------
Feature #20750: Expose ruby_thread_has_gvl_p in ruby/thread.h
https://bugs.ruby-lang.org/issues/20750#change-109850
* 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)
0001-rb_thread_call_with_gvl-is-lenient-when-it-has-alrea.patch (1.26 KB)
0002-Don-t-check-GVL-before-calling-into-with_gvl_callbac.patch (2.91 KB)
--
https://bugs.ruby-lang.org/
______________________________________________
ruby-core mailing list -- ruby-core@ml.ruby-lang.org
To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/
^ permalink raw reply [flat|nested] 8+ messages in thread
* [ruby-core:119416] [Ruby master Feature#20750] Allow rb_thread_call_with_gvl to work when thread already has GVL
2024-09-17 16:52 [ruby-core:119239] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h kbrock (Keenan Brock) via ruby-core
` (5 preceding siblings ...)
2024-09-19 16:39 ` [ruby-core:119261] " kbrock (Keenan Brock) via ruby-core
@ 2024-10-03 4:43 ` matz (Yukihiro Matsumoto) via ruby-core
6 siblings, 0 replies; 8+ messages in thread
From: matz (Yukihiro Matsumoto) via ruby-core @ 2024-10-03 4:43 UTC (permalink / raw)
To: ruby-core; +Cc: matz (Yukihiro Matsumoto)
Issue #20750 has been updated by matz (Yukihiro Matsumoto).
OK, I accept to make `rb_thread_call_with_gvl` to acquire GVL only when needed. @ko1 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.
----------------------------------------
Feature #20750: Allow rb_thread_call_with_gvl to work when thread already has GVL
https://bugs.ruby-lang.org/issues/20750#change-110030
* 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)
11649-lenent-rb_thread_call_with_gvl.patch (1.26 KB)
11649b-additional-changes.patch (2.91 KB)
--
https://bugs.ruby-lang.org/
______________________________________________
ruby-core mailing list -- ruby-core@ml.ruby-lang.org
To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-10-03 4:43 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-09-17 16:52 [ruby-core:119239] [Ruby master Feature#20750] Expose ruby_thread_has_gvl_p in ruby/thread.h kbrock (Keenan Brock) via ruby-core
2024-09-17 20:03 ` [ruby-core:119240] " Eregon (Benoit Daloze) via ruby-core
2024-09-17 22:34 ` [ruby-core:119241] " kbrock (Keenan Brock) via ruby-core
2024-09-18 11:56 ` [ruby-core:119253] " Eregon (Benoit Daloze) via ruby-core
2024-09-18 12:00 ` [ruby-core:119254] " Eregon (Benoit Daloze) via ruby-core
2024-09-19 15:55 ` [ruby-core:119260] " kbrock (Keenan Brock) via ruby-core
2024-09-19 16:39 ` [ruby-core:119261] " kbrock (Keenan Brock) via ruby-core
2024-10-03 4:43 ` [ruby-core:119416] [Ruby master Feature#20750] Allow rb_thread_call_with_gvl to work when thread already has GVL matz (Yukihiro Matsumoto) via ruby-core
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).