Improve performance of generated enum methods #2693
Merged
Conversation
This file contains hidden or 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
## Motivation Generated enums use a standard `fromValue` method to resolve a string value (usually received on the wire) to the corresponding enum. The existing implementation is not optimal for a few reasons: 1. It calls `Enum#values()`, which must allocate a new array every time 2. It iterates the entire array with O(N) complexity The generated `knownValues` follows a similar pattern, plus it does not take advantage of `EnumSet`, which is typically faster than HashSet. These are minor optimization concerns, however many code paths repeatedly call `fromValue()` (as opposed to caching it). ## Modifications * For each generated enum, store a value map which maps the string value to the corresponding enum constant value. Use this map for `fromValue`. * Use an EnumSet for `knownValues`. * Create new `CollectionUtils.index()` & `EnumUtils.index()` methods to facilitate the common use case of constructing a unique index lookup map ## Result * `fromValue` changes from O(N) to O(1) * Minor increase in memory overhead from storing a static map, but reduced allocation from creating new arrays every method call
joviegas
reviewed
Sep 3, 2021
| for (V value : values) { | ||
| K index = indexFunction.apply(value); | ||
| V prev = map.put(index, value); | ||
| Validate.isNull(prev, "No duplicate indices allowed but both %s and %s have the same index: %s", |
There was a problem hiding this comment.
Query : Since we are indexing to unique Converted type should that be reflected in the function name ?
In some case in future I might even want to index duplicate value in where I would expect Map<K, List> might have difficulty to use the same function name in this class
There was a problem hiding this comment.
Good point/question.
My thinking is that indexing like this would be the common/standard use case, where users expect unique indices, and if we wanted to extend this to something like multi-map support, then that new function name would be specialized to differentiate. E.g., index vs indexMulti, or similar.
However, since the current function can throw, then perhaps there is an argument for giving this one more explicit naming. What do you think?
There was a problem hiding this comment.
Renamed index to uniqueIndex.
joviegas
approved these changes
Sep 3, 2021
## Motivation Generated enums use a standard `fromValue` method to resolve a string value (usually received on the wire) to the corresponding enum. The existing implementation is not optimal for a few reasons: 1. It calls `Enum#values()`, which must allocate a new array every time 2. It iterates the entire array with O(N) complexity The generated `knownValues` follows a similar pattern, plus it does not take advantage of `EnumSet`, which is typically faster than HashSet. These are minor optimization concerns, however many code paths repeatedly call `fromValue()` (as opposed to caching it). ## Modifications * For each generated enum, store a value map which maps the string value to the corresponding enum constant value. Use this map for `fromValue`. * Use an EnumSet for `knownValues`. * Create new `CollectionUtils.index()` & `EnumUtils.index()` methods to facilitate the common use case of constructing a unique index lookup map ## Result * `fromValue` changes from O(N) to O(1) * Minor increase in memory overhead from storing a static map, but reduced allocation from creating new arrays every method call
|
Kudos, SonarCloud Quality Gate passed! |
aws-sdk-java-automation
added a commit
that referenced
this pull request
Sep 15, 2023
…43782f0c0 Pull request: release <- staging/75cecad7-0108-47c2-889d-fa443782f0c0
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.
Motivation
Generated enums use a standard
fromValuemethod to resolve a stringvalue (usually received on the wire) to the corresponding enum. The
existing implementation is not optimal for a few reasons:
Enum#values(), which must allocate a new array every timeThe generated
knownValuesfollows a similar pattern, plus it does nottake advantage of
EnumSet, which is typically faster than HashSet.These are minor optimization concerns, however many code paths
repeatedly call
fromValue()(as opposed to caching it).Modifications
to the corresponding enum constant value. Use this map for
fromValue.knownValues.CollectionUtils.index()&EnumUtils.index()methods tofacilitate the common use case of constructing a unique index lookup map
Result
fromValuechanges from O(N) to O(1)reduced allocation from creating new arrays every method call
Types of changes
Checklist
mvn installsucceedsLicense