8357249: Compiler task keeps --system files open by david-beaumont · Pull Request #2374 · openjdk/valhalla

david-beaumont · 2026-04-28T16:49:07Z

Draft request for discussion purposes only...

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8357249: Compiler task keeps --system files open (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2374/head:pull/2374
$ git checkout pull/2374

Update a local copy of the PR:
$ git checkout pull/2374
$ git pull https://git.openjdk.org/valhalla.git pull/2374/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2374

View PR using the GUI difftool:
$ git pr show -t 2374

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2374.diff

bridgekeeper · 2026-04-28T16:51:38Z

👋 Welcome back dbeaumont! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-04-28T16:52:32Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2026-04-28T16:53:17Z

@david-beaumont this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK_8357249
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

david-beaumont

Discussion points and notes added.

david-beaumont · 2026-04-28T16:50:31Z

            isSystemProperty("sun.arch.data.model", "64", "32");
    private static final boolean USE_JVM_MAP =
            isSystemProperty("jdk.image.use.jvm.map", "true", "true");
+    // Whether to map the entire file contents.


It took me ages to realise this isn't "map all image files that are opened", but rather "map an entire image file if it is being mapped at all"

david-beaumont · 2026-04-28T16:51:11Z

    private final FileChannel channel;
    private final ImageHeader header;
-    private final long indexSize;
+    private final int indexSize;


Just in passing since it's not a long from the getter.

david-beaumont · 2026-04-28T16:51:32Z

        this.name = this.imagePath.toString();

+        // Only the runtime image is loaded with the root class-loader.
+        final boolean isRuntimeImage = BasicImageReader.class.getClassLoader() == null;


Semantic readability/intent.

david-beaumont · 2026-04-28T16:53:43Z

+            if (isRuntimeImage) {
+                map = channel.map(FileChannel.MapMode.READ_ONLY, 0, channel.size());
+            } else {
+                // Non-runtime images are used for compiler tasks, and we avoid


This is what fixes the non-runtime jimage closing issue.
It's nasty because it goes from memory mapped file to 100M of copied data.

An alternative (with performance cost) would be to do only the "partial" loading of the buffer, say up to the index, and then fetch file content with normal file reads. But this is all open to much discussion.

The changes look right but "Non-runtime images" is a not great phase. You could speak of the current run-time image vs. a target run-time image. That way the comments are clear - use memory-mapping for the current run-time image, regular file I/O for other run-time images.

david-beaumont · 2026-04-28T16:54:04Z

+                if ((long) imageFileSize != channel.size()) {
+                    throw new IOException("\"" + name + "\" too large");
+                }
+                map = readDirectBuffer(channel, imageFileSize, name);


Pulled out a little helper since it's used in 3 places.

david-beaumont · 2026-04-28T16:58:56Z

-
-    private volatile boolean closed;
+    // Set to null when closed to allow GC of underlying buffers to unmap files.
+    private final AtomicReference<SharedImageReader> reader;


This class is the place where we've already got lifetime semantics.
By letting the reader be cleared, we can replace the closed flag and retain non-locking patterns of use.
The benefit is that now the underlying reader becomes GC-able when the ImageReader is closed.

While this may not be of benefit when using direct buffers, it does let any mapped buffers get GC-ed sooner, which is required for closing the underlying memory mapped file.

I think this change is reasonable in general and just improves the semantics of the way this class's lifetime is managed wrt GC.

david-beaumont · 2026-04-28T17:00:42Z

            isOpen = false;
            image.close();
            image = null;
+


This is the change that lets the JAR file be closed. I hate it!
However it shows that there's potential for passing a "closer" or some kind into the JrtFileSystem from the provider to close the underling JarLoader (which is the actual thing that needs closing).

More to look at here, but this is proof of concept at least.

david-beaumont · 2026-04-28T17:02:54Z

            useCtProps = false;
        }
-        if (useCtProps && JRTIndex.isAvailable()) {
+        JRTIndex index = null;


Removes need for JRTIndex.isAvailable() which opened the uncloseable image in order to do its test.
We can just try and open the thing we want to open instead.
This should be identical in behaviour to what's there now and is necessary for preventing the non-runtime image being held open forever.

david-beaumont · 2026-04-28T17:03:14Z

        }
    }

