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

fix: optimize new set implementation and update docs #917

Merged
merged 3 commits into from
Sep 23, 2022

Conversation

austinabell
Copy link
Contributor

There is no benefit to the cache in the set's case, as no APIs depend on it, and since no data loaded, the cache adds more overhead than benefit. This change also removes the Ord and Clone requirements on the set's keys.

Benchmarking these changes are ~15% decrease in gas for a small number of elements, and ~25% for larger amounts of modifications

Copy link
Contributor

@itegulov itegulov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarking these changes are ~15% decrease in gas for a small number of elements, and ~25% for larger amounts of modifications

Interesting, would you mind sharing the benchmark source code?

In any case, LGTM and sounds likely that the caching overhead outweighs the benefits from what I remember from the benchmarks I did back in January.

@austinabell
Copy link
Contributor Author

Interesting, would you mind sharing the benchmark source code?

Yeah, was hacked together quickly, and didn't update all the values or point to a git dep, but here it is: austinabell/storage_collections_bench@b3864e3

@austinabell austinabell merged commit aa1c1e0 into master Sep 23, 2022
@austinabell austinabell deleted the austin/set_opt branch September 23, 2022 06:59
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.

2 participants