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

Suppress stack-trace filling of ShortBufferException #849

Merged
merged 3 commits into from
Jun 15, 2020

Conversation

veblush
Copy link
Contributor

@veblush veblush commented Jun 2, 2020

This is about modifying Conscrypt to throw ShortBufferWithoutStackTraceException instead of ShortBufferException for the purpose of not wasting cpu-time filling in a stack trace which isn't used actually.

This is particularity problematic on Java 8 because it has a bug to throw an excessive number of ShortBufferExceptions, which was fixed on Java 11 by JDK-8178374 and will be included in future OpenJDK 8. But this still be valuable for those who have to use OpenJDK8 252 or older.

The bottom screenshot was taken from my gRPC Java benchmark and it was wasting up to 9% cpu-time just calling fillInStackTrace.

Screenshot from 2020-06-01 17-08-15

With this PR, this unnecessary code path can be removed. Note that this is sort-of implementation detail of CipherSpi and stripping the stack trace from ShortBufferException doesn't hurt the usability of Conscrypt.
Screenshot from 2020-06-01 17-18-24

@daulet daulet requested a review from prbprbprb June 2, 2020 13:01
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@kruton
Copy link
Contributor

kruton commented Jun 2, 2020

I guess most people just catch ShortBufferException and provide a bigger buffer so not having a stack trace isn't much of an issue. The CPU savings looks like a big win!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants