> Can I please get a review for this change which fixes an issue in > `jdk.internal.jimage.ImageReader`? > > The `ImageReader` maintains a map of `nodes` which it uses to keep track of > directories/resources from within the image. When a `findNode` is called, it > leads to building the `Node` if the node isn't present in the map or if isn't > complete (i.e. if the directory's immediate child nodes haven't been built). > During this building of nodes, the code can end up creating a directory node > and then visiting the child resources to create nodes for those resources. > While doing so, the code currently doesn't check if a (child) node for the > resource was already created previously (during a different call) and added > to the parent directory's `children` list. As a result, it ends up adding the > child node more than once. > > One way to solve this would have been to make the `children` in `Directory` > be a `Set`. I suspect it's currently a `List` so as to keep track of the > order? If we did make it a `Set`, we could still keep the order using a > `LinkedHashSet`. But changing this to a `Set` doesn't look feasible because > there's a `List<Node> getChildren()` API on `Directory`, which would then > have to create a copy of the `Set` to return as a `List`. > > The commit in this PR instead prevents adding the child more than once by > checking the `nodes` map for the presence of the child. This new check is > added only for "resources" and not directories, because `makeDirectory` > already skips a `Node` creation if the `nodes` has the corresponding entry > (line 564 in the current PR). > Additionally an `assert` has been added to `addChild` of `Directory`. > > A new jtreg test has been added which reproduces the issue without this > change and verifies the fix. > > This part of the code is very new to me, so if there's anything more to be > considered, then please let me know. > > tier1, tier2, tier3 testing is currently in progress.
Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge latest from jdk19 master - 8288744: Remove tools/jlink/plugins/CompressorPluginTest.java from problemlist - 8247407: tools/jlink/plugins/CompressorPluginTest.java test failing ------------- Changes: - all: https://git.openjdk.org/jdk19/pull/60/files - new: https://git.openjdk.org/jdk19/pull/60/files/40bc7db8..8a9aa702 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk19&pr=60&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk19&pr=60&range=00-01 Stats: 443 lines in 21 files changed: 272 ins; 105 del; 66 mod Patch: https://git.openjdk.org/jdk19/pull/60.diff Fetch: git fetch https://git.openjdk.org/jdk19 pull/60/head:pull/60 PR: https://git.openjdk.org/jdk19/pull/60
