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

Use a Guava Cache instead of a ThreadLocal #1862

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

octylFractal
Copy link
Member

This allows high performance without leaking memory, and works around the JVM bug with ThreadLocals. Fixes #1722.

This allows high performance without leaking memory, and works around
the JVM bug with ThreadLocals. See #1722.
@octylFractal octylFractal added type:bug Incorrect behavior, not working as intended status:accepted Will be fixed / added to WorldEdit, eventually labels Aug 2, 2021
@octylFractal octylFractal added this to the 7.2.6 milestone Aug 2, 2021
@octylFractal octylFractal requested a review from a team August 2, 2021 01:24
@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #1862 (1169ff4) into version/7.2.x (96c9799) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##             version/7.2.x    #1862      +/-   ##
===================================================
- Coverage            12.27%   12.27%   -0.01%     
  Complexity            1089     1089              
===================================================
  Files                  812      812              
  Lines                33278    33281       +3     
  Branches              3840     3840              
===================================================
  Hits                  4086     4086              
- Misses               28994    28997       +3     
  Partials               198      198              
Impacted Files Coverage Δ
...a/com/sk89q/worldedit/session/request/Request.java 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96c9799...1169ff4. Read the comment docs.

@octylFractal octylFractal merged commit b9f0416 into version/7.2.x Aug 2, 2021
@octylFractal octylFractal deleted the bugfix/shop-non-local branch August 2, 2021 17:21
@TomyLobo
Copy link
Collaborator

TomyLobo commented Aug 4, 2021

hmm, this JDK bug has been closed as "not a java bug", with reference to a hadoop bug.
what they say in that bug thread is that weak references dont get cleaned up reliably with G1GC.
You told that cache to use weakly referenced keys that we're now relying on in order to remove entries for the dead threads from the cache.

So I don't know how deep you looked into this, but at a surface level this looks like it might have made it worse, not better, but I'm not sure.

@me4502
Copy link
Member

me4502 commented Aug 4, 2021

I'm under the impression that one of the main reasons it was closed was because they had no reliable reproduction environment and therefore no way to confirm it was a Java bug? Either way - if a part of Java depends on weak reference cleanups, and the default GC doesn't handle that reliably, that's a Java bug.

@octylFractal
Copy link
Member Author

The main difference is that we are using weakly-referenced Thread objects, rather than ThreadLocal objects -- we expect there to be usually only a handful of these Threads (main and WorldEdit's processing pools), and for them to be long-lived. They should rarely need to be GC'd, and even then Guava's Cache employs reference queues to more eagerly clean-up these weak references. I think this is a safe hold-over solution.

gtosh4 added a commit to GTNewHorizons/worldedit-gtnh that referenced this pull request Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:accepted Will be fixed / added to WorldEdit, eventually type:bug Incorrect behavior, not working as intended
Projects
Development

Successfully merging this pull request may close these issues.

4 participants