Optimize parent controller path retrieval in JMeterThread

[rajat315315]

Fixes: #6720

Description

This PR introduces a thread-local parent controller path cache (parentControllersCache) inside JMeterThread.

Specifically, the changes are:

1. Added a HashMap mapping Sampler to List<Controller> in JMeterThread.
2. Created a lightweight nested class CachedPathToRootTraverser extending FindTestElementsUpToRootTraverser to bypass tree walks on cache hits.
3. Updated triggerLoopLogicalActionOnParentControllers to lookup resolved paths in the cache and save the path on a cache miss.

Motivation and Context

When a sampler execution fails with the onErrorStartNextLoop option enabled, or when loop logical actions (such as Break/Continue) are triggered, JMeter traverses the entire test tree using DFS (testTree.traverse()) from the sampler to the root to resolve parent controllers.

For large test plans with deep nesting, this full tree walk is redundant since the test tree's hierarchy is static during thread execution. Under error-heavy loads, these continuous traversals degrade performance and spike CPU usage. Caching the path to the root per-sampler inside each thread avoids this overhead entirely.

How Has This Been Tested?

1. Performance Microbenchmark:
   Created a microbenchmark simulating a deeply nested test plan structure (10 nested levels of loop controllers) over 100,000 iterations:
   • Without cache (full tree walk): 292.57 ms
   • With cache (lazy map lookup): 30.68 ms
   • Performance Speedup: ~9.53x (over 950% performance improvement).
2. Compilation:
   Compiled core module Java classes using ./gradlew :src:core:compileJava.
3. Unit Tests:
   Executed the core test suite using ./gradlew :src:core:test (379 tests completed, 0 failed, 1 skipped).

Screenshots (if appropriate):

N/A (performance optimization)

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code follows the code style of this project.
• I have updated the documentation accordingly.

[vlsi]

Please share benchmark code

[milamberspace]

Thanks for the optimization, @rajat315315 — the microbenchmark and the motivation are clear, and caching a static path is the right instinct. One blocking correctness issue needs to be resolved before this can land, plus a testing gap; details inline.

Blocking (see inline on the cache field): the cache is keyed on Sampler, but AbstractTestElement defines equals/hashCode by value (propMap), whereas the original traversal matches the node by reference identity (node == nodeToFind). Two distinct-but-equal samplers would collide and get the wrong parent-controller path on error, and samplers mutate their properties at runtime (unstable hash key). Switching to IdentityHashMap restores the original identity semantics.

Testing: the change alters behaviour on a correctness-sensitive path (loop control on sample error) and adds no tests, so a cache-correctness regression would be silent. Please add a regression test that runs enough iterations to exercise the cache-hit branch and asserts identical Break/Continue/StartNextLoop behaviour to the uncached path — ideally including two equal-but-distinct samplers to lock in the identity concern above. TestTransactionController and the existing onErrorStartNextLoop tests are good starting points.

(Not blocking: the two red macOS checks are the flaky :src:dist-check:batchServerBatchTestLocal distributed test, unrelated to this diff — the rest of the matrix, including the same-hashcode job, is green.)

---

This review was drafted by an AI-assisted tool (Apache Magpie) and may contain mistakes. An Apache JMeter maintainer has reviewed and confirmed this submission. See CONTRIBUTING.md.

[milamberspace]

Correctness (blocking): keying this cache on Sampler changes the lookup semantics.

The original code locates the sampler in the tree by reference identity:

// FindTestElementsUpToRootTraverser.addNode
if (node == nodeToFind) { this.stopRecording = true; }   // ==, identity

But a HashMap keys on equals/hashCode, and AbstractTestElement overrides both to be value-based:

// AbstractTestElement.java
public boolean equals(Object o) { ... return other.propMap.equals(propMap); }
public int hashCode()        { return propMap.hashCode(); }

Two real consequences:

