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.

[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();
    }
}

[rajat315315]

Request to re-review..
@vlsi

[vlsi]

Thanks for looking into this — reusing the path TestCompiler already computed is the right instinct, and the tree walk it removes is genuinely redundant.

The description does not match the diff

The description explains a parentControllersCache in JMeterThread, a nested CachedPathToRootTraverser, and a HashMap<Sampler, List<Controller>>. None of that is in the diff, which instead exposes the existing SamplePackage.controllers through TestCompiler. The benchmark numbers therefore measured different code than what is proposed here. Could you rewrite the description around the actual change and re-run the measurement against it? JMeter has a JMH module at src/core/src/jmh — a benchmark there would be more convincing than a one-off harness.

Correctness

I walked through the equivalence and it holds:

• JMeterThread builds compiler = new TestCompiler(testTree) and traverses the same testTree in initRun, so both paths see one tree and one traversal order.
• Order matches: saveSamplerConfigs iterates for (int i = stack.size(); i > 0; i--), sampler to root, and getControllersToRoot() returns leaf to root. That matters — breakOnCurrentLoop stops at the first IteratingController.
• Identity matches: samplerConfigMap is an IdentityHashMap, and the traverser compares node == nodeToFind.
• After findRealSampler, the sampler has always been through configureSampler (which would have thrown NPE otherwise), so the map lookup cannot miss on the live path.

One behavioral difference worth calling out in the description: if the same Sampler instance appears twice in the tree, the traverser keeps the first occurrence (stopRecording) while samplerConfigMap.put keeps the last. Obscure, but it changes silently.

The empty-list case deserves a log line

Returning List.of() on a map miss is the right call, but it should not be silent.

A miss is reachable in a degraded state, and it is worth tracing where the failure lands. executeSamplePackage calls compiler.configureSampler(current), which NPEs on a missing package. That NPE is caught inside processSampler and logged as Error while processing sampler, and the thread keeps going. Execution then reaches the onErrorStartNextLoop branch in run() and calls triggerLoopLogicalActionOnParentControllers — which sits outside that try block. So an exception thrown from getControllersForSampler propagates to catch (Exception | JMeterError e) in run() and kills the thread.

That is a worse outcome than today's behavior, where the loop action is skipped but the thread survives, and the underlying error has already been reported by the earlier catch. Turning a degraded loop into a dead load-generator thread is a behavior change that does not belong in a performance PR.

I would keep the lookup total and put the invariant check at the call site:

List<Controller> controllers = compiler.getControllersForSampler(realSampler);
if (controllers.isEmpty()) {
    log.warn("No parent controllers found for sampler '{}', loop action is skipped", realSampler.getName());
}
consumer.accept(controllers);

This keeps the new public API total — an unknown sampler maps to an empty list, and callers pick their own failure policy — while making the anomaly greppable. If maintainers prefer fail-fast, IllegalStateException matching the realSampler == null check a few lines above is the right shape, but it should land as a separate commit so the behavior change gets reviewed as one.

API surface

SamplePackage.getControllers() returns the live mutable list. That is consistent with getConfigs() and getTimers(), so it is not a blocker, but this is a new public method on a public class and we are committing to it permanently. TestCompiler lives in the same org.apache.jmeter.threads package, so a package-private getter would do the job. If it stays public, please add @API(status = ..., since = ...) — that is the convention here.

Also: xdocs/changes.xml needs an entry, and after this change FindTestElementsUpToRootTraverser has no callers left in the codebase. Worth noting in the description; deprecating it can be a separate discussion.

Generics

Consumer<List<Controller>> drops the ? super the previous signature had. Since the method feeds the consumer, PECS asks for:

Consumer<? super List<Controller>> consumer

At the three current call sites this makes no practical difference — they are method references, and inference handles them either way. But it costs nothing and restores the original variance, so I would rather keep it.

The return type of getControllersForSampler is correct as List<Controller>; a wildcard there would only inconvenience callers.

Tests

This is my main ask. TestJMeterThread has no coverage for break, continue, or onErrorStartNextLoop today, so the change moves untested logic onto a new data source and rests entirely on "both paths produce the same list." Let's pin that claim down.

1. Equivalence test — cheapest and highest value. Build a tree, traverse it with both TestCompiler and FindTestElementsUpToRootTraverser, and assert assertIterableEquals(traverser.getControllersToRoot(), compiler.getControllersForSampler(sampler)). Order, not set membership. Parameterize over tree shapes: sampler directly under the thread group; LoopController → GenericController → IfController nesting; sampler under a TransactionController, including nested transactions; non-controllers (config, timer, assertion) interleaved, to confirm they stay out of the list; two samplers sharing a parent; a sampler absent from the tree, which should yield an empty list rather than throw.

2. Behavioral tests through JMeterThread. testBug63490EndTestWhenDelayIsTooLongForScheduler already gives you the harness: build a tree, run new TestCompiler plus traverse, construct a JMeterThread, call run(). On top of that, add a spy controller that records startNextLoop, breakLoop, and triggerEndOfLoop calls, then drive a failing sampler with onErrorStartNextLoop and assert the same controllers fire in the same order. One test per TestLogicalAction value (BREAK_CURRENT_LOOP, START_NEXT_ITERATION_OF_THREAD, START_NEXT_ITERATION_OF_CURRENT_LOOP), set from a PostProcessor via threadContext.setTestLogicalAction(...). Plus a nested-loop case: a break in the inner loop must leave the outer one alone.

3. TransactionController (bug 52968). This is the riskiest path, because findRealSampler unwraps the TransactionSampler down to the child sampler, whose package comes from samplerConfigMap rather than transactionControllerConfigMap. bin/testfiles/Bug52968.jmx covers it at the batch level and is wired into src/dist-check/build.gradle.kts — please confirm it passes. A JUnit test is still more useful, since it shows which controllers received triggerEndOfLoop instead of only that the end result matched.

Item 1 is what guards the specific thing this PR could break. Items 2 and 3 close a coverage gap that predates the PR, so they are a fair ask but not strictly yours to fix.