milamberspace commented on issue #6720: URL: https://github.com/apache/jmeter/issues/6720#issuecomment-4995816172
### Review summary for the proposed optimization The implementation in #6721 has been reviewed — disposition: **changes requested** ([review](https://github.com/apache/jmeter/pull/6721#pullrequestreview-4717026454)). The caching idea is sound (a sampler's path to root is static during thread execution), but the current implementation has a **blocking correctness issue** plus a testing gap: 1. **Cache key semantics (blocking).** The cache is a `HashMap<Sampler, List<Controller>>`, so it keys on `equals`/`hashCode` — which `AbstractTestElement` defines **by value** (`propMap`). The original tree walk instead identifies the node by **reference identity** (`node == nodeToFind` in `FindTestElementsUpToRootTraverser`). Two distinct-but-equal samplers (e.g. copy-pasted requests) would collide and receive the wrong parent-controller path on error; additionally, a sampler's properties mutate at runtime, making it an unstable hash key. Suggested fix: `IdentityHashMap`, which restores the original identity semantics. 2. **No regression test.** The change alters behaviour on a correctness-sensitive path (loop control on sample error), so a cache-correctness regression would be silent. A test exercising the cache-hit branch — ideally with two equal-but-distinct samplers — is needed. The performance motivation itself is not in question; only the correctness of the caching mechanism. Full details are in the inline review comments on #6721. <sub>This summary was drafted by an AI-assisted tool (Apache Magpie) and may contain mistakes; an Apache JMeter maintainer has reviewed and confirmed it.</sub> -- 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]
