-
Notifications
You must be signed in to change notification settings - Fork 419
Implementation of Concurrent::JavaSemaphore in pure Java. #238
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
Conversation
Wow. I'm still surprised that the performance is so different, but you can't argue with the data! 👍 |
@jdantonio This one is done. I'll migrate one class at time so we can analyze them individually and easily revert if necessary. Should I merge it? |
public static class JavaSemaphore extends RubyObject { | ||
|
||
private JRubySemaphore semaphore; | ||
private ThreadContext context; |
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.
As mentioned here #236 (comment), this should not be stored probably.
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.
@pitr-ch Makes completely sense! I'll remove it here and I'll open a new pull request to remove the ThreadContext
from JavaAtomicFixnum
and JavaBooleanFixnum
.
BTW thanks for doing this! |
@lucasallan Absolutely. Once the tests pass, we have some performance tests, and everyone has had a day or two to review, please feel free to merge. Thank you for taking this on! |
381e844
to
30fbf4b
Compare
Implementation of Concurrent::JavaSemaphore in pure Java.
|
||
@JRubyMethod(name = "drain_permits") | ||
public IRubyObject drainPermits(ThreadContext context) { | ||
return new RubyFixnum(getRuntime(), this.semaphore.drainPermits()); |
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've missed one more thing. JRuby extensions should try to use interface provided by Ruby
object (result of getRuntime()
) it may do additional steps like caching etc. see https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/RubyFixnum.java#L207-L212. So it's better to use the newBoolean
and newFixnum
methods on Ruby
object.
@jdantonio Another good performance improvement:
Pure Java
JRuby