1. Collision → wrong controllers. Two distinct samplers with identical properties (copy-pasted requests, two default-named Debug Samplers, …) are .equals() and share a bucket. On a cache hit the second sampler gets the first sampler's path-to-root, so breakOnCurrentLoop / continueOnThreadLoop / startNextLoop act on the wrong parent controllers on error. The identity-based traversal never had this bug.
2. Mutable key. A sampler's propMap mutates during execution (variable substitution, runningVersion, config merge), so its hashCode changes while it is a key — violating the Map contract.

Suggested fix: use an identity-keyed map, which matches the traverser's == semantics and is immune to property mutation.

Suggested change

	private final Map<Sampler, List<Controller>> parentControllersCache = new HashMap<>();
	private final Map<Sampler, List<Controller>> parentControllersCache = new IdentityHashMap<>();

(Remember to import java.util.IdentityHashMap; and drop the now-unused HashMap import.)

[milamberspace]

Minor (design): subclassing the traverser via super(null) just to override getControllersToRoot() works only because every current consumer calls that one method. A future consumer that touches the recorded stack would get an empty deque from this cached instance. Since the cache is consumed purely as a List<Controller>, consider passing the list directly to the consumers instead of faking a traverser — or at minimum add a comment documenting that only getControllersToRoot() is valid on this subclass.

[vlsi]

Thanks for the PR. I think we should take a different approach here. TestCompiler already computes each sampler’s controller path while compiling the test tree and stores it in SamplePackage. We should expose and reuse that existing information rather than adding a second cache and traversing the tree again on cache misses.

[rajat315315]

I agree. I totally missed that we are building the same cache in SamplePackage class too. Let me add and test.

[rajat315315]

Benchmarking Results

Sibling Count	Tree Depth	Without Change (traversal)	With Change (compilerLookup)	Speedup	Allocation Rate (Without)	Allocation Rate (With)
1	2	215.899 ns/op	5.939 ns/op	36.3x	224 B/op	0 B/op
1	5	370.216 ns/op	4.269 ns/op	86.7x	248 B/op	0 B/op
1	10	791.889 ns/op	6.977 ns/op	113.5x	288 B/op	0 B/op
10	2	686.411 ns/op	5.292 ns/op	129.7x	224 B/op	0 B/op
10	5	1818.898 ns/op	5.649 ns/op	321.9x	248 B/op	0 B/op
10	10	3191.963 ns/op	5.318 ns/op	600.2x	288 B/op	0 B/op

Note: All execution times represent average time per operation. Lower is better.

Key Takeaways

1. Constant-Time Lookup: The old tree-traversal approach scales linearly with the size, depth, and width of the test plan (taking over 3.1 microseconds on a larger plan). The new map-lookup approach runs in constant time (~4–7 nanoseconds) regardless of the tree configuration.
2. Zero GC Pressure: The old tree-traversal allocated between 224 and 288 bytes per operation on the heap to track path traversal state. The optimized lookup does not allocate any heap memory (0 B/op).

[rajat315315]

I used below benchmark script to compare the time taken.
It was auto-generated for me using an AI model.

/*
 * Licensed to the Apache Software Foundation (ASF) under one or more
 * contributor license agreements.  See the NOTICE file distributed with
 * this work for additional information regarding copyright ownership.
 * The ASF licenses this file to you under the Apache License, Version 2.0
 * (the "License"); you may not use this file except in compliance with
 * the License.  You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package org.apache.jmeter.threads;

import java.util.List;
import java.util.concurrent.TimeUnit;

import org.apache.jmeter.control.Controller;
import org.apache.jmeter.control.GenericController;
import org.apache.jmeter.control.LoopController;
import org.apache.jmeter.samplers.AbstractSampler;
import org.apache.jmeter.samplers.Entry;
import org.apache.jmeter.samplers.SampleResult;
import org.apache.jorphan.collections.ListedHashTree;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.Blackhole;
import org.openjdk.jmh.profile.GCProfiler;
import org.openjdk.jmh.runner.Runner;
import org.openjdk.jmh.runner.RunnerException;
import org.openjdk.jmh.runner.options.Options;
import org.openjdk.jmh.runner.options.OptionsBuilder;

/**
 * Benchmark comparing two approaches for retrieving a sampler's parent
 * controller path in JMeterThread:
 * <ul>
 *   <li><b>traversal</b> — original approach: traverse the entire HashTree
 *       with {@link FindTestElementsUpToRootTraverser} on every call
 *       (master branch behavior).</li>
 *   <li><b>compilerLookup</b> — optimized approach: O(1) lookup via
 *       {@link TestCompiler#getControllersForSampler} which reuses the
 *       controller path already computed during compilation.</li>
 * </ul>
 */
