On Wed, 21 Jan 2026 10:22:07 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 URLs
>> discovered during JAR `Class-Path` expansion 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
>> using a directory class path)
>> * 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.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use "list" instead of "queue" when referring to expandedPath which is
> processed FIFO and as such is not a queue
src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 100:
> 98: }
> 99:
> 100: /* Search path of URLs passed to the constructor or by calls to
> addURL
I don't know if it was intentional, but this comment and some others added in
this PR are missing a `.` after the sentence.
src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 113:
> 111: * Access is guarded by a monitor on 'searchPath'
> 112: */
> 113: private final ArrayList<URL> expandedPath = new ArrayList<>();
I think we should name this `manifestClassPath`.
src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 139:
> 137: public URLClassPath(URL[] urls,
> 138: URLStreamHandlerFactory factory) {
> 139: // Reject null URLs
Nit - I think it would be better to clarify that it's intentional to (continue)
to reject null element in the array. So may be reword this comment to "Reject
null URL array or any null element in the array"?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2727583497
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2727581431
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2727574621
