Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8297379: Enable the ByteBuffer path of Poly1305 optimizations #11338

Closed
wants to merge 6 commits into from

Conversation

vpaprotsk
Copy link
Contributor

@vpaprotsk vpaprotsk commented Nov 23, 2022

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:


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

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

bridgekeeper bot commented Nov 23, 2022

👋 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
Copy link

openjdk bot commented Nov 23, 2022

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

@openjdk openjdk bot added the security security-dev@openjdk.org label Nov 23, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 23, 2022
@mlbridge
Copy link

mlbridge bot commented Nov 23, 2022

Webrevs

@jatin-bhateja
Copy link
Member

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sviswa7
Copy link

sviswa7 commented Dec 1, 2022

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

@openjdk
Copy link

openjdk bot commented Dec 1, 2022

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

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 1, 2022
@sviswa7
Copy link

sviswa7 commented Dec 5, 2022

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

@jnimeh
Copy link
Member

jnimeh commented Dec 5, 2022

@sviswa7 I will be looking at this today

@ascarpino
Copy link
Contributor

The changes look ok to me

@vpaprotsk
Copy link
Contributor Author

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

Copy link
Member

@jnimeh jnimeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 6, 2022
@openjdk
Copy link

openjdk bot commented Dec 6, 2022

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

@sviswa7
Copy link

sviswa7 commented Dec 6, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Dec 6, 2022

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 openjdk bot added the integrated Pull request has been integrated label Dec 6, 2022
@openjdk openjdk bot closed this Dec 6, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 6, 2022
@openjdk
Copy link

openjdk bot commented Dec 6, 2022

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

@vpaprotsk vpaprotsk deleted the avx512-poly-buf branch December 6, 2022 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
5 participants