-    /** {@return whether the JRT file-system is available to create an index} */


Removed just to prove it's not needed anywhere else.

I'm pretty sure this dates back to before the jrt file system worked with exploded builds.

AlanBateman · 2026-04-29T07:10:50Z

        }
    }

-    /** {@return whether the JRT file-system is available to create an index} */


I'm pretty sure this dates back to before the jrt file system worked with exploded builds.

AlanBateman · 2026-04-29T07:30:01Z

+            ClassLoader cl = provider.getClass().getClassLoader();
+            if (cl instanceof URLClassLoader) {
+                ((URLClassLoader) cl).close();
+            }


JrtFileSystemProvider creates the JrtFsLoader/URLClassLoader and that might be a better place to close it. A postClose or some other callback to JrtFileSystemProvider could do this. It's just a suggestion to encapsulate everything about the custom and closeable class loader in one place.

I tried looking at this, but there's no nice way to pass the classloader, or some delegate callback across the boundary (since you can't easily add a new API because you might be talking to old versions of the code).
Unless you're will to pass the class-loader in the env map, but that's pretty nasty.
I do really want to neaten this up, but might not get time.

Okay, I was thinking something simple like provider.afterClose(this) so it's local to the jrtfs implementation (so version mismatch issues). We can look at it another time.

AlanBateman · 2026-04-29T07:32:19Z

+            if (isRuntimeImage) {
+                map = channel.map(FileChannel.MapMode.READ_ONLY, 0, channel.size());
+            } else {
+                // Non-runtime images are used for compiler tasks, and we avoid


The changes look right but "Non-runtime images" is a not great phase. You could speak of the current run-time image vs. a target run-time image. That way the comments are clear - use memory-mapping for the current run-time image, regular file I/O for other run-time images.

AlanBateman · 2026-04-29T12:41:01Z

+        }
+        map.rewind();
+        return map;
+    }


I assume this doesn't really need to be a direct buffer, it will work equally well with a heap buffer.

In terms of naming, "readDirectBuffer" doesn't "read a direct buffer". It can be just "read". Same thing for the pre-existing "readBuffer".

As regards the != size check. A short read is possible, meaning it would be valid for read to return a value < size.

AlanBateman

Are you going to resolve the merge conflict and make the PR ready for review?

AlanBateman · 2026-04-29T18:58:01Z

+    /**
+     * Reads the image file to return a newly allocated buffer.
+     */
+    private ByteBuffer copyBuffer(int size)


This reads "size" bytes from the start of the file into a buffer, and returns it. So something like readNBytes is a more appropriate name.

david-beaumont · 2026-04-30T13:35:02Z

I'm not necessarily happy with this as it stands. It's more proof of concept.
I also don't like it being on lworld.
Once my preview mode changes are in, some/all of these changes can/should go directly into mainline.

minimal but hacky changes for letting files be closed

0451e5a

openjdk Bot added the merge-conflict Pull request has merge conflict with target branch label Apr 28, 2026

david-beaumont commented Apr 28, 2026

View reviewed changes

AlanBateman reviewed Apr 29, 2026

View reviewed changes

avoid copying complete buffer and tidy code

ba59470

AlanBateman reviewed Apr 29, 2026

View reviewed changes

david-beaumont mentioned this pull request Apr 30, 2026

8357249: Compiler task keeps --system files open #2377

Draft

4 tasks

Conversation

david-beaumont commented Apr 28, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper Bot commented Apr 28, 2026

Uh oh!

openjdk Bot commented Apr 28, 2026

Uh oh!

openjdk Bot commented Apr 28, 2026

Uh oh!

david-beaumont left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-beaumont commented Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

david-beaumont commented Apr 28, 2026 •

edited by openjdk Bot

Loading

AlanBateman Apr 29, 2026 •

edited

Loading

AlanBateman Apr 29, 2026 •

edited

Loading

AlanBateman Apr 29, 2026 •

edited

Loading