8333583: Crypto-XDH.generateSecret regression after JDK-8329538

[vpaprotsk]

This fix recovers XDH performance but removes some of the P256 gains (~-8-14%). Still faster, but not as much.

The fix is to undo 'int' return type on mult()/square(), which allowed to return partially reduced result (e.g. this avoids extra reductions when mult() result is fed into addition). This is the behaviour before the Montgomery ECC PR.

---

XDH.generateSecret performance
before Montgomery PR:

Benchmark                             (algorithm)  (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt     Score    Error  Units
KeyAgreementBench.XDH.generateSecret          XDH          255             XDH              thrpt    3  8435.277 ± 27.230  ops/s

after Montgomery PR:

Benchmark                             (algorithm)  (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt     Score    Error  Units
KeyAgreementBench.XDH.generateSecret          XDH          255             XDH              thrpt    3  8309.028 ± 22.071  ops/s

with this PR:

Benchmark                             (algorithm)  (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt     Score    Error  Units
KeyAgreementBench.XDH.generateSecret          XDH          255             XDH              thrpt    3  8491.268 ± 32.858  ops/s

---

P256 performance with/without mult intrinsic:

Performance before Montgomery PR:

Benchmark                        (algorithm)  (dataSize)  (keyLength)  (provider)   Mode  Cnt     Score    Error  Units
SignatureBench.ECDSA.sign    SHA256withECDSA        1024          256              thrpt    3  6398.727 ±  7.400  ops/s
SignatureBench.ECDSA.sign    SHA256withECDSA       16384          256              thrpt    3  6129.739 ±  5.995  ops/s
SignatureBench.ECDSA.verify  SHA256withECDSA        1024          256              thrpt    3  1889.928 ± 54.660  ops/s
SignatureBench.ECDSA.verify  SHA256withECDSA       16384          256              thrpt    3  1866.339 ± 42.438  ops/s
Benchmark                                            (algorithm)  (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt     Score    Error  Units
o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret          ECDH          256              EC              thrpt    3  1350.745 ± 28.514  ops/s
o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret         ECDH          256              EC              thrpt    3  1349.393 ± 32.050  ops/s

Performance in master without mult() intrinsic

Benchmark                        (algorithm)  (dataSize)  (keyLength)  (provider)   Mode  Cnt     Score     Error  Units
SignatureBench.ECDSA.sign    SHA256withECDSA        1024          256              thrpt    3  6539.589 ± 132.844  ops/s
SignatureBench.ECDSA.sign    SHA256withECDSA       16384          256              thrpt    3  6202.530 ± 124.496  ops/s
SignatureBench.ECDSA.verify  SHA256withECDSA        1024          256              thrpt    3  1967.038 ±  15.819  ops/s
SignatureBench.ECDSA.verify  SHA256withECDSA       16384          256              thrpt    3  1931.667 ±  22.901  ops/s
Benchmark                                            (algorithm)  (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt     Score    Error  Units
o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret          ECDH          256              EC              thrpt    3  1354.143 ± 24.861  ops/s
o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret         ECDH          256              EC              thrpt    3  1354.139 ± 21.904  ops/s

Performance in master with mult() intrinsic

Benchmark                        (algorithm)  (dataSize)  (keyLength)  (provider)   Mode  Cnt      Score     Error  Units
SignatureBench.ECDSA.sign    SHA256withECDSA        1024          256              thrpt    3  10534.707 ±  20.690  ops/s
SignatureBench.ECDSA.sign    SHA256withECDSA       16384          256              thrpt    3   9729.246 ± 102.803  ops/s
SignatureBench.ECDSA.verify  SHA256withECDSA        1024          256              thrpt    3   3549.011 ±  77.343  ops/s
SignatureBench.ECDSA.verify  SHA256withECDSA       16384          256              thrpt    3   3458.107 ±  14.622  ops/s
Benchmark                                            (algorithm)  (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt     Score    Error  Units
o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret          ECDH          256              EC              thrpt    3  2563.566 ± 94.381  ops/s
o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret         ECDH          256              EC              thrpt    3  2569.143 ± 53.337  ops/s

THIS PR without mult intrinsic

Benchmark                        (algorithm)  (dataSize)  (keyLength)  (provider)   Mode  Cnt     Score     Error  Units
SignatureBench.ECDSA.sign    SHA256withECDSA        1024          256              thrpt    3  6401.053 ± 125.123  ops/s
SignatureBench.ECDSA.sign    SHA256withECDSA       16384          256              thrpt    3  6152.548 ± 150.399  ops/s
SignatureBench.ECDSA.verify  SHA256withECDSA        1024          256              thrpt    3  1926.609 ±   8.420  ops/s
SignatureBench.ECDSA.verify  SHA256withECDSA       16384          256              thrpt    3  1898.697 ±  28.935  ops/s
Benchmark                                            (algorithm)  (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt     Score    Error  Units
o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret          ECDH          256              EC              thrpt    3  1359.759 ± 24.033  ops/s
o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret         ECDH          256              EC              thrpt    3  1361.320 ± 21.072  ops/s

THIS PR with mult intrinsic

Benchmark                        (algorithm)  (dataSize)  (keyLength)  (provider)   Mode  Cnt     Score     Error  Units
SignatureBench.ECDSA.sign    SHA256withECDSA        1024          256              thrpt    3  9631.846 ±   9.796  ops/s
SignatureBench.ECDSA.sign    SHA256withECDSA       16384          256              thrpt    3  8984.315 ± 159.869  ops/s
SignatureBench.ECDSA.verify  SHA256withECDSA        1024          256              thrpt    3  3116.537 ±  31.496  ops/s
SignatureBench.ECDSA.verify  SHA256withECDSA       16384          256              thrpt    3  3034.870 ±  78.147  ops/s
Benchmark                                            (algorithm)  (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt     Score    Error  Units
o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret          ECDH          256              EC              thrpt    3  2244.094 ± 17.384  ops/s
o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret         ECDH          256              EC              thrpt    3  2249.111 ± 54.525  ops/s

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8333583: Crypto-XDH.generateSecret regression after JDK-8329538 (Bug - P3)

Reviewers

• Sandhya Viswanathan (@sviswa7 - Reviewer)
• Vladimir Kozlov (@vnkozlov - Reviewer)
• Anthony Scarpino (@ascarpino - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19728/head:pull/19728
$ git checkout pull/19728

Update a local copy of the PR:
$ git checkout pull/19728
$ git pull https://git.openjdk.org/jdk.git pull/19728/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19728

View PR using the GUI difftool:
$ git pr show -t 19728

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19728.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back vpaprotsk! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@vpaprotsk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8333583: Crypto-XDH.generateSecret regression after JDK-8329538

Reviewed-by: sviswanathan, kvn, ascarpino

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 241 new commits pushed to the master branch:

• b3bf31a: 8333542: Breakpoint in parallel code does not work
• 86b0cf2: 8334653: ISO 4217 Amendment 177 Update
• 861aefc: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
• f8bf470: 8334810: Redo: Un-ProblemList LocaleProvidersRun and CalendarDataRegression
• 933eaba: 8334629: [BACKOUT] PhaseIdealLoop::conditional_move is too conservative
• 7429c37: 8334598: Default classlist in JDK is not deterministic after JDK-8293980
• 9c89f08: 8334421: assert(!oldbox->is_unbalanced()) failed: this should not be called for unbalanced region
• 57f8b91: 8333658: NMT: Use an allocator with 4-byte pointers to save memory in NativeCallStackStorage
• 6c67933: 8334899: Test runtime/cds/appcds/javaldr/ExceptionDuringDumpAtObjectsInitPhase.java failed after JDK-8306580
• cae94b2: 8334397: RISC-V: verify perf of ReverseBytesS/US
• ... and 231 more: https://git.openjdk.org/jdk/compare/326dbb1b139dd1ec1b8605339b91697cdf49da9a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@sviswa7, @vnkozlov, @ascarpino) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@vpaprotsk The following labels will be automatically applied to this pull request:

• build
• hotspot
• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (960b8333)
• 01: Full - Incremental (2ab7bcbd)
• 00: Full (0219018b)

[vpaprotsk]

@ascarpino Would you mind reviewing this again please? Mostly java you reviewed before.

[magicus]

/label -build

[openjdk]

@magicus
The build label was successfully removed.

[sviswa7]

A minor nit: here NULL could be nullptr instead.

[vpaprotsk]

done, thanks!

[sviswa7]

Looks good to me.

[vnkozlov]

What causes regression in P256 "(~-8-14%)"?
From what I see, you re-arranged code to not execute some code ("reducePositive()") when it is not needed. How this affects P256?

[vpaprotsk]

What causes regression in P256 "(~-8-14%)"? From what I see, you re-arranged code to not execute some code ("reducePositive()") when it is not needed. How this affects P256?

Actually, the other way around; reducePositive is now an unconditionally executed for both pure java and the intrinsic paths. Perhaps that's what is misleading, it was only the mult() intrinsic that was taking advantage of this 'skip reduction' before. (pure java did not benefit from removing reduction, so I kept it. Now 'keeping it' for both paths)

[ascarpino]

Hi @vpaprotsk,
@ferakocz is going to take a look at the change. When he says it's ok, I'll approve the PR.

[vnkozlov]

Actually, the other way around; reducePositive is now an unconditionally executed for both pure java and the intrinsic paths.

Looking on MontgomeryIntegerPolynomialP256.java the code in multImpl() + reducePositive() is similar to original mult() except new additional code at the end of multImpl(). Now you intrinsify only multImpl(). Looks like reducePositive()is not included into intrinsic and will be normally JIT compiled (hopeful inlined when JIT compiling mult(). Then what do you mean in above statement?

Also you did not change assembler for intrinsic but you changed corresponding Java code (multImpl()). How it works?

[vpaprotsk]

Looking on MontgomeryIntegerPolynomialP256.java the code in multImpl() + reducePositive() is similar to original mult() except new additional code at the end of multImpl().

Yep, I split the original java mult() into multImpl() and reducePositive().

Now you intrinsify only multImpl(). Looks like reducePositive()is not included into intrinsic and will be normally JIT compiled (hopeful inlined when JIT compiling mult(). Then what do you mean in above statement?
Also you did not change assembler for intrinsic but you changed corresponding Java code (multImpl()). How it works?

The intrinsic used to return 1 (i.e. numAdds = 1), which would let the next operation decide if it needed to do the reduction or skip it. Now reducePositive() reduction always happens after the intrinsic (when it could had been skipped before).

[vnkozlov]

Let me know that I got it right:

• The reduction operation was optional and P256 benefitted by not executing it.
• Previous mult() Java code always retuned 0 because it executes reduction so callers do not need to do it.
• _intpoly_montgomeryMult_P256 intrinsic code executes only part of code from previous mult() and it returns 1 to indicate that reduction should be executed if needed.
• Now mult() is split into 2 methods (with multImpl() intrinisfied) and always executes reduction so it can return 0.

I like new implementation because intrinsic matches Java code. It would allow avoid confusion I had.

The only question left: do we need to do something about Java code which checks return value? It is always 0 now. And I don't see you changed such checks.

[vpaprotsk]

Let me know that I got it right:

• The reduction operation was optional and P256 benefitted by not executing it.
• Previous mult() Java code always retuned 0 because it executes reduction so callers do not need to do it.
• _intpoly_montgomeryMult_P256 intrinsic code executes only part of code from previous mult() and it returns 1 to indicate that reduction should be executed if needed.
• Now mult() is split into 2 methods (with multImpl() intrinisfied) and always executes reduction so it can return 0.

Thats it exactly. Except I would correct the last two words return 0. It is now void so no return (and I imagine that is why XDH did not like it; having it hardcoded to 0, without having to do inlining, opens the doors for some more optimizations. Also, the code I had checked in as part of montgomery PR was returning 0 everywhere but the intrinsic.

I like new implementation because intrinsic matches Java code. It would allow avoid confusion I had.

I disliked this too. I originally removed the Java reduction too, but it hurt the non-intrinsic performance, so put it back in. (Before I got distracted with this bug, I was actually working on next ECC iteration, and was trying to fix this mismatch. But I also hadn't realized how much this optimization actually helped.)

There is also a 'bigger' complaint.. this optimization tried to use virtual methods to specialize one particular curve. Fairly standard practice. And it brought the other 'unaffected' curve down. If I can't use virtual methods for further optimizations.. how am I supposed to optimize further? Hmm. Not the time to discuss an answer, this release is going out, not the time to get 'creative', but this will give me problems next time I try to add code here.

The only question left: do we need to do something about Java code which checks return value? It is always 0 now. And I don't see you changed such checks.

(Correction: no return, void). numAdds is now again pretty much a 'private' concept to the IntegerPolynomial class, so figure it was fine before, it should be fine now?

[vnkozlov]

numAdds is now again pretty much a 'private' concept to the IntegerPolynomial class, so figure it was fine before, it should be fine now?

I did not mean it for this changes but as general improvement of code in other RFE. But it is up to core libs group to decide.

[vnkozlov]

Talking about future improvements. Is it possible to optimize reduction code by converting it to intrinsic too? Or code generated by C2 is good enough?

[vpaprotsk]

Talking about future improvements. Is it possible to optimize reduction code by converting it to intrinsic too? Or code generated by C2 is good enough?

I had some experiments to try where I was using virtual methods to add optimizations, similar to the optimization here (i.e. the default method 'does nothing' and have just one override).

Perhaps this issue could had been solved differently and there is something to do on the compiler side i.e. requires a specific order of optimizations.. specialize the IntegerPolynomial.setProduct() hot path for XDH field type, inline mult() from XDH field, realize that the return is always zero, which allows whatever optimizations that werent run for 4% performance. (I don't yet know enough about the C2 to be able to answer or 'fix' that)

[vnkozlov]

There are examples in C2 how to check method's class holder (intrinsic's predicate) before executing intrinsic code.
See, for example, code for _counterMode_AESCrypt in library_call.cpp. I am not sure is this what you are asking for.

[vnkozlov]

Approved for VM changes.

[vnkozlov]

@TobiHartmann ran our testing and it passed.

[vpaprotsk]

Thanks @vnkozlov @TobiHartmann !

[vpaprotsk]

@ferakocz just tagging you as reminder of (the many) items in your queue :)
Thanks!

[ferakocz]

@ferakocz just tagging you as reminder of (the many) items in your queue :) Thanks!

Sorry, I was out of office last week. I will take a deeper look at the changes tomorrow, but I have a question based on my first look at it: Do you attribute the performance loss of the XDH code path to the mult() function returning an int instead of being void? Do you think that this prevented some optimization in the hotspot compiler?

[vpaprotsk]

@ferakocz, now I was out on long weekend...

Do you attribute the performance loss of the XDH code path to the mult() function returning an int instead of being void? Do you think that this prevented some optimization in the hotspot compiler?

That's exactly it. I 'proved experimentally' that that's the case. Though I haven't identified which exact sequence of optimizations is missing deterministically from compilation logs. That's beyond me yet. Identifying which optimization(s) is missing might be great for long term, but figured since we are closing down commits for this release, I should put something in soonest. This PR essentially 'reverts' the part of my ECC PR to original code. Which in turn should be easiest to review.

[ferakocz]

Looks good to me. It would be good, though, to figure out what else could be done to regain the P256 performance with keeping the speed of this code path.

[ferakocz]

@ascarpino please approve this change.

[ascarpino]

Approved with review by @ferakocz

[vpaprotsk]

Thanks @ferakocz @ascarpino

/integrate

[openjdk]

@vpaprotsk
Your change (at version 960b833) is now ready to be sponsored by a Committer.

[sviswa7]

/sponsor

[openjdk]

Going to push as commit f101e15.
Since your change was applied there have been 241 commits pushed to the master branch:

• b3bf31a: 8333542: Breakpoint in parallel code does not work
• 86b0cf2: 8334653: ISO 4217 Amendment 177 Update
• 861aefc: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
• f8bf470: 8334810: Redo: Un-ProblemList LocaleProvidersRun and CalendarDataRegression
• 933eaba: 8334629: [BACKOUT] PhaseIdealLoop::conditional_move is too conservative
• 7429c37: 8334598: Default classlist in JDK is not deterministic after JDK-8293980
• 9c89f08: 8334421: assert(!oldbox->is_unbalanced()) failed: this should not be called for unbalanced region
• 57f8b91: 8333658: NMT: Use an allocator with 4-byte pointers to save memory in NativeCallStackStorage
• 6c67933: 8334899: Test runtime/cds/appcds/javaldr/ExceptionDuringDumpAtObjectsInitPhase.java failed after JDK-8306580
• cae94b2: 8334397: RISC-V: verify perf of ReverseBytesS/US
• ... and 231 more: https://git.openjdk.org/jdk/compare/326dbb1b139dd1ec1b8605339b91697cdf49da9a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@sviswa7 @vpaprotsk Pushed as commit f101e15.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[vpaprotsk]

/backport :jdk23

[openjdk]

@vpaprotsk To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

[TobiHartmann]

/backport jdk:jdk23

[openjdk]

@TobiHartmann the backport was successfully created on the branch backport-TobiHartmann-f101e153-jdk23 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk23, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit f101e153 from the openjdk/jdk repository.

The commit being backported was authored by Volodymyr Paprotski on 25 Jun 2024 and was reviewed by Sandhya Viswanathan, Vladimir Kozlov and Anthony Scarpino.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk:

$ git fetch https://github.com/openjdk-bots/jdk.git backport-TobiHartmann-f101e153-jdk23:backport-TobiHartmann-f101e153-jdk23
$ git checkout backport-TobiHartmann-f101e153-jdk23
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk.git backport-TobiHartmann-f101e153-jdk23

⚠️ @TobiHartmann You are not yet a collaborator in my fork openjdk-bots/jdk. An invite will be sent out and you need to accept it before you can proceed.