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

Replace map types with alloy_primitives::map #11220

Closed
Tracked by #11169 ...
DaniPopes opened this issue Sep 25, 2024 · 0 comments · Fixed by #11235
Closed
Tracked by #11169 ...

Replace map types with alloy_primitives::map #11220

DaniPopes opened this issue Sep 25, 2024 · 0 comments · Fixed by #11235
Labels
C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started

Comments

@DaniPopes
Copy link
Member

DaniPopes commented Sep 25, 2024

Describe the feature

Replace all {Hash,BTree}{Map,Set} with their equivalent in alloy_primitives::map, as explained below. This both improves performance and is more compatible with no_std, as the fallback will be using hashbrown when "std" is not enabled.

Migration guide:

  1. std::collections::{HashMap, HashSet} -> alloy_primitives::map::{HashMap, HashSet}; unless:
    • the key type is Selector, Address, or B256, then use alloy_primitives::map::{<Name>{HashMap,HashSet}}, e.g. AddressHashMap
    • the key type is any other B* or FixedBytes, then use alloy_primitives::map::{FbHashMap, FbHashSet}
  2. std::collections::{hash_map, hash_set} -> alloy_primitives::map::{hash_map, hash_set}
  3. std::collections::{BTreeMap, BTreeSet} -> refer to 1. above
    • IMPORTANT: this is not an equivalent conversion because BTree iteration is guaranteed to be sorted, while HashMap iteration depends on the hash value which is random/changes across runs; only do this if iteration order does not matter!
    • If iteration affects user-facing output, or serialization, e.g. in types implementing serde::Serialize, BTree should be replaced with the Index* equivalents so as to have a consistent iteration order, rather than random; this requires the "map-index" feature.
    • If sorted iteration matters, or BTree specific methods are used, such as split_off, then there is nothing to do.
    • When in doubt, it's OK to leave it and we can revisit it at a later point.

The minimum alloy-primitives version should also be bumped to 0.8.4.

Note that this is rather large in scope, so feel free to open a PR for doing this to a single crate for example.

Additional context

@DaniPopes DaniPopes added C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Sep 25, 2024
@DaniPopes DaniPopes added D-good-first-issue Nice and easy! A great choice to get started and removed S-needs-triage This issue needs to be labelled labels Sep 25, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Reth Tracker Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant