On Sat, 25 Jun 2022 02:45:54 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> 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 LGTM You'll now have to remove the test from the problem list. ------------- Marked as reviewed by jlaskey (Reviewer). PR: https://git.openjdk.org/jdk19/pull/60
