-
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
Refactor sorted map encode to use fewer buffers for nested maps. #537
Conversation
3d0c715
to
8f7d76b
Compare
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.
Thanks @benluddy for opening this PR!
It looks good and I left a few comments that Less()
used for sorting map elements should use valueOffset
instead of nextOffset
to sort map keys.
encode.go
Outdated
@@ -1247,11 +1280,13 @@ func (x *bytewiseKeyValueSorter) Swap(i, j int) { | |||
} | |||
|
|||
func (x *bytewiseKeyValueSorter) Less(i, j int) bool { | |||
return bytes.Compare(x.kvs[i].keyCBORData, x.kvs[j].keyCBORData) <= 0 | |||
kvi, kvj := x.kvs[i], x.kvs[j] | |||
return bytes.Compare(x.data[kvi.offset:kvi.nextOffset], x.data[kvj.offset:kvj.nextOffset]) <= 0 |
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.
We should only compare keys (not entire map element).
return bytes.Compare(x.data[kvi.offset:kvi.nextOffset], x.data[kvj.offset:kvj.nextOffset]) <= 0 | |
return bytes.Compare(x.data[kvi.offset:kvi.valueOffset], x.data[kvj.offset:kvj.valueOffset]) <= 0 |
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.
Great catch, thanks! Fixed, and tweaked the test covering this slightly so that it will fail if the length of the entire entry is compared instead of the length of the key only.
Runs a bit faster, but more importantly, only needs a single buffer to encode nested, sorted maps instead of using multiple temporary buffers. │ before.txt │ after.txt │ │ sec/op │ sec/op vs base │ MarshalCanonical/Go_map[string]string_to_CBOR_map_canonical 1.464µ ± 0% 1.395µ ± 0% -4.68% (p=0.000 n=10) MarshalCanonical/Go_map[int]int_to_CBOR_map_canonical 192.1n ± 0% 186.2n ± 1% -3.10% (p=0.000 n=10) geomean 530.2n 509.6n -3.89% │ before.txt │ after.txt │ │ B/op │ B/op vs base │ MarshalCanonical/Go_map[string]string_to_CBOR_map_canonical 88.00 ± 0% 112.00 ± 0% +27.27% (p=0.000 n=10) MarshalCanonical/Go_map[int]int_to_CBOR_map_canonical 3.000 ± 0% 3.000 ± 0% ~ (p=1.000 n=10) ¹ geomean 16.25 18.33 +12.82% ¹ all samples are equal │ before.txt │ after.txt │ │ allocs/op │ allocs/op vs base │ MarshalCanonical/Go_map[string]string_to_CBOR_map_canonical 2.000 ± 0% 2.000 ± 0% ~ (p=1.000 n=10) ¹ MarshalCanonical/Go_map[int]int_to_CBOR_map_canonical 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹ geomean 1.414 1.414 +0.00% ¹ all samples are equal Signed-off-by: Ben Luddy <bluddy@redhat.com>
8f7d76b
to
6396be3
Compare
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.
LGTM! Thanks for super fast response and updating this PR on a Sunday! 👍
Description
Runs a bit faster, but more importantly, only needs a single buffer to encode nested, sorted maps
instead of using multiple temporary buffers.
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 firstname.lastname@example.com
(see next section).
Certify the Developer's Certificate of Origin 1.1
the Developer Certificate of Origin 1.1.