Review understandingcollections/jmh/

[Aleksey Shipilev]

Hi,

On 10/22/2016 10:40 PM, Bruce Eckel wrote:
> I've gotten through the first working draft of the JMH tests in the
> "Understanding Collections" chapter and would like feedback about how to
> improve it.
> 
> To try it out, clone:
> https://github.com/BruceEckel/OnJava8-Examples.git
> 
> Then run:
> gradlew understandingcollections:jmh
> 
> The source code is in understandingcollections/jmh/
> 
> Everything seems to work; now I'm interested in tuning it appropriately (or
> fixing any misconceptions I might have).

I'm looking at tests at this path:
 https://github.com/BruceEckel/OnJava8-Examples/blob/master/understandingcollections/jmh/

Stylistic:

 *) Don't use System.exit, just throw the exception from the @Setup
method. JMH will treat that as workload failure better than the abrupt
VM termination. Ditto for println-ing in @Benchmark: just throw the
exception out of @Benchmark.

 *) There is no particular reason to prefer explicit Blackhole rather
than implicit return, if you have only a single result. E.g.:

  @Benchmark
  public void pollFirst(Blackhole bh) {
    bh.consume(deque.pollFirst());
  }

vs.

  @Benchmark
  public String pollFirst() {
    return deque.pollFirst();
  }

Okay, the problematic parts are:

 *) The benchmark like this is deceptive, because it will drain the
deque on the first N invocations, and then you will measure the
performance of polling from empty deque. This is probably not something
you want. Deques, Lists, Queues, are affected.

  @Benchmark
  public String pollFirst() {
    return deque.pollFirst();
  }

 *) Ditto for "add" tests: this workload would measure putting at the
larger and larger queues, which in many cases would mean the performance
would depend on iteration time... again, probably not something you
want. Deques, Lists, Maps, Queues, are affected

  @Benchmark
  public Deque<String> addLast() {
    deque.addLast("test");
    return deque;
  }

The benchmarks without steady state are difficult to do right. See e.g.:
https://shipilev.net/blog/2014/nanotrusting-nanotime/#_steady_state_considerations.
You might want to bite the bullet and repopulate the collection after
you poll it dry + clear when it is full. Or, you might want to turn them
into the steady state benchmarks by measuring add/poll pairs.

 *) For "size() / 2" index checks, are we assuming that the underlying
implementations have simple size()? All four List implementations you
have seem OKay, but this might not hold for any other List.

 *) It is better to precompute all non-measured parts in @Setup, to
avoid this:

  @Benchmark
  public void Sets.contains(Blackhole bh) {
    String key = Integer.toString(size/2); // can do in @Setup
    bh.consume(set.contains(key));
  }


Thanks,
-Aleksey