On Sat, 17 Jan 2026 21:56:05 GMT, Eirik Bjørsnøs <[email protected]> wrote:
> Please review this PR which proposes we replace `j.u.ArrayDeque` with
> `j.u.ArrayList` in `jdk.internal.loader.URLClassPath`.
>
> The motivation for using a double-ended queue may have been that "original"
> search path URLs are added to the tail of the queue while any "loader
> discovered" class path URLs are inserted at the head such that they are
> processed first.
>
> By splitting these two concerns and processing loader discovered class path
> URLs separately from the original search path, we no longer need a
> double-ended queue. We can replace `ArrayDeque` with `ArrayList`.
>
> Advantages:
>
> * A "hello world" Java program no longer loads `ArrayDeque`, `Deque` and
> `Queue` (`URLClassPath` is the only consumer of these classes during startup)
> * One data structure is simpler than two
> * We no longer need to manage search path URLs across two different
> collections
> * Code and comments to dance around `ArrayDeque` calling into lambda too
> early is no longer a concern and can be removed
> * I think this PR leaves the code overall simpler and easier to follow.
>
> Testing:
>
> This PR introduces a new test to verify that URLs disovered via a multi-level
> tree paths discovered via `Class-Path` JAR attributes are found in the
> expected DFS order. The ordering aspect seems to lack existing coverage.
Looks much better and thanks for the test, but I still think `loaderPath` might
be renamed to `expandedJars` or something similar to indicate the nature of
those urls. (You can consider renaming `push` too)
The code looks good to me logically.
The rename is a net improvement for code readability. 👍
src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 107:
> 105:
> 106: /* A list of loader-discovered URLs, if any */
> 107: private ArrayList<URL> loaderPath;
Document these three variables' accesses are guarded by monitor on `path`.
In addition, this is probably more accurately called `dfsUrls`, because they
are paths waiting for expansion from a DFS.
src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 371:
> 369: * Returns the next URL to process or null if finished
> 370: */
> 371: private URL nextURL() {
Document that this method must be called when holding a lock on `path`
src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 485:
> 483: // Lazily create list
> 484: if (loaderPath == null) {
> 485: loaderPath = new ArrayList<>();
This will always be used if we are using `JarLoader`, which seems to be used in
the majority of cases. I recommend initializing this dfs variable
unconditionally.
src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 490:
> 488: for (int i = urls.length - 1; i >= 0; --i) {
> 489: loaderPath.addLast(urls[i]);
> 490: }
Should we do `dfsUrls.addAll(Arrays.asList(urls).reversed())` for simplicity?
src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 490:
> 488: private void addExpandedPaths(URL[] urls) {
> 489: synchronized (searchPath) {
> 490: // Adding in reversed order since URLs are consumed
> tail-first
Suggestion:
// Adding in reversed order since expandedPath is consumed
tail-first
-------------
PR Review: https://git.openjdk.org/jdk/pull/29288#pullrequestreview-3676106618
Marked as reviewed by liach (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/29288#pullrequestreview-3679120128
PR Comment: https://git.openjdk.org/jdk/pull/29288#issuecomment-3769348993
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2702193525
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2702193246
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2702220970
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2702222377
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2705527983