@Fork(value = 2, jvmArgsPrepend = {"-Xmx256m"})
@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
@Warmup(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
@State(Scope.Thread)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class ControllerPathLookupBenchmark {

    /** Depth of nested controllers above the target sampler. */
    @Param({"2", "5", "10"})
    int treeDepth;

    /** Number of sibling samplers at each controller level (widens the tree). */
    @Param({"1", "10"})
    int siblingsPerLevel;

    private ListedHashTree testTree;
    private TestCompiler compiler;
    private AbstractSampler targetSampler;

    /** Minimal sampler implementation for benchmarking. */
    private static class BenchmarkSampler extends AbstractSampler {
        private static final long serialVersionUID = 1L;

        BenchmarkSampler(String name) {
            setName(name);
        }

        @Override
        public SampleResult sample(Entry e) {
            return null;
        }
    }

    @Setup
    public void setup() {
        testTree = new ListedHashTree();

        // Build a tree:  root controller -> nested controllers -> samplers
        //   depth=2:  Root -> Controller1 -> [targetSampler, sibling1, ...]
        //   depth=5:  Root -> C1 -> C2 -> C3 -> C4 -> [targetSampler, ...]
        //   depth=10: Root -> C1 -> ... -> C9 -> [targetSampler, ...]

        LoopController root = new LoopController();
        root.setName("RootLoop");
        root.setLoops(1);

        ListedHashTree currentSubTree = (ListedHashTree) testTree.add(root);

        // Create nested controllers
        for (int d = 1; d < treeDepth; d++) {
            GenericController ctrl = new GenericController();
            ctrl.setName("Controller-" + d);

            // Add sibling samplers at this level (to make the tree wider)
            for (int s = 0; s < siblingsPerLevel; s++) {
                BenchmarkSampler sibling = new BenchmarkSampler("Sibling-" + d + "-" + s);
                currentSubTree.add(ctrl, sibling);
            }

            currentSubTree = (ListedHashTree) currentSubTree.add(ctrl);
        }

        // Add the target sampler at the deepest level
        targetSampler = new BenchmarkSampler("TargetSampler");
        currentSubTree.add(targetSampler);

        // Add more siblings at the deepest level
        for (int s = 0; s < siblingsPerLevel; s++) {
            currentSubTree.add(new BenchmarkSampler("DeepSibling-" + s));
        }

        // Compile the tree (populates samplerConfigMap)
        TestCompiler.initialize();
        compiler = new TestCompiler(testTree);
        testTree.traverse(compiler);
    }

    /**
     * OLD approach (master): traverse the full tree every time.
     */
    @Benchmark
    public List<Controller> traversal(Blackhole bh) {
        FindTestElementsUpToRootTraverser traverser =
                new FindTestElementsUpToRootTraverser(targetSampler);
        testTree.traverse(traverser);
        List<Controller> controllers = traverser.getControllersToRoot();
        bh.consume(controllers);
        return controllers;
    }

    /**
     * NEW approach: O(1) lookup via TestCompiler's precomputed map.
     */
    @Benchmark
    public List<Controller> compilerLookup(Blackhole bh) {
        List<Controller> controllers = compiler.getControllersForSampler(targetSampler);
        bh.consume(controllers);
        return controllers;
    }

    public static void main(String[] args) throws RunnerException {
        Options opt = new OptionsBuilder()
                .include(ControllerPathLookupBenchmark.class.getSimpleName())
                .addProfiler(GCProfiler.class)
                .detectJvmArgs()
                .build();
        new Runner(opt).run();
    }
}