[performance] Avoid reading SourceFile twice #2910
Conversation
jukzi
force-pushed
the
SourceFile
branch
2 times, most recently
from
87e11f3
to
e2f4ffc
Compare
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 until no longer needed. It is cached in a SoftReference to avoid OutOfMemoryError. eclipse-jdt#2691
|
@stephan-herrmann could you review, please? |
My focus is still on shipping complete support for Java 23. Any other reviews will have to wait until after 17 Sept. |
There was a problem hiding this comment.
This solution looks fine, but I still wonder if SourceFile is the best location for this cache.
Given the large number of clients of this class, and many of them calling getContents(), it is difficult to see what will be the effect for clients other than Compiler.
- Do any of those read the same source more than once?
- It would be difficult to define clean-up protocol for each of them.
- hence we need to ask: is holding unused
SoftReferences for free, or does it put some kind of pressure on the garbage collector?
- hence we need to ask: is holding unused
Since compiler always accesses the SourceFile via CompilationResult, perhaps the latter could be made responsible for caching (encapsulated as CompilationResult.getContents()). And then cleanup could be hooked into the existing CompilationUnitDeclaration.cleanUp(), to the advantage that we don't add a new protocol to be observed.
| ICompilationUnit sourceUnit = sourceUnits[i]; | ||
| if (this.options.verbose) { | ||
| this.out.println( | ||
| Messages.bind(Messages.compilation_request, | ||
| new String[] { | ||
| String.valueOf(i + 1), | ||
| String.valueOf(maxUnits), | ||
| new String(sourceUnits[i].getFileName()) | ||
| new String(sourceUnit.getFileName()) | ||
| })); | ||
| } | ||
| // diet parsing for large collection of units | ||
| CompilationUnitDeclaration parsedUnit; | ||
| unitResult = new CompilationResult(sourceUnits[i], i, maxUnits, this.options.maxProblemsPerUnit); | ||
| unitResult = new CompilationResult(sourceUnit, i, maxUnits, this.options.maxProblemsPerUnit); | ||
| long parseStart = System.currentTimeMillis(); | ||
| if (this.totalUnits < this.parseThreshold) { | ||
| parsedUnit = this.parser.parse(sourceUnits[i], unitResult); | ||
| parsedUnit = this.parser.parse(sourceUnit, unitResult); | ||
| } else { | ||
| parsedUnit = this.parser.dietParse(sourceUnits[i], unitResult); | ||
| parsedUnit = this.parser.dietParse(sourceUnit, unitResult); | ||
| } | ||
| long resolveStart = System.currentTimeMillis(); | ||
| this.stats.parseTime += resolveStart - parseStart; | ||
| // initial type binding creation | ||
| this.lookupEnvironment.buildTypeBindings(parsedUnit, null /*no access restriction*/); | ||
| this.stats.resolveTime += System.currentTimeMillis() - resolveStart; | ||
| addCompilationUnit(sourceUnits[i], parsedUnit); | ||
| addCompilationUnit(sourceUnit, parsedUnit); |
There was a problem hiding this comment.
why is this method changed?
please see #3030 for an alternate version based on CompilationResult |
During compile parsing happens in two stages:
#2691