Improve speed in MessageDigest and Hmac #54
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bdonlan
reviewed
Aug 27, 2019
| @ private exceptional_behavior | ||
| @ requires buff.count <= buffSize - 1; | ||
| @ assignable \nothing; | ||
| @ signals_only ArrayIndexOutOfBoundsException; |
There was a problem hiding this comment.
I'm a little confused by this. The behavior here is not documented in the javadoc (which appears to have been copied from a different function) and doesn't really make sense to me either; in particular, this appears to be an equivalent condition (modulo overflow) to the one on 385.
There was a problem hiding this comment.
These are for formal proofs of correctness. It has been slightly modified to take into account the hard coded length (of 1). We do not currently have this system running, but I'm avoiding bit-rot.
There was a problem hiding this comment.
Do we know if these proofs are correct? The requires conditions seem to overlap to me, unless I'm missing something.
There was a problem hiding this comment.
They were and we need to figure out how to reenable running of them. They have have drifted even separate from this PR. I'm trying to keep them as close to correct as possible.
| @ ensures canTakeData(bufferState); | ||
| @ also | ||
| @ public exceptional_behavior | ||
| @ requires buff.count <= buffSize - 1; |
There was a problem hiding this comment.
Why are we throwing exceptions when the buffer count is within the buffer size?
There was a problem hiding this comment.
This is not Java exceptions but for the formal verification. The listed line requires that the buffer count be valid.
|
Heap error. Restarted |
|
Close and re-open to rerun status checks. |
|
As |
|
We ran our benchmark suite for MessageDigest and HMac on an i3.metal instance with 16 simultaneous threads.
|
bdonlan
approved these changes
Aug 28, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #: #52
Description of changes:
Improves handling of single-byte updates to both MessageDigest and Hmac by not using array copies and by removing unneeded synchronization. Both of these objects are not thread-safe to begin with and we only need to ensure we are synchronized when crossing the JNI boundary. We achieve this synchronization by taking a lock on the
ctx(which is unique to each instance of the MessageDigest or Hmac SPI). We do this rather than use a standardsynchronizedmethod as passing a non-static method as a functional interface creates new lambda instances in memory and can reduce efficiency (due to object allocation/deallocation). This does not guarantee that there are no other threads updating the data arrays at the same time. Doing so may (correctly) create a corrupted result (as the behavior in this case is undefined). We are not worried about reading past the ends of these arrays (and creating memory errors) as the C++ code performs correct bounds checking on these arrays prior to use (withinjava_buffer).Detailed benchmarks are being run, but initial results look promising:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.