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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

less sync-ing with thread registration #5474

Merged
merged 9 commits into from Dec 3, 2018

Conversation

Projects
None yet
2 participants
@kares
Copy link
Member

commented Nov 27, 2018

... here's an interesting find while trying to upgrade an app stuck on JRuby 1.7.4

the app uses celluloid's actor model and under load degraded considerably (~ 10x).
with celluloid all calls upon actors are executed in fibers, which are kind of heavy under JRuby,
the system ends up operating with ~3000 threads and interestingly thread registration (those are fiber backing threads) accounts for quite some delay as its synchronizing on ThreadService.

now, that locking shouldn't be needed since the backing map is already a synchronized collection.

screenshot_025

... poked around history and there's no explicit reason for registerNewThread to lock on ThreadService
pretty much the only thing I could think of that might not work 'exactly' as before is listing threads (getActiveRubyThreads()) so I changed the order for the registration logic to write to the map last (after the thread/context gets fully initialized).

have patched this on top of of 9.1.17.0 and put under load - the 10x degradation was gone and all seems good, so far. still could use a review whether I did not miss anything.

also how would you guys feel about backporting to 9.1, assuming there's going to be a 9.1.18.0 ?
(as the 9.2 upgrade isn't 馃摋 yet and I would hate for this app to stay behind on an old 1.7)

@kares kares added this to the JRuby 9.2.5.0 milestone Nov 27, 2018

@enebo enebo requested a review from headius Nov 27, 2018

@headius
Copy link
Member

left a comment

These changes look good to me. I don't see a problem with removing the synchronization.

FWIW I did look through history to figure out when these were added, and they go way back to 2005 when I was first reworking JRuby's runtime internal. It looks like it was overcautious then too, since the state changes are similar: d2c051a

@kares

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2018

FWIW I did look through history to figure out when these were added, and they go way back to 2005 when I was first reworking JRuby's runtime internal. It looks like it was overcautious then too, since the state changes are similar: d2c051a

also dug around and find old (similar) commits, think from Nick. did hope to find smt explicit where these were introduced for a concrete reason but there seems to be nothing relevant except being cautious.
which is fine - one can always remove sync later ... just like here.

@headius

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

Go for it. And post some celluloid numbers 馃榾

@headius

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

Oh, ftr I am fine with this in 9.1 as well. 9.1.18 seems less and less likely though.

@kares kares merged commit f990c6d into jruby:master Dec 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

kares added a commit to kares/jruby that referenced this pull request Dec 3, 2018

Merge pull request jruby#5474 from kares/sync-less-threads
less sync-ing with thread registration
@kares

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2018

OK, picked this up to the 9.1 branch ... just in case there's a release 馃槇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.