VectorAPI: JMH performance testing infrastructure review

[Halimi, Jean-Philippe]

Hi Vladimir,

Here is the updated webrev. I updated the README, deleted the old template files and fixed the array length parameter with a workaround that can be fixed later with your suggested technique.

http://cr.openjdk.java.net/~jphalimi/webrev_jmh_tests_v1.1/webrev/

Let me know if it looks good as is.

Thanks,

Jp

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

Jp,

I didn't intend to block or delay the initial push. Feel free to divide the work in whatever chunks you find appropriate.

> 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?

My comment was motivated by review convenience.
I had to apply your patch and generate the benchmarks first. I'd prefer to just see them as part of webrev (or pre-generated sources published along the webrev) instead.

> 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?

Sure.

> 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?

Sure.

> 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.

I'm also fine with having that as a follow-up patch.

Best regards,
Vladimir Ivanov

> -----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
>>