Skip to content
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

Reimplement the monitor stdlib in Java #2375

Closed
eregon opened this issue Jun 14, 2021 · 5 comments
Closed

Reimplement the monitor stdlib in Java #2375

eregon opened this issue Jun 14, 2021 · 5 comments
Assignees
Milestone

Comments

@eregon
Copy link
Member

eregon commented Jun 14, 2021

Currently we use monitor.rb: https://github.com/oracle/truffleruby/blob/d2532c21cd85663329e083de1870a6466f258766/lib/mri/monitor.rb
That's quite inefficient, notably because it uses a lot of Thread.handle_interrupt which seems quite expensive.
By moving it to Java code we can avoid the need for changing the interrupt mode, because we have better control where guest safepoints can happen (i.e., no risk for a Thread#raise in the middle of ensure with Java finally, which is what those Thread.handle_interrupt prevent).
Probably using a ReetrantLock + Condition and reusing Mutex code is the way to go.

CRuby rewrote monitor to a C extension in ruby/ruby#2576 https://bugs.ruby-lang.org/issues/16255.

@eregon eregon changed the title Reimplement the monitor stdlib in Java, as core Reimplement the monitor stdlib in Java Jun 14, 2021
@chrisseaton
Copy link
Collaborator

I see this everywhere

               MonitorMixin#mon_synchronize                                                           |          1ms   0.0% |   0.0% ||          0ms   0.0% |   0.0% | ../graal/sdk/mxbuild/linux-amd64/GRAALVM_92F985B265_JAVA11/graalvm-92f985b265-java11-21.2.0-dev/languages/ruby/lib/mri/monitor.rb~230-243:5366-5709 
                Thread.handle_interrupt                                                               |          1ms   0.0% |   0.0% ||          0ms   0.0% |   0.0% | resource:/truffleruby/core/thread.rb~86-95:3743-4110 
                 Thread#handle_interrupt                                                              |          1ms   0.0% |   0.0% ||          0ms   0.0% |   0.0% | (core)~1:0 
                  block in MonitorMixin#mon_synchronize                                               |          1ms   0.0% |   0.0% ||          0ms   0.0% |   0.0% | ../graal/sdk/mxbuild/linux-amd64/GRAALVM_92F985B265_JAVA11/graalvm-92f985b265-java11-21.2.0-dev/languages/ruby/lib/mri/monitor.rb~233-236:5497-5627 
                   MonitorMixin#mon_enter      

Doesn't seem like it takes up a lot of self time anywhere, but it must add an inlining burden.

Used in key operations like Logger::LogDevice#write.

@aardvark179
Copy link
Contributor

This seems doable. We already have a Java implementation of Mutex and ConditionVariabl, and since the mutex is a reeentrant lock under the hood it could easily serve as a monitor if we exposed the right methods. We'd also need a way to create the condition variable linked to the underlying monitory, but again this seems entirely doable.

@eregon eregon added this to the 21.3.0 milestone Jul 26, 2021
@eregon
Copy link
Member Author

eregon commented Jul 26, 2021

Fixed in 00b825c

@eregon eregon closed this as completed Jul 26, 2021
@chrisseaton
Copy link
Collaborator

How much faster did this make it?

@aardvark179
Copy link
Contributor

I added a monitor/synchronize microbenchmark as part of this work. Without the change the peak performance was 4221084 iterations per second, and with the change it is 20322560 iterations per second, so about a 5x speed increase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants