On Mon, 19 Jan 2026 03:12:14 GMT, Chen Liang <[email protected]> wrote:
> You can consider renaming `push` too
How do you like the following:
/*
* Adds the specified URLs to the queue of 'Class-Path' expanded URLs
*/
private void addExpandedPaths(URL[] urls) {
//...
}
> 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.
Thanks for poking. I was pondering this, not being quite happy with any of the
names floated.
Maybe we should rename `path` as well, since this is competely generic and
doesn't contrast well with anything.
I think names and comments here should aid maintainers to quickly get some
understanding of why we have two different lists, which URLs go where and where
they originate from.
How do you like the following?
/* Search path of URLs passed to the constructor or by calls to addURL(URL)
* Access is guarded by a monitor on 'searchPath' itself
*/
private final ArrayList<URL> searchPath;
/* Index of the next URL in the search path to process
* Access is guarded by a monitor on 'searchPath'
*/
private int nextURL = 0;
/* Queue of URLs found during expansion of JAR 'Class-Path' attributes
* Access is guarded by a monitor on 'searchPath'
*/
private final ArrayList<URL> expandedPath = new ArrayList<>();
> The rename is a net improvement for code readability. 👍
Thanks a lot for very helpful feedback. I went ahead and folded the
synchronization into `nextURL`.
With all initial concerns addressed, I'm moving this out of Draft state to
invite more reviewers.
Channeling @jaikiran and Code Historian @Martin-Buchholz who was the one to
introduce `ArrayDeque` in this code.
> Document these three variables' accesses are guarded by monitor on `path`.
Thanks, see above.
> In addition, this is probably more accurately called `dfsUrls`, because they
> are paths waiting for expansion from a DFS.
Excuse my ignorance, what do you mean by 'DFS' in this context?
> 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`
Thanks, I have added notes to the three fields and this method.
> This will always be used if we are using `JarLoader`, which seems to be used
> in the majority of cases.
Not exactly, `push` is only called when the JAR uses the 'Class-Path:'
attribute. I don't have data to support this, but from personal experience I
don't see this being used too often.
> I recommend initializing this dfs variable unconditionally.
GIven the above, do you still think it's worth initializing this eagerly? It
would allow us to make the field final and avoid a null check.
EDIT: Decided to initialize eagerly and make the field final, see below.
> 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?
Sympathizing with Alan's concern in the JBS about being extra cautious about
introducing subtle behaviour changes in this change, I have tried to limit any
cleanups not motivated by the change itself to the absolute minimum. Leaving
iteration as-is also simplifies the analysis of performance implications.
I agree that the code you suggest expresses the intent much better though. If
you feel confident reviewing it despite the stated concerns, I'm happy to apply
it. I leave it to you.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/29288#issuecomment-3767376134
PR Comment: https://git.openjdk.org/jdk/pull/29288#issuecomment-3767412140
PR Comment: https://git.openjdk.org/jdk/pull/29288#issuecomment-3769506874
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2702267790
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2702267133
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2702269596
PR Review Comment: https://git.openjdk.org/jdk/pull/29288#discussion_r2702273490
