-
Notifications
You must be signed in to change notification settings - Fork 27
support Ractor #75
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
support Ractor #75
Conversation
eregon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented on some surface-level things.
In general its' not great we need extra indirections and hurt readability this way for Ractor :/
I'm also concerned about performance, could you run #15 (comment) before and after this change?
|
From a design POV it's not ideal to have potentially 1 Timeout thread per Ractor, especially with many Ractors, as that can double the number of threads if Timeout is used in every Ractor. |
Yes. See my slide: https://rubykaigi.org/2024/presentations/ko1.html (my trial and not matured on API) |
Thanks, I hadn't seen those. So yeah, I think solution 1 makes sense for now. |
|
I think after addressing my comments above and simplifying the changes now that everything can be defined as singleton methods and not module_function this PR will probably look good enough to me to merge and I'll re-review then. |
lib/timeout.rb
Outdated
| TIMEOUT_THREAD_MUTEX = Mutex.new | ||
| @timeout_thread = nil | ||
| private_constant :CONDVAR, :QUEUE, :QUEUE_MUTEX, :TIMEOUT_THREAD_MUTEX | ||
| DATA = Struct.new(*%i{condvar queue queue_mutex timeout_thread_mutex timeout_thread}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking this should be named State since it is the state of this library.
As a bonus it respects normal naming for classes (CamelCase) without conflicting with ::Data
|
Thank you for the review and I'd merged your idea.
I think it is much simpler. For the performance, on my machine the measurement doesn't stable. Maybe I think we clear all of concerns. If there is no big issue, we can merge it and apply another fixes in future. |
lib/timeout.rb
Outdated
|
|
||
| # We keep a private reference so that time mocking libraries won't break | ||
| # Timeout. | ||
| GET_TIME = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can shorten them for example with
m = Process.method(:clock_gettime)
GET_TIME = (defined?(Ractor.make_shareable) && Ractor.make_shareable(m) rescue m) || mbut it is cosmetic issue (and difficult to understand by a glance).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine. It could also be:
GET_TIME = Process.method(:clock_gettime)
Ractor.make_shareable(GET_TIME) rescue nil # Only works on Ruby 4+There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, would just GET_TIME = Process.method(:clock_gettime).freeze work and be enough? That seems nicest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, would just
GET_TIME = Process.method(:clock_gettime).freezework and be enough?
I tried but it does not work, it's not detected as shareable.
I thought it should work though for objects which use RUBY_TYPED_FROZEN_SHAREABLE (or RUBY_TYPED_FROZEN_SHAREABLE_NO_REC in this case).
Possibly a bug or is it my misunderstanding that.freeze should make such object shareable?
1. Introduce State to store all status. 2. Store State instance to the Ractor local storage if possible 3. Make `GET_TIME` (Method object) shareable if possible 3 is supporeted Ruby 4.0 and later, so the Rator support is works only on Ruby 4.0 and later.
lib/timeout.rb
Outdated
|
|
||
| ::Timeout::RACTOR_SUPPORT = true # for test | ||
| else | ||
| @GLOBAL_STATE = State.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @GLOBAL_STATE = State.new | |
| GLOBAL_STATE = State.new |
A constant should be faster (at least with JIT)
That's a great idea, it looks much cleaner now because these methods can then just access |
eregon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, I will just push some tiny changes on top and then I'll merge this.
* Fix indentation to stay a multiple of 2 spaces.
d199b4c to
b9f49f9
Compare
52bf5f8 to
b5d7d77
Compare
|
All good now, there was a small bug which I fixed as well (see extra commits). |
|
@hsbt I would like to make a release of timeout from current master, is that OK? Is the process just bumping |
|
As for having a single timeout thread, I think we would need |
GET_TIME(Method object) shareable if possible3 is supporeted Ruby 4.0 and later, so the Rator support is works only on Ruby 4.0 and later.