LantaoJin commented on PR #16807: URL: https://github.com/apache/lucene/pull/16807#issuecomment-6137934352
> A new data point with AVX 512, interestingly this is totally stupid. Repeated the comparison several times. With this PR it is SLOWER! > > Without PR: > > ``` > Benchmark (size) Mode Cnt Score Error Units > VectorUtilBenchmark.mixingUintVsIntScalar 1024 thrpt 15 0.699 ± 0.002 ops/us > VectorUtilBenchmark.mixingUintVsIntVector 1024 thrpt 75 5.603 ± 0.802 ops/us > ``` > > With PR: > > ``` > Benchmark (size) Mode Cnt Score Error Units > VectorUtilBenchmark.mixingUintVsIntScalar 1024 thrpt 15 0.700 ± 0.003 ops/us > VectorUtilBenchmark.mixingUintVsIntVector 1024 thrpt 75 2.067 ± 0.022 ops/us > ``` > > No idea how to explain that. Interestingly the benchmark is very unstable, so it looks like thermal throttling is involved on this AMD cpu....: AMD Ryzen 7 7700 8-Core Processor > > INFO: Java vector incubator API enabled; uses preferredBitSize=512; FMA enabled I reran #16818 before and after this change, on x86 (Xeon Platinum 8175M with `-XX:UseAVX=3`, so 512-bit like your Ryzen) and on aarch64 (Neoverse-N1), with `-prof gc` and `-XX:CompileCommand=PrintInlining` on the JMH stub. I can reproduce your result, but it is not the kernels that got slower: | size 1024, ops/us | x86 512-bit, before | x86 512-bit, after | aarch64, before | aarch64, after | |---|---|---|---|---| | each kernel alone (dot / uint8 dot / square / uint8 square) | 12.3 / 12.4 / 9.6 / 9.5 | 12.1 / 11.9 / 9.5 / 9.8 | 2.25 / 2.23 / 1.84 / 1.84 | 2.23 / 2.22 / 1.84 / 1.84 | | `mixingUintVsIntVector` | 2.17 ± 0.58 | 0.93 ± 0.004 | 0.008 | 0.507 | | `mixingUintVsIntVector`, bytes allocated per op | 1,286 ± 1,869 | 3,088 | 252,036 | 0.07 | On that x86 machine the four kernels alone add up to about 2.7 ops/us, so before this change the mixed benchmark ran at the right speed in 4 of 5 forks (2.55-2.63) and at 0.67 in the fifth, which is the instability you saw. After this change it runs at 0.93 in every fork and allocates 3 KB per call. `PrintInlining` shows why: every kernel is now small enough to inline, so C2 inlines all four into the benchmark stub, and in the steady-state compilation it runs out of its node budget in the middle of the first kernel's loop (`PanamaVectorUtilSupport$ArrayLoader::load ... failed to inline: NodeCountInliningCutoff`) and does not inline the other three at all. The `load` that is not inlined returns a boxed `ByteVector` to the loop on every iteration. Before this change, `dotProductBody512` had already been compiled on its own as a big method and was not inlined, which kept the stub under the cutoff. On aarch64 the cutoff is hit too, but at the fourth kernel, which then is an ordinary c all, so nothing is boxed and the mixed benchmark equals the sum of the four kernels. So `mixingUintVsIntVector` measures where C2 runs out of budget when four Vector API kernels share one compilation, and this change moves that point. A variant that calls each kernel from its own compiled method does not have that problem and still catches the bug; I posted it on https://github.com/apache/lucene/pull/16818#pullrequestreview-5498647361. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
