-
Notifications
You must be signed in to change notification settings - Fork 61
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
Improve speed & memory use for Diagnose() and DiagnoseFirst() #533
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
benluddy
changed the title
Eliminate some unnecessary alllocations for string diagnostic encodings.
Eliminate some unnecessary allocations for string diagnostic encodings.
May 8, 2024
Signed-off-by: Ben Luddy <[email protected]>
The write methods of *bytes.Buffer are guaranteed to return a nil error, so it is impossible to have test coverage for the if statements handling non-nil errors. Signed-off-by: Ben Luddy <[email protected]>
│ before.txt │ after.txt │ │ sec/op │ sec/op vs base │ Diagnose/escaped_character_in_text_string 180.3n ± 0% 109.2n ± 2% -39.41% (p=0.000 n=10) │ before.txt │ after.txt │ │ B/op │ B/op vs base │ Diagnose/escaped_character_in_text_string 192.00 ± 0% 72.00 ± 0% -62.50% (p=0.000 n=10) │ before.txt │ after.txt │ │ allocs/op │ allocs/op vs base │ Diagnose/escaped_character_in_text_string 5.000 ± 0% 2.000 ± 0% -60.00% (p=0.000 n=10) Signed-off-by: Ben Luddy <[email protected]>
The "Encoder" types for the various byte string diagnostic encodings (encoding/hex, encoding/base32, and encoding/base64) include sizable internal buffers (1KiB at time of writing), and a new encoder is created to produce the diagnostic encoding of every byte string in the input. This results in a lot of extra allocations, especially for inputs containing many small byte strings. │ before.txt │ after.txt │ │ sec/op │ sec/op vs base │ Diagnose/byte_string_base16_encoding 315.90n ± 0% 92.34n ± 1% -70.77% (p=0.000 n=10) Diagnose/byte_string_base32_encoding 325.55n ± 0% 96.98n ± 0% -70.21% (p=0.000 n=10) Diagnose/byte_string_base32hex_encoding 325.45n ± 0% 96.97n ± 0% -70.21% (p=0.000 n=10) Diagnose/byte_string_base64url_encoding 336.50n ± 0% 96.50n ± 0% -71.32% (p=0.000 n=10) geomean 325.8n 95.68n -70.63% │ before.txt │ after.txt │ │ B/op │ B/op vs base │ Diagnose/byte_string_base16_encoding 1344.00 ± 0% 80.00 ± 0% -94.05% (p=0.000 n=10) Diagnose/byte_string_base32_encoding 1344.00 ± 0% 80.00 ± 0% -94.05% (p=0.000 n=10) Diagnose/byte_string_base32hex_encoding 1344.00 ± 0% 80.00 ± 0% -94.05% (p=0.000 n=10) Diagnose/byte_string_base64url_encoding 1344.00 ± 0% 80.00 ± 0% -94.05% (p=0.000 n=10) geomean 1.313Ki 80.00 -94.05% │ before.txt │ after.txt │ │ allocs/op │ allocs/op vs base │ Diagnose/byte_string_base16_encoding 5.000 ± 0% 2.000 ± 0% -60.00% (p=0.000 n=10) Diagnose/byte_string_base32_encoding 5.000 ± 0% 2.000 ± 0% -60.00% (p=0.000 n=10) Diagnose/byte_string_base32hex_encoding 5.000 ± 0% 2.000 ± 0% -60.00% (p=0.000 n=10) Diagnose/byte_string_base64url_encoding 5.000 ± 0% 2.000 ± 0% -60.00% (p=0.000 n=10) geomean 5.000 2.000 -60.00% Signed-off-by: Ben Luddy <[email protected]>
fxamacker
changed the title
Eliminate some unnecessary allocations for string diagnostic encodings.
Reduce memory and allocs in Diagnose() and DiagnoseFirst()
May 11, 2024
fxamacker
changed the title
Reduce memory and allocs in Diagnose() and DiagnoseFirst()
Improve speed & memory use for Diagnose() and DiagnoseFirst()
May 13, 2024
fxamacker
approved these changes
May 13, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benluddy LGTM! Thanks for the optimization and benchmark results is very impressive!
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.
Description
This PR improves efficiency of functions in
diagnose.go
used byDiagnose()
andDiagnoseFirst()
, especially related to use ofencoding/hex
,encoding/base32
, andencoding/base64
.The "Encoder" types for the various byte string diagnostic encodings (encoding/hex, encoding/base32,
and encoding/base64) include sizable internal buffers (1KiB at time of writing), and a new encoder
is created to produce the diagnostic encoding of every byte string in the input. This results in a
lot of extra allocations, especially for inputs containing many small byte strings.
Producing the \uxxxx escape sequences also made a few more allocations than necessary.
PR Was Proposed and Welcomed in Currently Open Issue
Checklist (for code PR only, ignore for docs PR)
Last line of each commit message should be in this format:
Signed-off-by: Firstname Lastname [email protected]
(see next section).
Certify the Developer's Certificate of Origin 1.1
the Developer Certificate of Origin 1.1.