less sync-ing with thread registration#5474
Conversation
(thread) context is per-thread and its mostly about setting it up while the local-context field is a thread-local instance
... rest of unregister code is dealing with a thread-local
headius
left a comment
headius
left a comment
There was a problem hiding this 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
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. |
|
Go for it. And post some celluloid numbers 😀 |
|
Oh, ftr I am fine with this in 9.1 as well. 9.1.18 seems less and less likely though. |
less sync-ing with thread registration
|
OK, picked this up to the 9.1 branch ... just in case there's a release 😈 |
2 participants
... 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.
... poked around history and there's no explicit reason for
registerNewThreadto lock onThreadServicepretty 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)