Skip to content

Add identity maps#154

Merged
vinistock merged 1 commit intomainfrom
08-27-add_identity_maps
Sep 2, 2025
Merged

Add identity maps#154
vinistock merged 1 commit intomainfrom
08-27-add_identity_maps

Conversation

@vinistock
Copy link
Member

@vinistock vinistock commented Sep 2, 2025

This PR creates IdentityHashMap and IdentityHashSet implementations. These are identical to a regular HashMap or HashSet, with the sole difference that it does not try to hash any of the inserted values as they have already been hashed externally by our IDs.

This improves performance by quite a bit as we avoid a lot of duplicate work.

@vinistock vinistock self-assigned this Sep 2, 2025
Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@vinistock vinistock marked this pull request as ready for review September 2, 2025 14:17
@vinistock vinistock requested a review from a team as a code owner September 2, 2025 14:17
}

impl Hasher for IdentityHasher {
fn write(&mut self, _bytes: &[u8]) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do something like this?

Suggested change
fn write(&mut self, _bytes: &[u8]) {}
fn write(&mut self, _bytes: &[u8]) {
unreachable!("IdentityHasher#write should never be called");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also disable all the other variants? http://doc.rust-lang.org/beta/std/hash/trait.Hasher.html#provided-methods

}

pub type IdentityHashMap<K, V> = HashMap<K, V, IdentityHashBuilder>;
pub type IdentityHashSet<K> = HashSet<K, IdentityHashBuilder>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub type IdentityHashSet<K> = HashSet<K, IdentityHashBuilder>;
pub type IdentityHashSet<T> = HashSet<T, IdentityHashBuilder>;

Copy link
Contributor

Choose a reason for hiding this comment

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

Any way we can add tests for this?

@vinistock vinistock merged commit ab18554 into main Sep 2, 2025
12 checks passed
Copy link
Member Author

Merge activity

@vinistock vinistock deleted the 08-27-add_identity_maps branch September 2, 2025 16:20
@Morriar
Copy link
Contributor

Morriar commented Sep 2, 2025

Was this auto-merged? 🤔

@vinistock
Copy link
Member Author

Yes, my bad I had auto merge turned on. I'll address the comments.

@vinistock
Copy link
Member Author

#155

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