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(v2): use faster hasher to optimize #1941

Merged
merged 27 commits into from
Nov 27, 2024
Merged

Conversation

fu050409
Copy link
Member

Description:

BREAKING CHANGE:

Related issue (if exists):

Copy link

changeset-bot bot commented Nov 16, 2024

⚠️ No Changeset found

Latest commit: 64e4e47

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package or glob expression "farm-docs" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

@fu050409 fu050409 added WIP Working in progress v2.0 v2.0 version labels Nov 16, 2024
@fu050409 fu050409 removed the WIP Working in progress label Nov 16, 2024
@wre232114
Copy link
Member

You can rename FxHashMap to HashMap and reexport them at the place. So all usage will not be changed, also it's easy to update the hash implementation in the future

@fu050409
Copy link
Member Author

Infact, even though I create a alias for FxHashMap and named it farm_core::HashMap, the type check is not match. This means that althought our usage looks same, all the codes depend on these apis should be refactored. @wre232114 Is this acceptable? 😊

@wre232114
Copy link
Member

yeah, it's fine

@fu050409 fu050409 requested review from wre232114 and removed request for wre232114 and shulandmimi November 24, 2024 05:39
Copy link
Member

@wre232114 wre232114 left a comment

Choose a reason for hiding this comment

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

can we add a new() method for the faster hashmap, then we don't need to replace all the new() to default()

@fu050409
Copy link
Member Author

can we add a new() method for the faster hashmap, then we don't need to replace all the new() to default()

I'm afraid not. The method new() is provided by the Rust standard library. And in soruce code, it is:

#[inline]
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn new() -> HashMap<K, V, RandomState> {
    Default::default()
}

As we can see, this method is for HashMap with RandomState so it's not implemented for FxHashBuilder.

Otherwise, we cannout define inherent impl for foreign type.

@wre232114 wre232114 enabled auto-merge (squash) November 27, 2024 04:05
@wre232114 wre232114 merged commit 674e5ed into v2-dev Nov 27, 2024
36 checks passed
@wre232114 wre232114 deleted the refactor/hashmap branch November 27, 2024 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.0 v2.0 version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants