On Thu, 23 Jun 2022 12:59:34 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. LGTM. Nice catch! ------------- Marked as reviewed by sundar (Reviewer). PR: https://git.openjdk.org/jdk19/pull/60
