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

Added Hashed type for caching the 'hash' function result. #76

Merged
merged 1 commit into from
Oct 17, 2016

Conversation

mvv
Copy link
Contributor

@mvv mvv commented Mar 24, 2014

Supposed to save time spent on calculating hashes in lookups and other operations in unordered-containers.

@hvr
Copy link
Collaborator

hvr commented Mar 25, 2014

See discussion in #22

@mvv
Copy link
Contributor Author

mvv commented Mar 25, 2014

Hm, one benefit of the polymorphic version is that you don't have to write Eq, Ord, Hashable instances and hashed, unhashed functions by yourself. Just write

type MyText = Hashed Text
instance IsString MyText where
  fromString = hashed . Text.pack

and you are good to go. And even if you want a monomorphic type, the polymorphic version instances can still be reused via newtype deriving:

newtype MyText = MyText (Hashed Text) deriving (Eq, Ord, Hashable)

@tibbe
Copy link
Collaborator

tibbe commented Mar 25, 2014

@mvv I'm willing to add (some version of) this if you have a real need where you can show a speed-up.

@mvv
Copy link
Contributor Author

mvv commented Mar 30, 2014

I'm going to use this in the "renamer" stage of a compiler I'm writing (The parser represents symbols as Hashed Text and later all the symbols in the AST need to be replaced with glorified unique integers. The walker that does the job uses a HashMap (Hashed Text) Symbol to do the mapping) . I need more time to come up with a benchmark (the source code would probably need to be quite long to show a speed-up)..

Hashed a _ `compare` Hashed b _ = a `compare` b

instance Hashable a => Hashable (Hashed a) where
hashWithSalt s = hashWithSalt s . unhashed
Copy link
Contributor

Choose a reason for hiding this comment

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

should this one be defaultHashWithSalt to benefit from Hashed property?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should define both hash and hashWithSalt. The latter could use defaultHashWithSalt.

@andrewthad
Copy link
Contributor

I'm currently using something similar to optimize a syntax tree. It would be nice to have this functionality in hashable. The Eq instance looks a little dubious:

instance Eq a => Eq (Hashed a) where
  Hashed a _ == Hashed b _ = a == b

Since the purpose of a hash is to speed up equality tests, I think it should be:

instance Eq a => Eq (Hashed a) where
  Hashed a ha == Hashed b hb = if ha == hb then a == b else False

Or more succinctly:

instance Eq a => Eq (Hashed a) where
  Hashed a ha == Hashed b hb = (ha == hb) && (a == b)

The Ord instance could benefit similarly.

@tibbe
Copy link
Collaborator

tibbe commented Oct 16, 2016

@andrewthad the point is to avoid hashing, but we should definitely use the cached hash to speed up the negative case for equality checking as well.

This pull request seems abandoned. Do you want to pick it up?

@andrewthad
Copy link
Contributor

Yeah, I'll commandeer this.

@andrewthad
Copy link
Contributor

New PR is at #121

@tibbe tibbe merged commit 9de391f into haskell-unordered-containers:master Oct 17, 2016
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.

5 participants