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

[Truffle] No more global lock! #3148

Merged
merged 2 commits into from
Jul 23, 2015
Merged

[Truffle] No more global lock! #3148

merged 2 commits into from
Jul 23, 2015

Conversation

eregon
Copy link
Member

@eregon eregon commented Jul 17, 2015

@chrisseaton @nirvdrum @pitr-ch
This is the very simple commit to remove the GIL (cleanup will be done later when merged).
I tried a few trivial parallelism problems, a parallel richards benchmark and a few benchmarks from NPB (Ruby version).

Multi-threaded programs need to be properly synchronized or they might crash in various ways (as in standard JRuby).

@eregon eregon added the truffle label Jul 17, 2015
@eregon eregon added this to the truffle-dev milestone Jul 17, 2015
@chrisseaton
Copy link
Contributor

ha when I said open a PR I thought it would be massive - but I guess you've done all the work on master and then this is just the last step.

@nirvdrum can you test your tests using this branch? I'll try all the normal benchmarks.

@headius you may be interested in this

@chrisseaton
Copy link
Contributor

I wonder if since we already have a GIL here, it would make sense to keep it as a non-default option? Are there unsynchronised multi-threaded applications that work on MRI but not on JRuby?

@eregon
Copy link
Member Author

eregon commented Jul 17, 2015

I think that's not an option as it would be unmanageable complexity to have both.

@@ -61,8 +58,7 @@ public RubyThread getRootThread() {
*/
@TruffleBoundary
public void enterGlobalLock(RubyThread thread) {
globalLock.lock();
currentThread = thread;
currentThread.set(thread);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this method be renamed if there's no longer a global lock?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that method will go away in the cleanup phase once this is merged.
It has just been a lot more practical to keep a minimal diff until then.

@pitr-ch
Copy link
Member

pitr-ch commented Jul 18, 2015

This line is worrying me https://github.com/pitr-ch/jruby/blob/master/truffle/src/main/ruby/core/shims.rb#L165-165 Not sure how many problems we'll face in practice.

@chrisseaton
Copy link
Contributor

Yeah I don't think anyone is expecting the parallelism to just work perfectly straight away and there is clearly going to be a lot of work to make it correct. We need parallelism stress tests.

Obviously we should fix things as we find them (especially this one as it might impact on performance).

@eregon
Copy link
Member Author

eregon commented Jul 19, 2015

@pitr-ch That one is very wrong, I was not aware of it. Shims should be to implement only a subset of the semantics, but not totally incorrect semantics like that one.
I agree with @chrisseaton, we cannot expect to be perfectly correct from day 1. I plan to try using JCStress tests for stress tests.

@eregon
Copy link
Member Author

eregon commented Jul 19, 2015

@pitr-ch I fixed that one.

@pitr-ch
Copy link
Member

pitr-ch commented Jul 20, 2015

Nice 👍

@chrisseaton
Copy link
Contributor

Sorry the merge is broken - can you fix and merge?

eregon added 2 commits July 23, 2015 11:05
* Still a shim as we might have troubles supporting lock/unlock
  and might change the implementation.
@eregon
Copy link
Member Author

eregon commented Jul 23, 2015

Merged, here comes the way to parallel JRuby+Truffle!

A quick experiment on some Monte Carlo pi approximation shows we scale as good as C!
(even better with hyper-threading but that is strange)
monte_carlo_pi_c_ruby_16

eregon added a commit that referenced this pull request Jul 23, 2015
[Truffle] No more global lock!
@eregon eregon merged commit 155ee96 into master Jul 23, 2015
@chrisseaton
Copy link
Contributor

even better with hyper-threading but that is strange

Probably means you are waiting on fetching from RAM most of the time. Not sure how Monte Carlo pi works but if you can try a smaller working set it might fit into cache and so not scale was well with hyper threads.

@eregon eregon deleted the truffle_rm_gil branch July 23, 2015 09:26
@enebo enebo added this to the Non-Release milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants