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

review thread sync and clear references #5506

Merged
merged 8 commits into from Dec 15, 2018

Conversation

Projects
None yet
2 participants
@kares
Copy link
Member

kares commented Dec 11, 2018

  • a loop-less runtime.getCurrentContext
  • do not re-create a (context) soft-ref as threads get finalized (unregistered)
  • re-use un-registering logic (clearing refs) with fiber thread disposal
  • thread locals synchronization does not need to lock the RubyThread

the initial motivation was to avoid some soft-references as with tons of fibers they won't get collected until a full gc is run. might still try to look into that direction, these changes should be conservative and maybe help clearing some of those refs (as fibers are done).

kares added some commits Dec 10, 2018

setting a null context should not create a weak-ref
also clear out ref when setting null (happens on thread dispose)
[refactor] clear context-thread binding when un-registering
this piece gets invoked on RubyThread#dispose (finalize)
thus shall not attempt to adopt thread with getCurrentContext
... at the point this gets invoked refs should be null

@kares kares added this to the JRuby 9.2.6.0 milestone Dec 11, 2018

@headius
Copy link
Member

headius left a comment

Seems generally ok but I had a few questions.

@kares kares force-pushed the kares:proper-thread-sync branch from b1544cf to 47a9e5e Dec 13, 2018

@kares kares force-pushed the kares:proper-thread-sync branch from 47a9e5e to 67aab5d Dec 15, 2018

kares added some commits Dec 10, 2018

[refactor] do (same) thread unregistering from fiber-threads
unifies the un-registering logic for normal threads vs fibers

also cache volatile data in a local (its assigned on initialize)

@kares kares merged commit 0a804cb into jruby:master Dec 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.