On Tue, 29 Nov 2022 18:38:34 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>>> I may run it again after the integration of multiplicative inversion and >>> point multiplication improvement. >> >> After the integration of the improvement above, here is the benchmark >> numbers with this patch: >> >> Benchmark (algorithm) (messageLength) Mode Cnt >> Score Error Units >> Signatures.EdDSA.sign Ed25519 64 thrpt 15 >> 1084.556 ± 135.637 ops/s >> Signatures.EdDSA.sign Ed25519 512 thrpt 15 >> 1168.663 ± 25.072 ops/s >> Signatures.EdDSA.sign Ed25519 2048 thrpt 15 >> 1186.863 ± 16.224 ops/s >> Signatures.EdDSA.sign Ed25519 16384 thrpt 15 >> 1095.034 ± 6.462 ops/s >> Signatures.EdDSA.sign Ed448 64 thrpt 15 >> 323.771 ± 2.156 ops/s >> Signatures.EdDSA.sign Ed448 512 thrpt 15 >> 326.995 ± 2.101 ops/s >> Signatures.EdDSA.sign Ed448 2048 thrpt 15 >> 320.799 ± 5.452 ops/s >> Signatures.EdDSA.sign Ed448 16384 thrpt 15 >> 317.715 ± 2.554 ops/s >> Signatures.sign secp256r1 64 thrpt 15 >> 4072.636 ± 22.441 ops/s >> Signatures.sign secp256r1 512 thrpt 15 >> 4048.822 ± 40.769 ops/s >> Signatures.sign secp256r1 2048 thrpt 15 >> 4042.884 ± 20.147 ops/s >> Signatures.sign secp256r1 16384 thrpt 15 >> 3911.856 ± 12.039 ops/s >> Signatures.sign secp384r1 64 thrpt 15 >> 634.203 ± 4.532 ops/s >> Signatures.sign secp384r1 512 thrpt 15 >> 637.623 ± 1.592 ops/s >> Signatures.sign secp384r1 2048 thrpt 15 >> 620.283 ± 10.014 ops/s >> Signatures.sign secp384r1 16384 thrpt 15 >> 622.617 ± 5.695 ops/s >> Signatures.sign secp521r1 64 thrpt 15 >> 311.957 ± 5.420 ops/s >> Signatures.sign secp521r1 512 thrpt 15 >> 316.605 ± 2.204 ops/s >> Signatures.sign secp521r1 2048 thrpt 15 >> 316.700 ± 1.654 ops/s >> Signatures.sign secp521r1 16384 thrpt 15 >> 309.599 ± 7.167 ops/s >> >> >> and the numbers without this patch: >> >> Benchmark (algorithm) (messageLength) Mode Cnt >> Score Error Units >> Signatures.EdDSA.sign Ed25519 64 thrpt 15 >> 1138.578 ± 57.908 ops/s >> Signatures.EdDSA.sign Ed25519 512 thrpt 15 >> 1172.242 ± 17.180 ops/s >> Signatures.EdDSA.sign Ed25519 2048 thrpt 15 >> 1163.793 ± 21.095 ops/s >> Signatures.EdDSA.sign Ed25519 16384 thrpt 15 >> 1093.856 ± 5.964 ops/s >> Signatures.EdDSA.sign Ed448 64 thrpt 15 >> 324.089 ± 2.894 ops/s >> Signatures.EdDSA.sign Ed448 512 thrpt 15 >> 323.580 ± 1.437 ops/s >> Signatures.EdDSA.sign Ed448 2048 thrpt 15 >> 323.680 ± 2.555 ops/s >> Signatures.EdDSA.sign Ed448 16384 thrpt 15 >> 310.641 ± 2.256 ops/s >> Signatures.sign secp256r1 64 thrpt 15 >> 4070.733 ± 27.059 ops/s >> Signatures.sign secp256r1 512 thrpt 15 >> 4061.835 ± 18.776 ops/s >> Signatures.sign secp256r1 2048 thrpt 15 >> 4041.226 ± 19.082 ops/s >> Signatures.sign secp256r1 16384 thrpt 15 >> 3893.323 ± 11.869 ops/s >> Signatures.sign secp384r1 64 thrpt 15 >> 632.924 ± 8.273 ops/s >> Signatures.sign secp384r1 512 thrpt 15 >> 628.807 ± 7.604 ops/s >> Signatures.sign secp384r1 2048 thrpt 15 >> 631.052 ± 1.782 ops/s >> Signatures.sign secp384r1 16384 thrpt 15 >> 530.402 ± 122.967 ops/s >> Signatures.sign secp521r1 64 thrpt 15 >> 316.634 ± 1.724 ops/s >> Signatures.sign secp521r1 512 thrpt 15 >> 315.830 ± 2.333 ops/s >> Signatures.sign secp521r1 2048 thrpt 15 >> 315.855 ± 1.093 ops/s >> Signatures.sign secp521r1 16384 thrpt 15 >> 315.019 ± 1.124 ops/s > > @XueleiFan, I have some questions about this integration. The performance > numbers you last posted showed some very small improvements but also some > regressions in throughput numbers, so its not clear to me if this is worth > integrating yet. Earlier, you said that the performance benefits of this fix > would not be realized until the changes for > https://github.com/openjdk/jdk/pull/10398 were to be integrated, but that > change is still in draft and has comments that have not been resolved. So, is > it possible this was integrated too early? @seanjmullan if you check the benchmark numbers, the throughput impact is limited. For some curves, it is improved; and some others is impacted. In theory of this update, it is not expected to have performance impact if no improvement. The throughput impact in the benchmark is more from the noice in the benchmark platform, I think. The fix for [10398](https://github.com/openjdk/jdk/pull/10398) depends on this update. Only after integrating this one, I can open [10398](https://github.com/openjdk/jdk/pull/10398) for review. The code is ready for [10398](https://github.com/openjdk/jdk/pull/10398) , I'm running the benchmarking. Once the benchmark numbers are ready, I will open it for review. It should be ready to review today. ------------- PR: https://git.openjdk.org/jdk/pull/10624
