VectorAPI: JMH performance testing infrastructure review

[Halimi, Jean-Philippe]

Hi Vladimir,

Thanks for your feedback. I think all your points are valid. As for the webrev not including the benchmark sources, I think we discussed about it earlier and we concluded that we would add support for JMH tests generation, but that we would not include the sources in the panama codebase. Is there something I missed here?

As for including a maven project, I am not exactly sure how to do that. Do you think it could be possible to do that as part of a follow-up patch, and merge the patch as is or is it a blocker?

As for the initialization phase, I think that it would require some design change considering the kernel templates currently have this code portion embedded. Once again, do you think it would be OK to merge it as is and do a follow-up patch?

As for the size of the array, I think you are right and this needs to be fixed. I'll publish a follow-up webrev in the coming days.

Thanks,

Jean-Philippe

-----Original Message-----
From: Vladimir Ivanov [mailto:vladimir.x.ivanov at oracle.com] 
Sent: Friday, August 17, 2018 11:26 PM
To: Halimi, Jean-Philippe <jean-philippe.halimi at intel.com>; panama-dev at openjdk.java.net
Subject: Re: VectorAPI: JMH performance testing infrastructure review

Hi Jp,

Nice work!

Some comments:

The webrev doesn't include benchmark sources.

======

   public static void main(String[] args) throws RunnerException {
       Options opt = new OptionsBuilder()
               .include(Byte128VectorTestsPerf.class.getSimpleName())
               .forks(1)
               .build();
       new Runner(opt).run();
   }

I'd prefer to see main() replaced with annotations and standard JMH launcher to be used. In my experience, it's much more convenient.

Having a Maven project pre-generated would be a big plus as well.

======

     static final int INVOC_COUNT =
Integer.getInteger("jdk.incubator.vector.test.loop-iterations", 100);

There's @Param("100") available. If JMH launcher is used, default value can be overriden with "-p INVOC_COUNT=value".

======

     @Benchmark
     static void addByte128VectorTests() {
         byte[] a = fa.apply(SPECIES.length());
         byte[] b = fb.apply(SPECIES.length());
         byte[] r = new byte[a.length];

Source/destination arrays allocation/initialization is part of the benchmark. Why not move it to setup step (method marked with @Setup(Level.Iteration)), cache arrays into instance fields, and reuse them in the benchmarks?

======

     static final IntFunction<byte[]> fa = (length) -> {
       byte[] arr = new byte[length];
...
     static void addByte128VectorTests() {
         byte[] a = fa.apply(SPECIES.length()); ...
         for (int ic = 0; ic < INVOC_COUNT; ic++) {
             for (int i = 0; i < a.length; i += SPECIES.length()) {
                 ByteVector<Shapes.S128Bit> av = SPECIES.fromArray(a, i);
                 ByteVector<Shapes.S128Bit> bv = SPECIES.fromArray(b, i);
                 av.add(bv).intoArray(r, i);
             }
         }

Arrays are precisely the size of vector. So, inner loop has only 1 iteration. I'd prefer to see INVOC_COUNT replaced with a parameter controlling the size of the arrays.

Also, it's important to control data size to be able to exclude L1/2/3 cache effects from consideration.

Best regards,
Vladimir Ivanov

On 13/08/2018 18:41, Halimi, Jean-Philippe wrote:
> Dear all,
> 
> I have uploaded a webrev allowing automatic generation of VectorAPI JMH performance test source files along with the regular unit tests that are already generated by the VectorAPI testing infrastructure.
> 
> I had to modify the template files and the core scripting that handles 
> the source generation. Please let me know what your thoughts are! :)
> 
> You can find the webrev here: 
> http://cr.openjdk.java.net/~jphalimi/webrev_jmh_tests_v1.0/webrev/
> 
> The JMH performance test files have not been tested, but should work with Maven.
> 
> Thank you,
> 
> Jp
>