On Tue, 25 Nov 2025 15:57:56 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.
>
> David Beaumont has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove note about StableValue (not possible)
src/jdk.compiler/share/classes/com/sun/tools/javac/file/JRTIndex.java line 341:
> 339:
> 340: public boolean isInJRT(FileObject fo) {
> 341: if (fo instanceof PathFileObject pathFileObject) {
This is surprisingly subtle and needs someone much more familiar with javac,
FileObject and other things to look at. This method is used exactly once in
ClassFinder to see if the path of a FileObject "came from" the JRT index.
Previously any FileObject created with a path that come from the global jrt
file system would cause this to return "true". Now, it might not, because
preview mode and non-preview mode no longer share a file system instance.
This feels correct in the sense that "the two classes could have different
content in different modes", but it's bad if anyone wants flags for a class
that wasn't changed by preview mode. In that case, the code will now fail to do
anything (whereas before it would):
--------
long getSupplementaryFlags(ClassSymbol c) {
if (jrtIndex == null || !jrtIndex.isInJRT(c.classfile) || c.name ==
names.module_info) {
return 0;
}
...
}
--------
So it's not at all clear to me if this is an issue or not. If ClassSymbols for
classes in the JRT image which get passed to getSupplementaryFlags() always
come from **this index instance**, then it should be okay, but otherwise it's
subtly broken. It depends how people can define ClassSymbol instances, and what
control they have over the paths of the file objects they use.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1761#discussion_r2560775156
