On Tue, 25 Nov 2025 15:14:33 GMT, David Beaumont <[email protected]> wrote:
>> Plumbing for javac flags, mostly inspired by/copied from test commits made
>> by @lahodaj .
>>
>> There are several things here, mostly entangled, so it's a bit tricky to try
>> splitting this out, but it would be possible if people wanted.
>>
>> The biggest "refactoing" part of this PR is
>> "src/jdk.compiler/share/classes/com/sun/tools/javac/file/JRTIndex.java"
>> which now has a properly controlled lifecycle and disposed its resources
>> correctly. Prior to this, the class used a non-closeable JRT file-system
>> reference, which leads to "persistent open file" issues such as JDK-8357249.
>>
>> This *does* mean that if compilation and the runtime have the same preview
>> mode, then a 2nd JRT file system to the same jimage file is "opened", *but*
>> the file system itself is lightweight, non-caching and both of them will use
>> the underlying SharedImageReader (which is where nodes are cached etc.) so
>> it really shouldn't be an issue (I will make sure javac benchmarks are
>> checked however).
>>
>> The benefit of this is that now, the shared index (which does do some
>> caching) is correctly tracked across all users, and will be closed when the
>> last user closes the lightweight wrapper instance.
>>
>> A lot of the smaller "spot fix" changes in this PR were just copied by me,
>> or at least inspired directly by Jan's work, so I may have missed some
>> semantic subtlety in the code I'm not familiar with. Please evaluate that
>> carefully.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/file/JRTIndex.java line
> 317:
>
>> 315: public void close() throws IOException {
>> 316: // Release is atomic and succeeds at most once.
>> 317: if (!sharedResources.release(this)) {
>
> This exception may be contentious. Please speak up if you don't like it.
> It's here to track any accidental double-closing of the index, and did catch
> one case already.
> Since this is an internal class it feels like we should be able to track and
> eliminate any double close events, but it is a runtime exception in complex
> code, so I'm happy to remove it or maybe replace it with logging of some kind.
> Please speak up if you have opinions.
Also, nulling out "sharedResources" during close() for better GC cleanup is
doable, but not completely trivial, since it adds "post-closure" failure modes
that didn't exist before.
I do think that *somewhere* we should be detaching the index for garbage
collection, but maybe all its users are sufficiently well scoped that it's
unlikely to matter.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1761#discussion_r2560446777
