Re: RFR: 8320353: Reenable stringop-overflow warnings
On Tue, 1 Jul 2025 12:25:04 GMT, Anton Artemov wrote: > Hi, please consider the following changes: > > this PR addresses the issue of stringop-overflow warnings produced by GCC. > The compiler does think that the thread pointer returned by > `JavaThread::current()` can be null, though it cant. The thread pointer ends > up being an argument in `__atomic_load`, and the compiler reports the warning > related to argument of that method. > > The patch adds a hint to the compiler by means of `__builtin_unreachable()` > intrinsic, which tells the compiler that certain piece of code will never be > reached (case of thread pointer being null). This solves the issue. > > Tested in tiers 1-3 and GHA. Sorry but I really dislike seeing this compiler-specific pollution in shared code. It is even worse that you have to put it in two places (what is so special about the jvmci code to require this?) just because gcc is "too dumb" keep track of things. Also IIUC from JBS the problem was only seen building Zero, so maybe we can do something there that is Zero specific? Sorry. - PR Review: https://git.openjdk.org/jdk/pull/26067#pullrequestreview-2977963403
Re: RFR: 8361076: Add benchmark for ImageReader in preparation for Valhalla changes [v3]
On Tue, 1 Jul 2025 07:25:35 GMT, Alan Bateman wrote: >> David Beaumont has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Some tidying up relating to the state objects. > > test/micro/org/openjdk/bench/jdk/internal/jrtfs/ImageReaderBenchmark.java > line 230: > >> 228: // Created by running "java -verbose:class", throwing away >> anonymous inner >> 229: // classes and anything without a reliable name, and grouping by >> the stated >> 230: // source. It's not perfect, but it's representative. > > I don't think this is maintainable. How useful (or not) is this benchmark if > the names of all the internal classes (that will change from release to > release) are dropped from this? Debatable. It's obviously going to scale any results somewhat based on the size of the resources and number of classes. It's kind nice to see "this change removes at least N micro/milli seconds of time spent" since that's a minimum set of classes we expect to always be needed, so any time saved is a lower bound. I'd say maybe leave it as is for now with a note saying "if this keeps breaking, make the list less fragile". I'm also assuming this is only run manually, and not a part of any CI pipeline ... so please let me know if I'm wrong about that. - PR Review Comment: https://git.openjdk.org/jdk/pull/26044#discussion_r2178080151
RFR: 8320353: Reenable stringop-overflow warnings
Hi, please consider the following changes: this PR addresses the issue of stringop-overflow warnings produced by GCC. The compiler does think that the thread pointer returned by `JavaThread::current()` can be null, though it cant. The thread pointer ends up being an argument in `__atomic_load`, and the compiler reports the warning related to argument of that method. The patch adds a hint to the compiler by means of `__builtin_unreachable()` intrinsic, which tells the compiler that certain piece of code will never be reached (case of thread pointer being null). This solves the issue. Tested in tiers 1-3 and GHA. - Commit messages: - 8320353: Fixed comment - 8320353: Fixed whitespace error - 8320353: Fiixed build problem - 8320353: Added debug only case - 8320353: More work - 8320353: More work - 8320353: Reenabled stringop-overflow warning for linux zero/fastdebug Changes: https://git.openjdk.org/jdk/pull/26067/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=26067&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8320353 Stats: 25 lines in 3 files changed: 20 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/26067.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26067/head:pull/26067 PR: https://git.openjdk.org/jdk/pull/26067
RFR: 8357979: Compile jdk.internal.vm.ci targeting the Boot JDK version
>From the bug description: There are plans to build libgraal in JDK master using a version of Native Image running on a JDK one version behind JDK master. This Native Image execution needs to be able to load the JVMCI classes as they are built into the libgraal image. As such, the JVMCI classes must have a class file major version of N-1 where N is the major class file version of JDK master. This PR is basically the same change as published by Doug in https://github.com/openjdk/jdk/pull/25494, but it contains some cleanup and adds and uses the `TARGET_RELEASE` argument that Doug correctly realized was needed. - Commit messages: - 8357979: Compile jdk.internal.vm.ci targeting the Boot JDK version Changes: https://git.openjdk.org/jdk/pull/26069/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=26069&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8357979 Stats: 12 lines in 5 files changed: 4 ins; 3 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/26069.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26069/head:pull/26069 PR: https://git.openjdk.org/jdk/pull/26069
Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v15]
> This proposed change would move the native objects required for NIO file > interaction from the libnio native library to the libjava native library on > Linux, macOS, and Windows. Brian Burkhalter has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - Merge - Merge - Merge - Merge - Merge - Merge - Merge - 8337143: Minor makefile tweak - 8337143: Clean up to address reviewer comments - Merge - ... and 5 more: https://git.openjdk.org/jdk/compare/9d518b32...ffa3c469 - Changes: https://git.openjdk.org/jdk/pull/20317/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20317&range=14 Stats: 1539 lines in 93 files changed: 774 ins; 668 del; 97 mod Patch: https://git.openjdk.org/jdk/pull/20317.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20317/head:pull/20317 PR: https://git.openjdk.org/jdk/pull/20317
Re: RFR: 8360037: Refactor ImageReader in preparation for Valhalla support
On Mon, 30 Jun 2025 23:27:49 GMT, David Beaumont wrote:
> Refactoring `ImageReader` to make it easy to add preview mode functionality
> for Valhalla.
>
> This PR is a large change to `ImageReader` (effectively a rewrite) but
> reduces the surface area of the API significantly, reduces code complexity
> and increases performance/memory efficiency. The need for this sort of change
> comes from the need to support "preview mode" in Valhalla for classes loaded
> from core modules.
>
> ### Preview Mode
>
> In the proposed preview mode support for Valhalla, a resource with the name
> `/modules//` may come from one of two locations in the
> underlying jimage file; `//` or
> `//META-INF/preview/`. Furthermore, directories (represented as
> directory nodes via the API) will have a potentially different number of
> child nodes depending on whether preview mode is in effect, and some
> directories may only exist at all in preview mode.
>
> Furthermore, the directories and symbolic link nodes in `/packages/...` will
> also be affected by the existence of new package directories. To retain
> consistency and avoid "surprises" later, all of this needs to be taken into
> account.
>
> Below is a summary of the main changes for mainline JDK to better support
> preview mode later:
>
> ### 1: Improved encapsulation for preview mode
>
> The current `ImageReader` code exposes the data from the jimage file in
> several ways; via the `Node` abstraction, but also with methods which return
> an `ImageLocation` instance directly. In preview mode it would not be
> acceptable to return the `ImageLocation`, since that would leak the existence
> of resources in the `META-INF/preview` namespace and lets users see resource
> nodes with names that don't match the underlying `ImageLocation` from which
> their contents come.
>
> As such, the PR removes all methods which can leak `ImageLocation` or other
> "underlying" semantics about the jimage file. Luckily most of these are
> either used minimally and easily migrated to using nodes, or they were not
> being used at all. This PR also removes any methods which were already unused
> across the OpenJDK codebase (if I discover any issues with over-trimming of
> the API during full CI testing, it will be easy to address).
>
> ### 2. Simplification of directory child node handling
>
> The current `ImageReader` code attempts to create parent directories "on
> demand" for any child nodes it creates. This results in parent directories
> having a non-empty but incomplete set of child nodes present. This is
> referred to as the directory being "incomple...
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
456:
> 454: Optional node =
> Optional.ofNullable(reader.findNode(nodeName));
> 455: if (node.isPresent() && !node.get().isResource()) {
> 456: throw new IllegalStateException("Not a resource node: "
> + node.get());
The IllegalStateException is problematic here, maybe you mean to add an assert?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2177784419
Re: RFR: 8360037: Refactor ImageReader in preparation for Valhalla support
On Mon, 30 Jun 2025 23:27:49 GMT, David Beaumont wrote:
> Refactoring `ImageReader` to make it easy to add preview mode functionality
> for Valhalla.
>
> This PR is a large change to `ImageReader` (effectively a rewrite) but
> reduces the surface area of the API significantly, reduces code complexity
> and increases performance/memory efficiency. The need for this sort of change
> comes from the need to support "preview mode" in Valhalla for classes loaded
> from core modules.
>
> ### Preview Mode
>
> In the proposed preview mode support for Valhalla, a resource with the name
> `/modules//` may come from one of two locations in the
> underlying jimage file; `//` or
> `//META-INF/preview/`. Furthermore, directories (represented as
> directory nodes via the API) will have a potentially different number of
> child nodes depending on whether preview mode is in effect, and some
> directories may only exist at all in preview mode.
>
> Furthermore, the directories and symbolic link nodes in `/packages/...` will
> also be affected by the existence of new package directories. To retain
> consistency and avoid "surprises" later, all of this needs to be taken into
> account.
>
> Below is a summary of the main changes for mainline JDK to better support
> preview mode later:
>
> ### 1: Improved encapsulation for preview mode
>
> The current `ImageReader` code exposes the data from the jimage file in
> several ways; via the `Node` abstraction, but also with methods which return
> an `ImageLocation` instance directly. In preview mode it would not be
> acceptable to return the `ImageLocation`, since that would leak the existence
> of resources in the `META-INF/preview` namespace and lets users see resource
> nodes with names that don't match the underlying `ImageLocation` from which
> their contents come.
>
> As such, the PR removes all methods which can leak `ImageLocation` or other
> "underlying" semantics about the jimage file. Luckily most of these are
> either used minimally and easily migrated to using nodes, or they were not
> being used at all. This PR also removes any methods which were already unused
> across the OpenJDK codebase (if I discover any issues with over-trimming of
> the API during full CI testing, it will be easy to address).
>
> ### 2. Simplification of directory child node handling
>
> The current `ImageReader` code attempts to create parent directories "on
> demand" for any child nodes it creates. This results in parent directories
> having a non-empty but incomplete set of child nodes present. This is
> referred to as the directory being "incomple...
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 555:
> 553:
> 554: /**
> 555: * Returns the JRY file system compatible name of this node (e.g.
Typo: "JRY"
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
292:
> 290: @Override
> 291: public Optional find(String name) {
> 292: requireNonNull(name);
Personal preference is avoid static imports making the code readable by
retaining the "Objects." qualifier otherwise the non-local method call appears
out of thin air. YMMV.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2178300589
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2178311050
Re: RFR: 8360037: Refactor ImageReader in preparation for Valhalla support
On Tue, 1 Jul 2025 14:02:11 GMT, David Beaumont wrote:
>> Refactoring `ImageReader` to make it easy to add preview mode functionality
>> for Valhalla.
>>
>> This PR is a large change to `ImageReader` (effectively a rewrite) but
>> reduces the surface area of the API significantly, reduces code complexity
>> and increases performance/memory efficiency. The need for this sort of
>> change comes from the need to support "preview mode" in Valhalla for classes
>> loaded from core modules.
>>
>> ### Preview Mode
>>
>> In the proposed preview mode support for Valhalla, a resource with the name
>> `/modules//` may come from one of two locations in the
>> underlying jimage file; `//` or
>> `//META-INF/preview/`. Furthermore, directories (represented
>> as directory nodes via the API) will have a potentially different number of
>> child nodes depending on whether preview mode is in effect, and some
>> directories may only exist at all in preview mode.
>>
>> Furthermore, the directories and symbolic link nodes in `/packages/...` will
>> also be affected by the existence of new package directories. To retain
>> consistency and avoid "surprises" later, all of this needs to be taken into
>> account.
>>
>> Below is a summary of the main changes for mainline JDK to better support
>> preview mode later:
>>
>> ### 1: Improved encapsulation for preview mode
>>
>> The current `ImageReader` code exposes the data from the jimage file in
>> several ways; via the `Node` abstraction, but also with methods which return
>> an `ImageLocation` instance directly. In preview mode it would not be
>> acceptable to return the `ImageLocation`, since that would leak the
>> existence of resources in the `META-INF/preview` namespace and lets users
>> see resource nodes with names that don't match the underlying
>> `ImageLocation` from which their contents come.
>>
>> As such, the PR removes all methods which can leak `ImageLocation` or other
>> "underlying" semantics about the jimage file. Luckily most of these are
>> either used minimally and easily migrated to using nodes, or they were not
>> being used at all. This PR also removes any methods which were already
>> unused across the OpenJDK codebase (if I discover any issues with
>> over-trimming of the API during full CI testing, it will be easy to address).
>>
>> ### 2. Simplification of directory child node handling
>>
>> The current `ImageReader` code attempts to create parent directories "on
>> demand" for any child nodes it creates. This results in parent directories
>> having a non-empty but incomplete set of child nodes present. This is re...
>
> test/jdk/jdk/internal/jimage/ImageReaderTest.java line 78:
>
>> 76:
>> 77: @Test
>> 78: public void testModuleDirectories() throws IOException {
>
> I could make these more data driven (i.e. parametrized) if people want.
yes, please, it will make failures easier to identity.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2178321323
Re: RFR: 8320353: Reenable stringop-overflow warnings
On Tue, 1 Jul 2025 12:25:04 GMT, Anton Artemov wrote: > Hi, please consider the following changes: > > this PR addresses the issue of stringop-overflow warnings produced by GCC. > The compiler does think that the thread pointer returned by > `JavaThread::current()` can be null, though it cant. The thread pointer ends > up being an argument in `__atomic_load`, and the compiler reports the warning > related to argument of that method. > > The patch adds a hint to the compiler by means of `__builtin_unreachable()` > intrinsic, which tells the compiler that certain piece of code will never be > reached (case of thread pointer being null). This solves the issue. > > Tested in tiers 1-3 and GHA. Build changes look good. Someone else will have to review the hotspot changes. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/26067#pullrequestreview-2976656855
Re: RFR: 8361076: Add benchmark for ImageReader in preparation for Valhalla changes [v4]
> Initial benchmark to capture at least some comparative measures of > ImageReader performance. > > Current results on my laptop: > > Benchmark Mode Cnt ScoreError > Units > NewImageBenchmark.warmCache_CountAllNodes avgt5 0.785 ± 0.140 > ms/op > NewImageBenchmark.coldStart_CountOnly ss5 34.051 ± 17.662 > ms/op > NewImageBenchmark.coldStart_InitAndCountss5 31.006 ± 9.674 > ms/op > NewImageBenchmark.coldStart_LoadJavacInitClassesss5 3.752 ± 6.873 > ms/op > > The upcoming changes to ImageReader should show significant improvements to > these results. David Beaumont has updated the pull request incrementally with one additional commit since the last revision: Adding comment about maintainability. - Changes: - all: https://git.openjdk.org/jdk/pull/26044/files - new: https://git.openjdk.org/jdk/pull/26044/files/089b68e2..965abec0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=26044&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=26044&range=02-03 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/26044.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26044/head:pull/26044 PR: https://git.openjdk.org/jdk/pull/26044
Re: RFR: 8357979: [JVMCI] jdk.internal.vm.ci should have earlier class file version [v2]
On Mon, 2 Jun 2025 17:00:32 GMT, Doug Simon wrote: >> There are plans to have libgraal be built for JDK master using a version of >> Native Image running on a JDK one version behind the current JDK. This >> Native Image execution needs to be able to load the JVMCI classes. As such, >> the JVMCI classes must have a class file major version of N-1 where N is the >> major class file version of the current JDK. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > use BOOT_JDK_SOURCETARGET instead For another compilation, I got: error: option --system cannot be used together with --release I guess that even if the rule "-source/-target is almost never more appropriate than --release" applies to Java developers at large, building the JDK might be that small corner case where the "almost" actually applies. - PR Comment: https://git.openjdk.org/jdk/pull/25494#issuecomment-3024161108
RFR: 8360037: Refactor ImageReader in preparation for Valhalla support
Refactoring `ImageReader` to make it easy to add preview mode functionality for Valhalla. This PR is a large change to `ImageReader` (effectively a rewrite) but reduces the surface area of the API significantly, reduces code complexity and increases performance/memory efficiency. The need for this sort of change comes from the need to support "preview mode" in Valhalla for classes loaded from core modules. ### Preview Mode In the proposed preview mode support for Valhalla, a resource with the name `/modules//` may come from one of two locations in the underlying jimage file; `//` or `//META-INF/preview/`. Furthermore, directories (represented as directory nodes via the API) will have a potentially different number of child nodes depending on whether preview mode is in effect, and some directories may only exist at all in preview mode. Furthermore, the directories and symbolic link nodes in `/packages/...` will also be affected by the existence of new package directories. To retain consistency and avoid "surprises" later, all of this needs to be taken into account. Below is a summary of the main changes for mainline JDK to better support preview mode later: ### 1: Improved encapsulation for preview mode The current `ImageReader` code exposes the data from the jimage file in several ways; via the `Node` abstraction, but also with methods which return an `ImageLocation` instance directly. In preview mode it would not be acceptable to return the `ImageLocation`, since that would leak the existence of resources in the `META-INF/preview` namespace and lets users see resource nodes with names that don't match the underlying `ImageLocation` from which their contents come. As such, the PR removes all methods which can leak `ImageLocation` or other "underlying" semantics about the jimage file. Luckily most of these are either used minimally and easily migrated to using nodes, or they were not being used at all. This PR also removes any methods which were already unused across the OpenJDK codebase (if I discover any issues with over-trimming of the API during full CI testing, it will be easy to address). ### 2. Simplification of directory child node handling The current `ImageReader` code attempts to create parent directories "on demand" for any child nodes it creates. This results in parent directories having a non-empty but incomplete set of child nodes present. This is referred to as the directory being "incomplete" and requires users of the API to understand this and potentially "complete" these nodes by calling back into the `ImageReader` API. This approach is an attempt to provide lazy node creation (which is a necessary trait since there are ~31,000 nodes in a typical jimage hierarchy) but isn't actually necessary since a child node has no "get parent" functionality and simply not creating parent nodes at the point is functionally equivalent. The partially complete directory nodes also pose an issue for preview mode support, making it more complex to reason about where and when preview child nodes should be applied. This PR simplifies directory node handling in two ways: 1. Directory nodes are either empty (incomplete) or have all child nodes present. 2. Directory nodes no longer have an API to return their (possibly incomplete) child nodes to users. This results in simpler to reason about code for directory management and simpler changes later to support preview mode. Removing the on-demand parent directory creation and the partial child list management also has a noticeable performance improvement (based on the new `ImageReaderBenchmark` in pr/26044). ### 3. More comments and code assertions Since `ImageReader` is a critical bit of internal tooling for OpenJDK, I made sure to add significant comments explaining the behaviour, as well as adding many new in-code assert statements. The version of `ImageReader` in this PR is 70 lines shorter than the current version, but if you account for new comments, it's really a reduction of almost 40% (640 -> 400 lines of code) and has over 200 more comment lines. ### 4. New tests I added a new unit test for `ImageReader` since, until now, its API was not really being tested directly. These tests are useful now, but will really help when preview mode is added in Valhalla, since then there will need to be significant edge-case testing. I'm happy to improve or change tests in this PR, but they definitely cover the main cases. ### 5. Adding TODO comments for review discussion. There are obviously some open questions about the exact design of the APIs, and some questions around behaviour. To make reviewing easier, I've added inline TODOs which are there to illicit feedback. I will account for an remove all of these before the PR is integrated, but I want reviewers to read and comment on them (even if there is no change expected). ### Performance Improvements On my laptop (not objectively interesting, bu
Re: RFR: 8360037: Refactor ImageReader in preparation for Valhalla support
On Mon, 30 Jun 2025 23:27:49 GMT, David Beaumont wrote:
> Refactoring `ImageReader` to make it easy to add preview mode functionality
> for Valhalla.
>
> This PR is a large change to `ImageReader` (effectively a rewrite) but
> reduces the surface area of the API significantly, reduces code complexity
> and increases performance/memory efficiency. The need for this sort of change
> comes from the need to support "preview mode" in Valhalla for classes loaded
> from core modules.
>
> ### Preview Mode
>
> In the proposed preview mode support for Valhalla, a resource with the name
> `/modules//` may come from one of two locations in the
> underlying jimage file; `//` or
> `//META-INF/preview/`. Furthermore, directories (represented as
> directory nodes via the API) will have a potentially different number of
> child nodes depending on whether preview mode is in effect, and some
> directories may only exist at all in preview mode.
>
> Furthermore, the directories and symbolic link nodes in `/packages/...` will
> also be affected by the existence of new package directories. To retain
> consistency and avoid "surprises" later, all of this needs to be taken into
> account.
>
> Below is a summary of the main changes for mainline JDK to better support
> preview mode later:
>
> ### 1: Improved encapsulation for preview mode
>
> The current `ImageReader` code exposes the data from the jimage file in
> several ways; via the `Node` abstraction, but also with methods which return
> an `ImageLocation` instance directly. In preview mode it would not be
> acceptable to return the `ImageLocation`, since that would leak the existence
> of resources in the `META-INF/preview` namespace and lets users see resource
> nodes with names that don't match the underlying `ImageLocation` from which
> their contents come.
>
> As such, the PR removes all methods which can leak `ImageLocation` or other
> "underlying" semantics about the jimage file. Luckily most of these are
> either used minimally and easily migrated to using nodes, or they were not
> being used at all. This PR also removes any methods which were already unused
> across the OpenJDK codebase (if I discover any issues with over-trimming of
> the API during full CI testing, it will be easy to address).
>
> ### 2. Simplification of directory child node handling
>
> The current `ImageReader` code attempts to create parent directories "on
> demand" for any child nodes it creates. This results in parent directories
> having a non-empty but incomplete set of child nodes present. This is
> referred to as the directory being "incomple...
Anything marked "TODO" is for discussion during the coming review. I don't
intend to leave any of these in the code after, but they raise specific issues
I'd like address.
With these pre-preview comments, I think this is finally ready for review.
make/test/BuildMicrobenchmark.gmk line 1:
> 1: #
Ignore this file, it's part of the PR to add the benchmark. I'll merge and sort
everything out once that's in.
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 1:
> 1: /*
I think that this class is best reviewed as if it were a new implementation,
rather than trying to reason about the specific changes between the versions.
Hopefully the comments will make the design clear and the reduced
complexity/lines-of-code will help it be understood in a more stand-alone way.
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 95:
> 93: }
> 94:
> 95: // directory management interface
A large number of the methods below were either:
1. Unused or effectively no-ops.
2. Breaking encapsulation and leaking underlying types such as `ImageLocation`
(not a big problem now, but prevents cleanly implementing the preview mode
logic for Valhalla).
The new API has essentially 3 top level methods here:
1. findNode()
2. getResource(Node)
3. getResourceBuffer(Node)
Any navigation of the node hierarchy is done via `getChildNames()` and unlike
now, no user can obtain a reference to an "incomplete" node.
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 157:
> 155: }
> 156:
> 157: /**
This method and the `getResourceBuffer()` method below are, I think, only
called in one place and could, in theory, be factored out of this class
altogether.
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 255:
> 253: if (openers.isEmpty()) {
> 254: close();
> 255: // TODO (review note): Should this be synchronized
> by "this" ??
I genuinely this this might be an existing issue.
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 601:
> 599: }
> 600:
> 601: /**
Most of these were either never called or no longer needed in the new
implementation.
src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 122:
> 120:
> 121: @
Re: RFR: 8346719: Add relaunchers to the static JDK image for missing executables [v3]
On Fri, 27 Jun 2025 16:15:57 GMT, Magnus Ihse Bursie wrote: >> In the static JDK image, a single humongous java executable is generated, >> and no other launcher, such as javac. This makes it impossible to run our >> jtreg tests, which assume these are present. >> >> The solution is fortunately simply: we just need to add a bunch of trivial >> launchers, which are thin wrappers that execute the main java binary, with >> the proper arguments. This will result in the same behavior as the normal >> dynamic launchers, only that we will need to take the detour of launching >> another process instead of calling directly into the JLI library. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Remove problemlisting Any reviewers on this? - PR Comment: https://git.openjdk.org/jdk/pull/24380#issuecomment-3024250322
Re: RFR: 8361013: JMH does not export sufficient modules to benchmark ImageReader
On Sun, 29 Jun 2025 22:19:18 GMT, David Beaumont wrote: > Added jimage and jrtfs exports. > FWIW I think these changes to the makefiles are routine enough at this point > that we don't need to involve someone from the build team, but they can be > approved by any reviewer. I agree. Just adding addition `--exports` do not require a build system review. With that said, Skara will label any such changes with the `build` tag, so we will get notified. But you don't have to wait for a review from the build team. If I see such changes, I usually add a "build changes trivially fine" just to acknowledge this, but as I said, it's not really a requirement. - PR Comment: https://git.openjdk.org/jdk/pull/26035#issuecomment-3022005628
Re: RFR: 8360791: [ubsan] Adjust signal handling [v2]
On Mon, 30 Jun 2025 07:02:31 GMT, Matthias Baesken wrote: >> A couple of tests e.g. VendorInfoPluginsTest but also some Hotspot like >> runtime/ErrorHandling/CreateCoredumpOnCrash.java put (write) to special >> addresses like 0 to provoke crashs. >> However this does not work well with ubsan-enabled binaries on the clang >> based platforms (macOS, AIX). >> The mentioned tests generate a SIGSEGV. >> >> Some other tests generate a SIGBUS, e.g. >> >> runtime/memory/ReserveMemory.java >> runtime/memory/ReadFromNoaccessArea.java >> >> and this leads to similar issues with ubsan-enabled binaries. >> >> We should adjust the signal handling with the sanitizer options, how to do >> this is documented here : >> https://github.com/google/sanitizers/wiki/SanitizerCommonFlags > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Remove the clang check LGTM. I agree: don't exclude things just because there is a vague suspicion. - Marked as reviewed by lucy (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/26015#pullrequestreview-2974602300
Re: RFR: 8360791: [ubsan] Adjust signal handling [v2]
On Mon, 30 Jun 2025 07:02:31 GMT, Matthias Baesken wrote: >> A couple of tests e.g. VendorInfoPluginsTest but also some Hotspot like >> runtime/ErrorHandling/CreateCoredumpOnCrash.java put (write) to special >> addresses like 0 to provoke crashs. >> However this does not work well with ubsan-enabled binaries on the clang >> based platforms (macOS, AIX). >> The mentioned tests generate a SIGSEGV. >> >> Some other tests generate a SIGBUS, e.g. >> >> runtime/memory/ReserveMemory.java >> runtime/memory/ReadFromNoaccessArea.java >> >> and this leads to similar issues with ubsan-enabled binaries. >> >> We should adjust the signal handling with the sanitizer options, how to do >> this is documented here : >> https://github.com/google/sanitizers/wiki/SanitizerCommonFlags > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Remove the clang check Thanks for the reviews ! - PR Comment: https://git.openjdk.org/jdk/pull/26015#issuecomment-3023099250
Re: RFR: 8357979: [JVMCI] jdk.internal.vm.ci should have earlier class file version
On Mon, 2 Jun 2025 17:21:21 GMT, Joe Darcy wrote: >> I don't think this is a good way. We should not call to shell to make >> calculations in the makefiles; if that were needed, it should be done in >> configure. However, we are already doing this, or at least something very >> similar. Have a look at line 398 in boot-jdk.m4. We set up `OLDEST_BOOT_JDK` >> as the oldest JDK from the list in version-numbers.conf. >> >> That should, I think, match your expectation of N-1. There is always a bit >> uncertainty how you want these things to be handled around the version >> rollover, and I'm not sure about your use case in those situations. But in >> general, the oldest boot JDK will match the version number actually used in >> the build as the boot JDK, that is, N-1. >> >> Now, OLDEST_BOOT_JDK is not exported to make. But that is however: >> >> BOOT_JDK_SOURCETARGET="-source $OLDEST_BOOT_JDK -target $OLDEST_BOOT_JDK >> -Xlint:-options" >> >> which is almost, but not exactly, what you were planning to add. The added >> `-Xlint:-options` is harmless afaict, and I think was likely added at some >> point due to option mismatch between N and N-1 javac lint support, so I >> think it would make sense for you to use it as well. >> >> So, in essense, my suggestion boils down to that you should only add >> `$(BOOT_JDK_SOURCETARGET)` to your command line. > >> I don't think this is a good way. We should not call to shell to make >> calculations in the makefiles; if that were needed, it should be done in >> configure. However, we are already doing this, or at least something very >> similar. Have a look at line 398 in boot-jdk.m4. We set up `OLDEST_BOOT_JDK` >> as the oldest JDK from the list in version-numbers.conf. >> >> That should, I think, match your expectation of N-1. There is always a bit >> uncertainty how you want these things to be handled around the version >> rollover, and I'm not sure about your use case in those situations. But in >> general, the oldest boot JDK will match the version number actually used in >> the build as the boot JDK, that is, N-1. >> >> Now, OLDEST_BOOT_JDK is not exported to make. But that is however: >> >> ``` >> BOOT_JDK_SOURCETARGET="-source $OLDEST_BOOT_JDK -target $OLDEST_BOOT_JDK >> -Xlint:-options" >> ``` >> >> which is almost, but not exactly, what you were planning to add. The added >> `-Xlint:-options` is harmless afaict, and I think was likely added at some >> point due to option mismatch between N and N-1 javac lint support, so I >> think it would make sense for you to use it as well. >> >> So, in essense, my suggestion boils down to that you should only add >> `$(BOOT_JDK_SOURCETARGET)` to your command line. > > I don't know the particulars here, but using > > `javac -source $OLD -target $OLD ..." > > is _almost_ never more appropriate than using > > `javac --release $OLD ..." > > The `options` warning would be noting the possible use of `--release`. @jddarcy I tried replacing the `-source/-target` with `--release` for the Boot JDK overall. However, I get this kind of errors: error: exporting a package from system module java.base is not allowed with --release I assume this is why we have still been using `-source/-target`. But maybe we should move to use `--release` instead, and figure out and fix this issue in a different way? - PR Comment: https://git.openjdk.org/jdk/pull/25494#issuecomment-3024001740
Re: RFR: 8361076: Add benchmark for ImageReader in preparation for Valhalla changes [v3]
On Mon, 30 Jun 2025 15:08:25 GMT, David Beaumont wrote: >> Initial benchmark to capture at least some comparative measures of >> ImageReader performance. >> >> Current results on my laptop: >> >> Benchmark Mode Cnt ScoreError >> Units >> NewImageBenchmark.warmCache_CountAllNodes avgt5 0.785 ± 0.140 >> ms/op >> NewImageBenchmark.coldStart_CountOnly ss5 34.051 ± 17.662 >> ms/op >> NewImageBenchmark.coldStart_InitAndCountss5 31.006 ± 9.674 >> ms/op >> NewImageBenchmark.coldStart_LoadJavacInitClassesss5 3.752 ± 6.873 >> ms/op >> >> The upcoming changes to ImageReader should show significant improvements to >> these results. > > David Beaumont has updated the pull request incrementally with one additional > commit since the last revision: > > Some tidying up relating to the state objects. test/micro/org/openjdk/bench/jdk/internal/jrtfs/ImageReaderBenchmark.java line 230: > 228: // Created by running "java -verbose:class", throwing away anonymous > inner > 229: // classes and anything without a reliable name, and grouping by the > stated > 230: // source. It's not perfect, but it's representative. I don't think this is maintainable. How useful (or not) is this benchmark if the names of all the internal classes (that will change from release to release) are dropped from this? - PR Review Comment: https://git.openjdk.org/jdk/pull/26044#discussion_r2176636106
Re: RFR: 8360037: Refactor ImageReader in preparation for Valhalla support
On Mon, 30 Jun 2025 23:27:49 GMT, David Beaumont wrote:
> Refactoring `ImageReader` to make it easy to add preview mode functionality
> for Valhalla.
>
> This PR is a large change to `ImageReader` (effectively a rewrite) but
> reduces the surface area of the API significantly, reduces code complexity
> and increases performance/memory efficiency. The need for this sort of change
> comes from the need to support "preview mode" in Valhalla for classes loaded
> from core modules.
>
> ### Preview Mode
>
> In the proposed preview mode support for Valhalla, a resource with the name
> `/modules//` may come from one of two locations in the
> underlying jimage file; `//` or
> `//META-INF/preview/`. Furthermore, directories (represented as
> directory nodes via the API) will have a potentially different number of
> child nodes depending on whether preview mode is in effect, and some
> directories may only exist at all in preview mode.
>
> Furthermore, the directories and symbolic link nodes in `/packages/...` will
> also be affected by the existence of new package directories. To retain
> consistency and avoid "surprises" later, all of this needs to be taken into
> account.
>
> Below is a summary of the main changes for mainline JDK to better support
> preview mode later:
>
> ### 1: Improved encapsulation for preview mode
>
> The current `ImageReader` code exposes the data from the jimage file in
> several ways; via the `Node` abstraction, but also with methods which return
> an `ImageLocation` instance directly. In preview mode it would not be
> acceptable to return the `ImageLocation`, since that would leak the existence
> of resources in the `META-INF/preview` namespace and lets users see resource
> nodes with names that don't match the underlying `ImageLocation` from which
> their contents come.
>
> As such, the PR removes all methods which can leak `ImageLocation` or other
> "underlying" semantics about the jimage file. Luckily most of these are
> either used minimally and easily migrated to using nodes, or they were not
> being used at all. This PR also removes any methods which were already unused
> across the OpenJDK codebase (if I discover any issues with over-trimming of
> the API during full CI testing, it will be easy to address).
>
> ### 2. Simplification of directory child node handling
>
> The current `ImageReader` code attempts to create parent directories "on
> demand" for any child nodes it creates. This results in parent directories
> having a non-empty but incomplete set of child nodes present. This is
> referred to as the directory being "incomple...
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 207:
> 205: this.nodes = new HashMap<>();
> 206: // TODO (review note): These should exist under all
> circumstances, but there's
> 207: // probably a more robust way of getting it these offsets.
Suggestion:
// probably a more robust way of getting these offsets.
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 211:
> 209: this.packagesStringOffset =
> findLocation("/packages/java.lang").getModuleOffset();
> 210:
> 211: // Node creation is very lazy, se can just make the
> top-level directories
Typo:
Suggestion:
// Node creation is very lazy, so can just make the top-level
directories
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 544:
> 542:
> 543: // A node is completed when all its direct children have been
> built. As
> 544: // such, non-directory nodes are always complete.
I think this might flow better:
Suggestion:
// A node is completed when all its direct children have been built.
// As such, non-directory nodes are always complete.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2177769091
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2177768332
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r219216
Re: RFR: 8360037: Refactor ImageReader in preparation for Valhalla support
On Mon, 30 Jun 2025 23:27:49 GMT, David Beaumont wrote:
> Refactoring `ImageReader` to make it easy to add preview mode functionality
> for Valhalla.
>
> This PR is a large change to `ImageReader` (effectively a rewrite) but
> reduces the surface area of the API significantly, reduces code complexity
> and increases performance/memory efficiency. The need for this sort of change
> comes from the need to support "preview mode" in Valhalla for classes loaded
> from core modules.
>
> ### Preview Mode
>
> In the proposed preview mode support for Valhalla, a resource with the name
> `/modules//` may come from one of two locations in the
> underlying jimage file; `//` or
> `//META-INF/preview/`. Furthermore, directories (represented as
> directory nodes via the API) will have a potentially different number of
> child nodes depending on whether preview mode is in effect, and some
> directories may only exist at all in preview mode.
>
> Furthermore, the directories and symbolic link nodes in `/packages/...` will
> also be affected by the existence of new package directories. To retain
> consistency and avoid "surprises" later, all of this needs to be taken into
> account.
>
> Below is a summary of the main changes for mainline JDK to better support
> preview mode later:
>
> ### 1: Improved encapsulation for preview mode
>
> The current `ImageReader` code exposes the data from the jimage file in
> several ways; via the `Node` abstraction, but also with methods which return
> an `ImageLocation` instance directly. In preview mode it would not be
> acceptable to return the `ImageLocation`, since that would leak the existence
> of resources in the `META-INF/preview` namespace and lets users see resource
> nodes with names that don't match the underlying `ImageLocation` from which
> their contents come.
>
> As such, the PR removes all methods which can leak `ImageLocation` or other
> "underlying" semantics about the jimage file. Luckily most of these are
> either used minimally and easily migrated to using nodes, or they were not
> being used at all. This PR also removes any methods which were already unused
> across the OpenJDK codebase (if I discover any issues with over-trimming of
> the API during full CI testing, it will be easy to address).
>
> ### 2. Simplification of directory child node handling
>
> The current `ImageReader` code attempts to create parent directories "on
> demand" for any child nodes it creates. This results in parent directories
> having a non-empty but incomplete set of child nodes present. This is
> referred to as the directory being "incomple...
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
62:
> 60: import jdk.internal.module.ModuleHashes.HashSupplier;
> 61:
> 62: import static java.util.Objects.requireNonNull;
The existing Objects.requireNonNull are okay, no need to change them all as it
is nothing to do with the changes.
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
251:
> 249: }
> 250:
> 251: private static Stream getModuleAttributes() {
Can you rename this to allModuleAttributes and add a method description to
match the other methods.
src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
257:
> 255: .getChildNames()
> 256: .map(mn -> getNode(reader, mn + "/module-info.class"))
> 257: // This fails with ISE if the node isn't a resource
> (corrupt JImage).
Can you drop this comment and check getNode to thrown an Error (ISE isn't right
when we have a broken image).
test/micro/org/openjdk/bench/jdk/internal/jrtfs/ImageReaderBenchmark.java line
60:
> 58: @OutputTimeUnit(TimeUnit.MILLISECONDS)
> 59: @Fork(value = 1, jvmArgs = {"--add-exports",
> "java.base/jdk.internal.jimage=ALL-UNNAMED"})
> 60: public class ImageReaderBenchmark {
This is the benchmark from the other PR, did you mean to include it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2177761299
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2177768203
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r212042
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2177762265
Integrated: 8360791: [ubsan] Adjust signal handling
On Fri, 27 Jun 2025 09:30:19 GMT, Matthias Baesken wrote: > A couple of tests e.g. VendorInfoPluginsTest but also some Hotspot like > runtime/ErrorHandling/CreateCoredumpOnCrash.java put (write) to special > addresses like 0 to provoke crashs. > However this does not work well with ubsan-enabled binaries on the clang > based platforms (macOS, AIX). > The mentioned tests generate a SIGSEGV. > > Some other tests generate a SIGBUS, e.g. > > runtime/memory/ReserveMemory.java > runtime/memory/ReadFromNoaccessArea.java > > and this leads to similar issues with ubsan-enabled binaries. > > We should adjust the signal handling with the sanitizer options, how to do > this is documented here : > https://github.com/google/sanitizers/wiki/SanitizerCommonFlags This pull request has now been integrated. Changeset: aeca49e4 Author:Matthias Baesken URL: https://git.openjdk.org/jdk/commit/aeca49e43fab951c2031895fee32703fb4a19524 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod 8360791: [ubsan] Adjust signal handling Reviewed-by: ihse, lucy - PR: https://git.openjdk.org/jdk/pull/26015
RFR: 8361142: Improve custom hooks for makefiles
Add some additional and more fine-grained way in which custom makefiles can be integrated. - Commit messages: - Missed hook for cleaning - Turns out these were needed after all... - 8361142: Improve custom hooks for makefiles Changes: https://git.openjdk.org/jdk/pull/26060/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=26060&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8361142 Stats: 38 lines in 8 files changed: 24 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/26060.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26060/head:pull/26060 PR: https://git.openjdk.org/jdk/pull/26060
