Treat map keys matching the same struct field as duplicates. #492
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
Signed-off-by: Ben Luddy <bluddy@redhat.com>
benluddy
force-pushed
the
dupmapkey-same-struct-field
branch
from
c5eff94
to
cbbabde
Compare
This resolves the case where two map keys could match the same struct field without being treated as
duplicate map keys. The DupMapKey documentation has been updated accordingly.
A map key that matches a previously-matched field will no longer quietly fall back to a
case-insensitive match and is now always treated as a duplicate key. Case-sensitive matching of map
key to struct field name is now a map lookup to improve the quadratic worst-case:
│ manykeys-before.txt │ manykeys-after.txt │
│ sec/op │ sec/op vs base │
UnmarshalMapToStruct/default_options/many_fields_one_key_per_field 24.137µ ± 4% 5.240µ ± 1% -78.29% (p=0.000 n=10)
│ manykeys-before.txt │ manykeys-after.txt │
│ B/op │ B/op vs base │
UnmarshalMapToStruct/default_options/many_fields_one_key_per_field 112.0 ± 0% 0.0 ± 0% -100.00% (p=0.000 n=10)
│ manykeys-before.txt │ manykeys-after.txt │
│ allocs/op │ allocs/op vs base │
UnmarshalMapToStruct/default_options/many_fields_one_key_per_field 1.000 ± 0% 0.000 ± 0% -100.00% (p=0.000 n=10)
This fix cleared the way to optimize the way duplicate keys are tracked, especially for inputs that
contain no unknown fields:
│ before.txt │ after.txt │
│ sec/op │ sec/op vs base │
UnmarshalMapToStruct/default_options/all_known_fields 642.7n ± 3% 735.4n ± 0% +14.42% (p=0.000 n=10)
UnmarshalMapToStruct/default_options/all_known_duplicate_fields 1611.5n ± 1% 521.1n ± 1% -67.66% (p=0.000 n=10)
UnmarshalMapToStruct/default_options/all_unknown_fields 1.731µ ± 3% 1.174µ ± 0% -32.18% (p=0.000 n=10)
UnmarshalMapToStruct/default_options/all_unknown_duplicate_fields 1.726µ ± 2% 1.076µ ± 0% -37.66% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_known_fields 638.3n ± 1% 725.5n ± 1% +13.66% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_known_duplicate_fields 476.1n ± 1% 514.7n ± 0% +8.11% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_unknown_fields 448.1n ± 1% 398.8n ± 1% -11.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_unknown_duplicate_fields 447.9n ± 0% 399.9n ± 1% -10.71% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_known_fields 1825.5n ± 0% 726.0n ± 0% -60.23% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_known_duplicate_fields 744.3n ± 0% 440.8n ± 0% -40.78% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_unknown_fields 2.885µ ± 1% 3.329µ ± 0% +15.37% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_unknown_duplicate_fields 844.8n ± 0% 738.7n ± 1% -12.56% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_known_fields 1828.5n ± 0% 723.4n ± 0% -60.44% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_known_duplicate_fields 744.3n ± 0% 436.4n ± 0% -41.36% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_unknown_fields 643.4n ± 0% 435.3n ± 0% -32.34% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_unknown_duplicate_fields 642.1n ± 0% 435.6n ± 1% -32.15% (p=0.000 n=10)
geomean 936.7n 748.3n -29.34%
│ before.txt │ after.txt │
│ B/op │ B/op vs base │
UnmarshalMapToStruct/default_options/all_known_fields 16.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/default_options/all_known_duplicate_fields 16.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/default_options/all_unknown_fields 16.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/default_options/all_unknown_duplicate_fields 16.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_known_fields 16.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_known_duplicate_fields 24.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_unknown_fields 24.000 ± 0% 8.000 ± 0% -66.67% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_unknown_duplicate_fields 24.000 ± 0% 8.000 ± 0% -66.67% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_known_fields 800.0 ± 0% 0.0 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_known_duplicate_fields 648.00 ± 0% 40.00 ± 0% -93.83% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_unknown_fields 800.0 ± 0% 1285.0 ± 0% +60.62% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_unknown_duplicate_fields 648.0 ± 0% 248.0 ± 0% -61.73% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_known_fields 800.0 ± 0% 0.0 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_known_duplicate_fields 648.00 ± 0% 40.00 ± 0% -93.83% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_unknown_fields 616.00 ± 0% 24.00 ± 0% -96.10% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_unknown_duplicate_fields 616.00 ± 0% 24.00 ± 0% -96.10% (p=0.000 n=10)
geomean 113.6 ? ¹ ²
¹ summaries must be >0 to compute geomean
² ratios must be >0 to compute geomean
│ before.txt │ after.txt │
│ allocs/op │ allocs/op vs base │
UnmarshalMapToStruct/default_options/all_known_fields 1.000 ± 0% 0.000 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/default_options/all_known_duplicate_fields 1.000 ± 0% 0.000 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/default_options/all_unknown_fields 1.000 ± 0% 0.000 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/default_options/all_unknown_duplicate_fields 1.000 ± 0% 0.000 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_known_fields 1.000 ± 0% 0.000 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_known_duplicate_fields 2.000 ± 0% 0.000 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_unknown_fields 2.000 ± 0% 1.000 ± 0% -50.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown/all_unknown_duplicate_fields 2.000 ± 0% 1.000 ± 0% -50.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_known_fields 15.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_known_duplicate_fields 5.000 ± 0% 2.000 ± 0% -60.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_unknown_fields 15.00 ± 0% 17.00 ± 0% +13.33% (p=0.000 n=10)
UnmarshalMapToStruct/reject_duplicate/all_unknown_duplicate_fields 5.000 ± 0% 5.000 ± 0% ~ (p=1.000 n=10) ¹
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_known_fields 15.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_known_duplicate_fields 5.000 ± 0% 2.000 ± 0% -60.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_unknown_fields 4.000 ± 0% 2.000 ± 0% -50.00% (p=0.000 n=10)
UnmarshalMapToStruct/reject_unknown_and_duplicate/all_unknown_duplicate_fields 4.000 ± 0% 2.000 ± 0% -50.00% (p=0.000 n=10)
geomean 3.043 ? ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean
Signed-off-by: Ben Luddy <bluddy@redhat.com>
benluddy
force-pushed
the
dupmapkey-same-struct-field
branch
from
cbbabde
to
1d28086
Compare
|
@benluddy Thanks for opening this PR! The benchmarks you shared look great! I'll take a look this weekend! 🙏 |
fxamacker
approved these changes
Feb 19, 2024
Comment on lines
+1905
to
+1908
| // For structs with typical field counts, expect that this can be | ||
| // stack-allocated. | ||
| var a [maxStackFields]bool | ||
| foundFldIdx = a[:nfields] |
There was a problem hiding this comment.
Nice way to reduce heap allocations! 👍
Comment on lines
+2095
to
+2097
| if mapKeys == nil { | ||
| mapKeys = make(map[interface{}]struct{}, 1) | ||
| } |
There was a problem hiding this comment.
Thanks for optimizing and adding code comments! 👍
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 resolves the case where two map keys could match the same struct field without being treated as
duplicate map keys. The DupMapKey documentation has been updated accordingly.
A map key that matches a previously-matched field will no longer quietly fall back to a
case-insensitive match and is now always treated as a duplicate key. Case-sensitive matching of map
key to struct field name is now a map lookup to improve the quadratic worst-case:
This fix cleared the way to optimize the way duplicate keys are tracked, especially for inputs that
contain no unknown fields:
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.