Replace AtomicInteger with int in TestTemplateTestDescriptor.execute()

[gaganis]

An atomic integer is not really required to count the invocationIndex.
A local int variable should shuffice. Rewrite using java5 for-each and
while loop instead of lambdas to remove the need for an effectively
final mutable number.

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes
• All continuous integration builds pass

[marcphilipp]

We will merge the branch issues/14-jupiter-params into master soon. In commit d933781, I've changed the implementation of this method so I'd rather not merge this pull request at this time.

[gaganis]

👍

I will revisit this once issues/14-jupiter-params is merged.

[marcphilipp]

#728 has now been merged to master.

[gaganis]

Redid this on top of the new master.

[marcphilipp]

To me, the old implementation looks simpler and more elegant. Usage of AtomicInteger being a minor hindrance.

@junit-team/junit-lambda What do you think?

[marcphilipp]

The stream should be used within a try-with-resources statement to ensure it's closed in case odf an exception.

[sormuras]

I'm with @marcphilipp here.

If I converted the lambda-chain to a loop-construct, I'd used a plain for-i loop here emphasizing on the importance of the index variable.

[marcphilipp]

Out of curiosity I did a micro-benchmark:

import static org.openjdk.jmh.annotations.Scope.Benchmark;

import java.util.concurrent.atomic.AtomicInteger;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.State;

public class MyBenchmark {

    @State(Benchmark)
    public static class MyState {
        private int primitiveCounter = 0;
        private AtomicInteger atomicCounter = new AtomicInteger();
        private MutableInteger mutableCounter = new MutableInteger();
    }

    @Benchmark
    public int primitiveCounter(MyState state) {
        ++state.primitiveCounter;
        return state.primitiveCounter;
    }

    @Benchmark
    public int atomicCounter(MyState state) {
        state.atomicCounter.incrementAndGet();
        return state.atomicCounter.get();
    }

    @Benchmark
    public int mutableCounter(MyState state) {
        state.mutableCounter.incrementAndGet();
        return state.mutableCounter.get();
    }

    private static class MutableInteger {
        private int value;

        int incrementAndGet() {
            return ++value;
        }

        int get() {
            return value;
        }
    }
}

Unsurprisingly, using a primitive counter is the fastest, but the mutableCounter example comes very close:

Benchmark                      Mode  Cnt          Score          Error  Units
MyBenchmark.atomicCounter     thrpt   10   91191927.400 ±  1269340.171  ops/s
MyBenchmark.mutableCounter    thrpt   10  382869639.833 ± 10470678.543  ops/s
MyBenchmark.primitiveCounter  thrpt   10  419642909.800 ± 12674982.268  ops/s

91 million ops/s is still fast enough, though. Thus, it's certainly not a performance bottleneck.

[sbrannen]

91 million ops/s is still fast enough, though. Thus, it's certainly not a performance bottleneck.

I agree, and I prefer the use of the stream over the for-loop.

If we change anything there, it should be a switch from AtomicInteger to Marc's MutableInteger.

[gaganis]

My motivation for this was AtomicInteger is a class to be used when an application is trying to achieve atomic semantics in a concurrency context.

Otherwise it can be misleading as it happened with @LiamClark in #723 (comment).

I was definitely not trying to optimize.

---

On the other hand I fell victim to Esclation of commitment for this.

On the previous version of master my change was not that bad in readability but @marcphilipp's new lambda is pretty neat and very readable(:+1:) so my change is much worse now.

Closing this.

[jbduncan]

@gaganis It was probably me who set the precedent of using AtomicInteger in situations like this, because I initially used it to avoid the less readable alternative of using a single-element int[] when I was implementing Assertions.assertIterableEquals.

So basically, you can blame/tease me about it. 😜

[gaganis]

@jbduncan I am not fond of blaming so you need not worry about that :)

[bmalinowsky]

@marcphilipp [jmh micro benchmark] (little off-topic to OP) In fairness, the atomic counter method can use return state.atomicCounter.incrementAndGet();. This still consumes the result (produces side effect), and catches up on the other 2 methods already being optimized. Should give a >30 to ~50% improvement in test result (and matches code in question AFAICT).

[marcphilipp]

Updated benchmark:

import static org.openjdk.jmh.annotations.Scope.Benchmark;

import java.util.concurrent.atomic.AtomicInteger;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.State;

public class MyBenchmark {

    @State(Benchmark)
    public static class MyState {
        private int primitiveCounter = 0;
        private AtomicInteger atomicCounter = new AtomicInteger();
        private MutableInteger mutableCounter = new MutableInteger();
    }

    @Benchmark
    public int primitiveCounter(MyState state) {
        return ++state.primitiveCounter;
    }

    @Benchmark
    public int atomicCounter(MyState state) {
        return state.atomicCounter.incrementAndGet();
    }

    @Benchmark
    public int mutableCounter(MyState state) {
        return state.mutableCounter.incrementAndGet();
    }

    private static class MutableInteger {
        private int value;

        int incrementAndGet() {
            return ++value;
        }
    }
}

Results:

Benchmark                      Mode  Cnt          Score         Error  Units
MyBenchmark.atomicCounter     thrpt   10  132394296.905 ± 4283142.877  ops/s
MyBenchmark.mutableCounter    thrpt   10  391508615.532 ± 9909552.468  ops/s
MyBenchmark.primitiveCounter  thrpt   10  416481646.276 ± 7232553.412  ops/s