8297379: Enable the ByteBuffer path of Poly1305 optimizations #11338

vpaprotsk · 2022-11-23T23:33:32Z

There is now an intrinsic for Poly1305, which is only enabled on the engineUpdate([]byte) path. This PR adds intrinsic support engineUpdate(ByteBuffer) (when the bytebuffer hasArray).

Fuzzing test expanded to also include ByteBuffer payloads.

Performance is now matched:

Benchmark                         (dataSize)  (provider)   Mode  Cnt        Score        Error  Units
Poly1305DigestBench.digestBuffer          64              thrpt    3  3320909.857 ± 787300.545  ops/s
Poly1305DigestBench.digestBuffer         256              thrpt    3  3006447.324 ± 704790.796  ops/s
Poly1305DigestBench.digestBuffer        1024              thrpt    3  2645041.509 ± 664904.622  ops/s
Poly1305DigestBench.digestBuffer       16384              thrpt    3   893389.209 ±   6288.743  ops/s
Poly1305DigestBench.digestBuffer     1048576              thrpt    3    14932.680 ±    170.238  ops/s
Poly1305DigestBench.digestBytes           64              thrpt    3  3548298.515 ± 859964.530  ops/s
Poly1305DigestBench.digestBytes          256              thrpt    3  3083261.994 ± 141802.417  ops/s
Poly1305DigestBench.digestBytes         1024              thrpt    3  2704357.584 ± 554683.019  ops/s
Poly1305DigestBench.digestBytes        16384              thrpt    3   898913.339 ±  99733.295  ops/s
Poly1305DigestBench.digestBytes      1048576              thrpt    3    14961.872 ±     38.003  ops/s
Finished running test 'micro:org.openjdk.bench.javax.crypto.full.Poly1305DigestBench'

Relates to:

8297972: Poly1305 Endianness on ByteBuffer not enforced #11463: Found inconsistency between processing []byte and ByteBuffer. When that one is fixed, Poly1305IntrinsicFuzzTest.java should not be setting the endianness on the ByteBuffer
Intrinsic introduced by 8288047: Accelerate Poly1305 on x86_64 using AVX512 instructions #10582.

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-8297379: Enable the ByteBuffer path of Poly1305 optimizations

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer) ⚠️ Review applies to 14726d7b
Anthony Scarpino (@ascarpino - Reviewer)
Jamil Nimeh (@jnimeh - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11338

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

Using diff file

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

bridgekeeper · 2022-11-23T23:34:46Z

👋 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 · 2022-11-23T23:37:54Z

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

security

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

mlbridge · 2022-11-23T23:45:41Z

Webrevs

jatin-bhateja · 2022-11-24T18:54:54Z

Please add a JMH micro showcasing gains, with heap allocated and native byte buffer (once you support it).

jatin-bhateja · 2022-11-24T18:42:01Z

src/java.base/share/classes/com/sun/crypto/provider/Poly1305.java

+            processMultipleBlocks(input, offset, blockMultipleLength, aLimbs, rLimbs);
+        } else {
+            while (blockMultipleLength > 0) {
+                processBlock(buf, BLOCK_LENGTH);


For native byte buffers, you can pass the buffer address and base.

Spent some time thinking about this one.. Originally, (from the sample you shared) I thought I had to use Unsafe, which I don't think is 'safe' to do in a crypto library. After some searching, I found a 'safe' alternative, sun.nio.ch.DirectBuffer interface, which would give me the long address().

The problem is that the current java signature for the intrinsic expects a []byte not a long. I could certainly change the existing intrinsic (or more likely add a new instrinsic calling existing stub assembly).

I would lean towards not supporting Direct Byte buffers right now. (Also, it doesn't seem like other crypto intrinsics do either. At least GCM does not..)

JMH also added (and main PR comment modified to match)

sviswa7 · 2022-11-29T01:16:28Z

src/java.base/share/classes/com/sun/crypto/provider/Poly1305.java

+    private void processMultipleBlocks(ByteBuffer buf, int blockMultipleLength) {
+        if (buf.hasArray()) {
+            byte[] input = buf.array();
+            int offset = buf.arrayOffset();


arrayOffset returns the offset of the first element of the backing buffer. Need to also take into account the position() of the buffer here. We should also add test case for this.

Thanks for catching this!! Fixed the mainline to handle position(). I have now reworked the test case quite a bit.

sviswa7 · 2022-12-01T23:28:13Z

The PR looks good to me.
@ascarpino Could you please help review this?

openjdk · 2022-12-01T23:31:17Z

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

8297379: Enable the ByteBuffer path of Poly1305 optimizations

Reviewed-by: sviswanathan, ascarpino, jnimeh

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 18 new commits pushed to the master branch:

1e46832: 8297602: Compiler crash with type annotation and generic record during pattern matching
b0e5432: 8297687: new URI(S,S,S,S) throws exception with incorrect index position reported in the error message
2243646: 8298145: Remove ContiguousSpace::capacity
84b927a: 8296024: Usage of DirectBuffer::address should be guarded
a9e6c62: 8297186: G1 triggers unnecessary full GCs when heap utilization is low
4458de9: 8297172: Fix some issues of auto-vectorization of Long.bitCount/numberOfTrailingZeros/numberOfLeadingZeros()
a613998: 8297689: Fix incorrect result of Short.reverseBytes() call in loops
f8f4630: 8297963: Partially fix string expansion issues in UTIL_DEFUN_NAMED and related macros
2a243a3: 8267617: Certificate's IP x509 NameConstraints raises ArrayIndexOutOfBoundsException
923c746: 8298057: (fs) Remove PollingWatchService.POLLING_INIT_DELAY
... and 8 more: https://git.openjdk.org/jdk/compare/8af6e8a67fc0355f11c270c3ea794366741856fa...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, @ascarpino, @jnimeh) 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).

sviswa7 · 2022-12-05T18:22:00Z

@valeriepeng Could you please take a look at this PR?

jnimeh · 2022-12-05T18:39:26Z

@sviswa7 I will be looking at this today

ascarpino · 2022-12-05T21:49:25Z

The changes look ok to me

vpaprotsk · 2022-12-05T22:01:19Z

(The build failures do not look related to this PR, trying to see if fixed in main branch)

jnimeh

Changes look fine. tier1-5 + hs-precheckin-comp test jobs came back clean with the exception of two jaxp failures only on linux-x64 but they don't appear to be related to your changes.

vpaprotsk · 2022-12-06T16:39:50Z

/integrate

openjdk · 2022-12-06T16:43:17Z

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

sviswa7 · 2022-12-06T16:45:20Z

/sponsor

openjdk · 2022-12-06T16:47:57Z

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

1e46832: 8297602: Compiler crash with type annotation and generic record during pattern matching
b0e5432: 8297687: new URI(S,S,S,S) throws exception with incorrect index position reported in the error message
2243646: 8298145: Remove ContiguousSpace::capacity
84b927a: 8296024: Usage of DirectBuffer::address should be guarded
a9e6c62: 8297186: G1 triggers unnecessary full GCs when heap utilization is low
4458de9: 8297172: Fix some issues of auto-vectorization of Long.bitCount/numberOfTrailingZeros/numberOfLeadingZeros()
a613998: 8297689: Fix incorrect result of Short.reverseBytes() call in loops
f8f4630: 8297963: Partially fix string expansion issues in UTIL_DEFUN_NAMED and related macros
2a243a3: 8267617: Certificate's IP x509 NameConstraints raises ArrayIndexOutOfBoundsException
923c746: 8298057: (fs) Remove PollingWatchService.POLLING_INIT_DELAY
... and 8 more: https://git.openjdk.org/jdk/compare/8af6e8a67fc0355f11c270c3ea794366741856fa...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-06T16:48:25Z

@sviswa7 @vpaprotsk Pushed as commit 203251f.

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

ByteBuffer support and tests

589ace9

openjdk bot added the security label Nov 23, 2022

whitespace

b31d790

openjdk bot added the rfr label Nov 23, 2022

jatin-bhateja reviewed Nov 24, 2022

View reviewed changes

sviswa7 reviewed Nov 29, 2022

View reviewed changes

vpaprotsk added 3 commits November 29, 2022 16:52

Merge remote-tracking branch 'origin/master' into avx512-poly-buf

1ba40a9

bench and handle buf.position()

e307e34

remove comment

14726d7

vpaprotsk mentioned this pull request Dec 1, 2022

8297972: Poly1305 Endianness on ByteBuffer not enforced #11463

Closed

3 tasks

sviswa7 approved these changes Dec 1, 2022

View reviewed changes

openjdk bot added the ready label Dec 1, 2022

Merge remote-tracking branch 'origin/master' into avx512-poly-buf

a87297f

ascarpino approved these changes Dec 5, 2022

View reviewed changes

jnimeh approved these changes Dec 6, 2022

View reviewed changes

openjdk bot added the sponsor label Dec 6, 2022

openjdk bot added the integrated label Dec 6, 2022

openjdk bot closed this Dec 6, 2022

openjdk bot removed ready rfr sponsor labels Dec 6, 2022

vpaprotsk deleted the avx512-poly-buf branch December 6, 2022 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8297379: Enable the ByteBuffer path of Poly1305 optimizations #11338

8297379: Enable the ByteBuffer path of Poly1305 optimizations #11338

vpaprotsk commented Nov 23, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

mlbridge bot commented Nov 23, 2022 •

edited

Loading

jatin-bhateja commented Nov 24, 2022

jatin-bhateja Nov 24, 2022

vpaprotsk Dec 1, 2022

vpaprotsk Dec 1, 2022

sviswa7 Nov 29, 2022

vpaprotsk Dec 1, 2022

sviswa7 commented Dec 1, 2022

openjdk bot commented Dec 1, 2022 •

edited

Loading

sviswa7 commented Dec 5, 2022

jnimeh commented Dec 5, 2022

ascarpino commented Dec 5, 2022

vpaprotsk commented Dec 5, 2022

jnimeh left a comment

vpaprotsk commented Dec 6, 2022

openjdk bot commented Dec 6, 2022

sviswa7 commented Dec 6, 2022

openjdk bot commented Dec 6, 2022

openjdk bot commented Dec 6, 2022

8297379: Enable the ByteBuffer path of Poly1305 optimizations #11338

8297379: Enable the ByteBuffer path of Poly1305 optimizations #11338

Conversation

vpaprotsk commented Nov 23, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

mlbridge bot commented Nov 23, 2022 • edited Loading

Webrevs

jatin-bhateja commented Nov 24, 2022

jatin-bhateja Nov 24, 2022

Choose a reason for hiding this comment

vpaprotsk Dec 1, 2022

Choose a reason for hiding this comment

vpaprotsk Dec 1, 2022

Choose a reason for hiding this comment

sviswa7 Nov 29, 2022

Choose a reason for hiding this comment

vpaprotsk Dec 1, 2022

Choose a reason for hiding this comment

sviswa7 commented Dec 1, 2022

openjdk bot commented Dec 1, 2022 • edited Loading

sviswa7 commented Dec 5, 2022

jnimeh commented Dec 5, 2022

ascarpino commented Dec 5, 2022

vpaprotsk commented Dec 5, 2022

jnimeh left a comment

Choose a reason for hiding this comment

vpaprotsk commented Dec 6, 2022

openjdk bot commented Dec 6, 2022

sviswa7 commented Dec 6, 2022

openjdk bot commented Dec 6, 2022

openjdk bot commented Dec 6, 2022

vpaprotsk commented Nov 23, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 23, 2022 •

edited

Loading

openjdk bot commented Dec 1, 2022 •

edited

Loading