gnodet commented on code in PR #2075:
URL: https://github.com/apache/maven-resolver/pull/2075#discussion_r3879441193
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java:
##########
@@ -445,12 +432,17 @@ private Path(State state, DependencyNode dn, String
conflictId, Path parent) {
/**
* Checks whether the given conflictId appears on the path from this
node to the root.
- * Uses a pre-built {@link HashSet} of conflict IDs accumulated along
the path from root,
- * making this an O(1) operation instead of the previous O(depth)
parent-chain walk that
- * showed up as a JFR hotspot (3.9% CPU) in large multi-module builds.
+ * Walks the parent chain comparing conflict IDs. Since dependency
tree depth is bounded
+ * in practice (< 30), each check is fast while avoiding per-node
HashSet allocation
+ * that was a major JFR hotspot (~45% CPU) in large multi-module
builds.
*/
Review Comment:
Minor (non-blocking): `targetConflictId.equals(current.conflictId)` would
NPE if `targetConflictId` is null. The pre-HashSet version (before commit
51f1af64) used `Objects.equals(current.conflictId, targetConflictId)` which was
null-safe.
In practice, null conflict IDs would indicate a separate upstream bug
(ConflictMarker assigns IDs to all nodes with dependencies), and the codebase
already uses the same non-null-safe pattern elsewhere (line 967), so this is
consistent. Just noting it for completeness.
```suggestion
for (Path current = this; current != null; current =
current.parent) {
if (Objects.equals(targetConflictId, current.conflictId)) {
return true;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
