Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
david-beaumont wants to merge 2 commits into
base: lworld
Choose a base branch
from
+97 −64
Draft

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0451e5a
minimal but hacky changes for letting files be closed
david-beaumont Apr 28, 2026
ba59470
avoid copying complete buffer and tidy code
david-beaumont Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 40 additions & 15 deletions src/java.base/share/classes/jdk/internal/jimage/BasicImageReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.nio.IntBuffer;
import java.nio.MappedByteBuffer;
import java.nio.channels.FileChannel;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
Expand Down Expand Up @@ -68,6 +69,7 @@ public Boolean run() {
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.

@david-beaumont david-beaumont Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

private static final boolean MAP_ALL =
isSystemProperty("jdk.image.map.all", "true", IS_64_BIT ? "true" : "false");

Expand All @@ -77,7 +79,7 @@ public Boolean run() {
private final ByteBuffer memoryMap;
private final FileChannel channel;
private final ImageHeader header;
private final long indexSize;
private final int indexSize;

@david-beaumont david-beaumont Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private final IntBuffer redirect;
private final IntBuffer offsets;
private final ByteBuffer locations;
Expand All @@ -92,13 +94,15 @@ protected BasicImageReader(Path path, ByteOrder byteOrder)
this.byteOrder = Objects.requireNonNull(byteOrder);
this.name = this.imagePath.toString();

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

@david-beaumont david-beaumont Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantic readability/intent.

ByteBuffer map;

if (USE_JVM_MAP && BasicImageReader.class.getClassLoader() == null) {
if (USE_JVM_MAP && isRuntimeImage) {
// Check to see if the jvm has opened the file using libjimage
// native entry when loading the image for this runtime
map = NativeImageBuffer.getNativeMap(name);
} else {
} else {
map = null;
}

Expand All @@ -111,7 +115,7 @@ protected BasicImageReader(Path path, ByteOrder byteOrder)
AccessController.doPrivileged(new PrivilegedAction<Void>() {
@Override
public Void run() {
if (BasicImageReader.class.getClassLoader() == null) {
if (isRuntimeImage) {
try {
Class<?> fileChannelImpl =
Class.forName("sun.nio.ch.FileChannelImpl");
Expand All @@ -134,33 +138,44 @@ public Void run() {

// If no memory map yet and 64 bit jvm then memory map entire file
if (MAP_ALL && map == null) {
map = channel.map(FileChannel.MapMode.READ_ONLY, 0, channel.size());
if (isRuntimeImage) {
map = channel.map(FileChannel.MapMode.READ_ONLY, 0, channel.size());
} else {
// Non-runtime images are used for compiler tasks, and we avoid

@david-beaumont david-beaumont Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@AlanBateman AlanBateman Apr 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// memory mapping them so the file handles can be closed when the task
// completes (rather than having to wait for garbage collection).
// This is important when long-lived server processes compile code
// across multiple runtime images. Once this code no longer requires
// JDK-8, we might be able to switch to using MemorySegment here.
int imageFileSize = (int) channel.size();
if ((long) imageFileSize != channel.size()) {
throw new IOException("\"" + name + "\" too large");
}
map = readDirectBuffer(channel, imageFileSize, name);

@david-beaumont david-beaumont Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

// Assume we have a memory map to read image file header
// headerBuffer is a temporary buffer for reading the heading (which
// may be the full buffer, or a scratch buffer we discard later).
ByteBuffer headerBuffer = map;
int headerSize = ImageHeader.getHeaderSize();

// If no memory map then read header from image file
if (headerBuffer == null) {
headerBuffer = ByteBuffer.allocateDirect(headerSize);
if (channel.read(headerBuffer, 0L) == headerSize) {
headerBuffer.rewind();
} else {
throw new IOException("\"" + name + "\" is not an image file");
}
headerBuffer = readDirectBuffer(channel, headerSize, name);
} else if (headerBuffer.capacity() < headerSize) {
// A fully mapped file cannot be smaller than the header size.
throw new IOException("\"" + name + "\" is not an image file");
}

// Interpret the image file header
// Read the header and find the index size, which is the minimum buffer
// size we need to read if we haven't already mapped the entire file.
header = readHeader(intBuffer(headerBuffer, 0, headerSize));
indexSize = header.getIndexSize();

// If no memory map yet then must be 32 bit jvm not previously mapped
if (map == null) {
// Just map the image index
map = channel.map(FileChannel.MapMode.READ_ONLY, 0, indexSize);
map = readDirectBuffer(channel, indexSize, name);
}

memoryMap = map.asReadOnlyBuffer();
Expand All @@ -178,6 +193,16 @@ public Void run() {
decompressor = new Decompressor();
}

private static ByteBuffer readDirectBuffer(FileChannel channel, int size, String name)
throws IOException {
ByteBuffer map = ByteBuffer.allocateDirect(size);
if (channel.read(map, 0L) != size) {
throw new IOException("\"" + name + "\" is not an image file");
}
map.rewind();
return map;
}

@AlanBateman AlanBateman Apr 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


protected BasicImageReader(Path imagePath) throws IOException {
this(imagePath, ByteOrder.nativeOrder());
}
Expand Down
52 changes: 26 additions & 26 deletions src/java.base/share/classes/jdk/internal/jimage/ImageReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import jdk.internal.jimage.ImageLocation.LocationType;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.nio.IntBuffer;
Expand All @@ -44,6 +45,7 @@
import java.util.Objects;
import java.util.Set;
import java.util.TreeMap;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Stream;
Expand Down Expand Up @@ -89,12 +91,11 @@
* to the jimage file provided by the shipped JDK by tools running on JDK 8.
*/
public final class ImageReader implements AutoCloseable {
private final SharedImageReader reader;

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

@david-beaumont david-beaumont Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private ImageReader(SharedImageReader reader) {
this.reader = reader;
this.reader = new AtomicReference<>(reader);
}

/**
Expand Down Expand Up @@ -123,23 +124,19 @@ public static ImageReader open(Path imagePath, ByteOrder byteOrder, PreviewMode

@Override
public void close() throws IOException {
if (closed) {
SharedImageReader r = reader.getAndSet(null);
if (r == null) {
throw new IOException("image file already closed");
}
reader.close(this);
closed = true;
r.close(this);
}

private void ensureOpen() throws IOException {
if (closed) {
private SharedImageReader ensureOpen() throws IOException {
SharedImageReader r = reader.get();
if (r == null) {
throw new IOException("image file closed");
}
}

private void requireOpen() {
if (closed) {
throw new IllegalStateException("image file closed");
}
return r;
}

/**
Expand All @@ -150,8 +147,7 @@ private void requireOpen() {
* @return a node representing a resource, directory or symbolic link.
*/
public Node findNode(String name) throws IOException {
ensureOpen();
return reader.findNode(name);
return ensureOpen().findNode(name);
}

/**
Expand All @@ -170,8 +166,7 @@ public Node findNode(String name) throws IOException {
*/
public Node findResourceNode(String moduleName, String resourcePath)
throws IOException {
ensureOpen();
return reader.findResourceNode(moduleName, resourcePath);
return ensureOpen().findResourceNode(moduleName, resourcePath);
}

/**
Expand All @@ -189,8 +184,7 @@ public Node findResourceNode(String moduleName, String resourcePath)
*/
public boolean containsResource(String moduleName, String resourcePath)
throws IOException {
ensureOpen();
return reader.containsResource(moduleName, resourcePath);
return ensureOpen().containsResource(moduleName, resourcePath);
}

/**
Expand All @@ -202,24 +196,30 @@ public boolean containsResource(String moduleName, String resourcePath)
* given node is not a resource node).
*/
public byte[] getResource(Node node) throws IOException {
ensureOpen();
return reader.getResource(node);
return ensureOpen().getResource(node);
}

/**
* Returns the content of a resource node in a newly allocated byte buffer.
*/
public ByteBuffer getResourceBuffer(Node node) {
requireOpen();
if (!node.isResource()) {
throw new IllegalArgumentException("Not a resource node: " + node);
}
return reader.getResourceBuffer(node.getLocation());
try {
return ensureOpen().getResourceBuffer(node.getLocation());
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}

// Package protected for use only by SystemImageReader.
ResourceEntries getResourceEntries() {
return reader.getResourceEntries();
try {
return ensureOpen().getResourceEntries();
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}

private static final class SharedImageReader extends BasicImageReader {
Expand Down
9 changes: 8 additions & 1 deletion src/java.base/share/classes/jdk/internal/jrtfs/JrtFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014, 2025, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2026, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -28,6 +28,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.URLClassLoader;
import java.nio.ByteBuffer;
import java.nio.channels.Channels;
import java.nio.channels.FileChannel;
Expand Down Expand Up @@ -313,6 +314,12 @@ synchronized void cleanup() throws IOException {
isOpen = false;
image.close();
image = null;

@david-beaumont david-beaumont Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Closes the jrt-fs.jar !!!
ClassLoader cl = provider.getClass().getClassLoader();
if (cl instanceof URLClassLoader) {
((URLClassLoader) cl).close();
}

@AlanBateman AlanBateman Apr 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@david-beaumont david-beaumont Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@AlanBateman AlanBateman Apr 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand Down
22 changes: 15 additions & 7 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/code/ClassFinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2025, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 2026, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -26,7 +26,9 @@
package com.sun.tools.javac.code;

import java.io.IOException;
import java.nio.file.FileSystemNotFoundException;
import java.nio.file.Path;
import java.nio.file.ProviderNotFoundException;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Iterator;
Expand Down Expand Up @@ -217,14 +219,20 @@ protected ClassFinder(Context context) {
} else {
useCtProps = false;
}
if (useCtProps && JRTIndex.isAvailable()) {
JRTIndex index = null;

@david-beaumont david-beaumont Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (useCtProps) {
Preview preview = Preview.instance(context);
JavaCompiler comp = JavaCompiler.instance(context);
jrtIndex = JRTIndex.instance(preview.isEnabled());
comp.closeables = comp.closeables.prepend(jrtIndex);
} else {
jrtIndex = null;
try {
index = JRTIndex.instance(preview.isEnabled());
} catch (ProviderNotFoundException | FileSystemNotFoundException e) {
// Leave index null.
}
if (index != null) {
JavaCompiler comp = JavaCompiler.instance(context);
comp.closeables = comp.closeables.prepend(index);
}
}
jrtIndex = index;

profile = Profile.instance(context);
cachedCompletionFailure = new CompletionFailure(null, () -> null, dcfh);
Expand Down
12 changes: 1 addition & 11 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/file/JRTIndex.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014, 2025, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2026, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -98,16 +98,6 @@ public static JRTIndex instance(boolean previewMode) {
}
}

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

@david-beaumont david-beaumont Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@AlanBateman AlanBateman Apr 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public static boolean isAvailable() {
try {
FileSystems.getFileSystem(URI.create("jrt:/"));
return true;
} catch (ProviderNotFoundException | FileSystemNotFoundException e) {
return false;
}
}

/**
* Underlying file system resources potentially shared between many indexes.
*
Expand Down
Loading