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

refactor: bump hashbrown to v0.15.0, use HashTable api #6

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

willothy
Copy link
Member

@willothy willothy commented Nov 4, 2024

I had previously thought that the older raw API was more performant, but after some additional testing the new API appears to work well. It allows me to remove some unsafe code, so updating seems like a win-win.


Important

Refactor to use hashbrown v0.15.0 HashTable API, removing unsafe code in shard.rs and shard_map.rs.

  • Dependencies:
    • Bump hashbrown to version 0.15.0 in Cargo.toml and Cargo.lock.
    • Add dependencies allocator-api2, equivalent, and foldhash in Cargo.lock.
  • Refactor:
    • Replace hashbrown::raw::RawTable with hashbrown::HashTable in shard.rs.
    • Use hashbrown::hash_table::Entry API in shard_map.rs for insert, get, get_mut, and remove functions.
    • Remove hash_u64() function in shard_map.rs and replace with hash_one() method.
  • Safety:
    • Remove unsafe code related to raw table operations in shard_map.rs.

This description was created by Ellipsis for bcc542b. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to bcc542b in 22 seconds

More details
  • Looked at 193 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. src/shard_map.rs:182
  • Draft comment:
    Ensure hash_one is used correctly and consistently across the codebase. It replaces the previous custom hash function.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The hash_one method is used multiple times in the code. It is a new method introduced in the updated hashbrown version. Ensure that it is used correctly and consistently across the codebase.
2. src/shard_map.rs:209
  • Draft comment:
    Ensure the logic using entry is correctly adapted from the previous find_or_find_insert_slot method.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The entry method is used to replace the previous find_or_find_insert_slot method. Ensure that the logic is correctly adapted to the new API.
3. src/shard_map.rs:251
  • Draft comment:
    Ensure the logic using find is correctly adapted from the previous get method.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The find method is used to replace the previous get method. Ensure that the logic is correctly adapted to the new API.
4. src/shard_map.rs:289
  • Draft comment:
    Ensure the logic using find_mut is correctly adapted from the previous get_mut method.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The find_mut method is used to replace the previous get_mut method. Ensure that the logic is correctly adapted to the new API.
5. src/shard_map.rs:352
  • Draft comment:
    Ensure the logic using find_entry is correctly adapted from the previous remove_entry method.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The find_entry method is used to replace the previous remove_entry method. Ensure that the logic is correctly adapted to the new API.

Workflow ID: wflow_HsBOW7JR0hVbqctg


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@willothy willothy merged commit c6ae8f9 into main Nov 4, 2024
2 checks passed
@willothy willothy deleted the hashbrown-hashtable branch November 4, 2024 08:50
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.

1 participant