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
Draft

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

Show file tree
Hide file tree
Changes from all commits
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
66 changes: 47 additions & 19 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 @@ -68,21 +68,32 @@ 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");
private static final boolean MAP_ALL =
// Whether to map the entire file contents for runtime images.
private static final boolean FULLY_MAP_RUNTIME_IMAGE =
isSystemProperty("jdk.image.map.all", "true", IS_64_BIT ? "true" : "false");

private final Path imagePath;
private final ByteOrder byteOrder;
private final String name;
// A buffer holding either the entire image file (memory mapped), or just
// the index data (copied). Only the runtime image can be fully mapped,
// but it might not be (see FULLY_MAP_RUNTIME_IMAGE).
private final ByteBuffer memoryMap;
// Whether memoryMap contains the complete file content or just the index.
private final boolean isFullyMapped;
// Channel from which file data is loaded (null if using a native buffer).
private final FileChannel channel;
// Header information parsed from the start of the image file.
private final ImageHeader header;
private final long indexSize;
// Size (bytes) of the index region at the start of the image file.
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.

// Sliced views taken from the index region of memoryMap.
private final IntBuffer redirect;
private final IntBuffer offsets;
private final ByteBuffer locations;
private final ByteBuffer strings;
private final ImageStringsReader stringsReader;
// Decompressor for resource entries.
private final Decompressor decompressor;

@SuppressWarnings({ "removal", "this-escape", "suppression" })
Expand All @@ -92,26 +103,29 @@ 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;
}

// Open the file only if no memory map yet or is 32 bit jvm
if (map != null && MAP_ALL) {
if (map != null && FULLY_MAP_RUNTIME_IMAGE) {
// No channel needed if we have the fully mapped native buffer.
channel = null;
} else {
channel = FileChannel.open(imagePath, StandardOpenOption.READ);
// No lambdas during bootstrap
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 @@ -132,35 +146,33 @@ public Void run() {
});
}

// If no memory map yet and 64 bit jvm then memory map entire file
if (MAP_ALL && map == null) {
// If no memory map yet and 64 bit jvm then memory map the runtime file.
isFullyMapped = isRuntimeImage && FULLY_MAP_RUNTIME_IMAGE;
if (map == null && isFullyMapped) {
map = channel.map(FileChannel.MapMode.READ_ONLY, 0, channel.size());
}

// 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 = copyBuffer(headerSize);
} 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 = copyBuffer(indexSize);
}

memoryMap = map.asReadOnlyBuffer();
Expand Down Expand Up @@ -402,6 +414,22 @@ private byte[] getBufferBytes(ByteBuffer buffer) {
return bytes;
}

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

@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.

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.

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

/**
* Reads entry data either from the mapped buffer, or directly from the channel.
*/
private ByteBuffer readBuffer(long offset, long size) {
if (offset < 0 || Integer.MAX_VALUE <= offset) {
throw new IndexOutOfBoundsException("Bad offset: " + offset);
Expand All @@ -413,7 +441,7 @@ private ByteBuffer readBuffer(long offset, long size) {
}
int checkedSize = (int) size;

if (MAP_ALL) {
if (isFullyMapped) {
ByteBuffer buffer = slice(memoryMap, checkedOffset, checkedSize);
buffer.order(ByteOrder.BIG_ENDIAN);

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