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

[performance] Avoid reading SourceFile twice #3030

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Sep 30, 2024

During compile parsing happens in two stages:

  1. diet parse (any blocks like method bodies are skipped)
  2. parse bodies Both phases did read the source .java file from file system. With this change the file contents is kept in CompilationResult.contentRef until no longer needed. It is cached in
    a SoftReference to avoid OutOfMemoryError.

#2691

@HannesWell
Copy link
Contributor

2. It is cached in
a SoftReference to avoid OutOfMemoryError.

Since I see you using SoftReference in more and more places I want to add a word of warning:
While the idea is tempting, using a lot of SoftReferences can cause serious performance problems. Depending on their count, the memory they retain, the JVM implementation respectively the GC in use and it's settings, it might happen that the GC is in constant 'panic' and trying to free memory but only a little and blocking the entire application with that.

See for example from this article Weak, Soft, and Phantom References in Java:

You need to keep in mind that filling almost all your memory can slow down your program so much that a cache hardly matters.
It’s easy to verify this just by running the program and uncommenting the line that creates the SoftReference.

Or for example this comment were I learned this myself: google/guava#5311 (comment)

So if SoftReferences are used (a lot), because all this is fuzzy, actually sophisticated benchmarks with different loads, GCs and CG-settings should be done to ensure there is an overall net benefit in using them.
Of course it would be ideal if they are simply avoided by a different logic or program flow, but I have no clue if this is possible.

During compile parsing happens in two stages:
1. diet parse (any blocks like method bodies are skipped)
2. parse bodies
Both phases did read the source .java file from file system. With this
change the file contents is kept in CompilationResult.contentRef until
no longer needed. It is cached in
a SoftReference to avoid OutOfMemoryError.

eclipse-jdt#2691
@jukzi jukzi force-pushed the CompilationResult.contentRef branch from 6468536 to 8f5a2bf Compare October 1, 2024 12:08
@jukzi
Copy link
Contributor Author

jukzi commented Oct 1, 2024

Depending on their count, the memory they retain

therefore do measurements, measurements and measurements. Like in all good science.

@stephan-herrmann i think i adapted the code as you demanded and i like the CompilationResult way. Do you want to review? Please just leave a message if you need more time.

Merge conflicts solved - CompilationResult had javadoc above the imports, which could not be automatically solved.

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.

3 participants