8372515: [lworld] Plumb in javac flags for compiling with preview mode #1761
Conversation
|
👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into |
There was a problem hiding this comment.
I don't know anything about this code per se, this is all Jan's work.
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
david-beaumont
left a comment
david-beaumont
left a comment
There was a problem hiding this comment.
A couple of notes because some of this is subtle and I want reviewers to not miss some parts.
| useCtProps = javacFileManager.isDefaultBootClassPath() && javacFileManager.isSymbolFileEnabled(); | ||
| } else { | ||
| useCtProps = false; | ||
| } |
There was a problem hiding this comment.
Much simpler to reason about lifetimes if the index (wrapper) is always closeable.
I've also made JRTIndex implement Closeable, which seemed sensible, but since it's a public class I want to be sure that's a permitted thing to do in this part of the codebase.
|
|
||
| @Override | ||
| public void close() throws IOException { | ||
| if (jrtIndex != null) { |
There was a problem hiding this comment.
I made sure it's safe to call close() more than once on the same instance, which saves having to null the reference here and worry about synchronization at this level.
However, I also understand if someone wanted to clear this reference so it could be GC'd.
Please speak up if you have an opinion.
| } | ||
| } | ||
|
|
||
| private static class FileSystemResources { |
There was a problem hiding this comment.
Just moving all the outer class code into here. The actual changes are pretty small.
Mostly just the static management of the preview/non-preview versions and introduction of close semantics.
There was a problem hiding this comment.
This kind of refactoring is better done separately, making the intended change harder to review.
| boolean release(JRTIndex owner) throws IOException { | ||
| int idx = previewMode ? 1 : 0; | ||
| boolean shouldClose; | ||
| synchronized (FileSystemResources.class) { |
There was a problem hiding this comment.
Be careful to not call close() with the class synchronized.
| @Override | ||
| public void close() throws IOException { | ||
| // Release is atomic and succeeds at most once. | ||
| if (!sharedResources.release(this)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| synchronized Entry getEntry(RelativeDirectory rd) throws IOException { | ||
| if (isClosed) { |
There was a problem hiding this comment.
One other example of a new exception, not previously possible. This is rather unavoidable post-closure, and feels like IOException is the appropriate response (as opposed to the IllegalStateException proposed in close()).
| } | ||
|
|
||
| public boolean isInJRT(FileObject fo) { | ||
| if (fo instanceof PathFileObject pathFileObject) { |
There was a problem hiding this comment.
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.
| /** Get a context-specific instance of a cache. */ | ||
| public static JRTIndex instance(Context context) { | ||
| public class JRTIndex implements Closeable { | ||
| public static JRTIndex getInstance(boolean previewMode) { |
There was a problem hiding this comment.
Generally, we (core-libs) have been dropping the "get" prefix on accessors; so there's no need to change the name of the method back to an older pattern.
| } | ||
| } | ||
|
|
||
| private static class FileSystemResources { |
There was a problem hiding this comment.
This kind of refactoring is better done separately, making the intended change harder to review.
| static final CtSym EMPTY = new CtSym(false, false, null); | ||
| } | ||
|
|
||
| private final FileSystemResources sharedResources; |
There was a problem hiding this comment.
The fields of JRTIndex should be at the top of the class.
Fields, constructors, static methods, the rest.
There's some debate about where nested classes go (I say at the end).
There was a problem hiding this comment.
It is worth saying something about the values that are kept here.
It looks like a singleton but is per JRTIndex and preview.
|
|
||
| for (String option : new String[]{"--source", "--release"}) { | ||
| for (Mode mode : new Mode[]{Mode.API, Mode.CMDLINE}) { | ||
| //without preview: |
There was a problem hiding this comment.
Space after //; here and below
2 participants
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.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1761/head:pull/1761$ git checkout pull/1761Update a local copy of the PR:
$ git checkout pull/1761$ git pull https://git.openjdk.org/valhalla.git pull/1761/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1761View PR using the GUI difftool:
$ git pr show -t 1761Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1761.diff
Using Webrev
Link to Webrev Comment