Higher kinded classes #124

andrewthad · 2016-10-19T14:24:59Z

This implements what is proposed in #114. It is not yet complete. I still need to add more instances. There are two deviations from the original proposal:

The method liftHash is not included. The reason for this are found in the documentation in Data.Hashable.Lifted.
The Hashable2 class was included as well. This is mostly for consistency with other packages that provide higher rank typeclass variants.

In some of existing Hashable instances, I call hashWithSalt1 to avoid duplicating code. You can look at the instance for [] to see this. I do not believe that this will hurt performance because the inliner should expand these.

The example instances I provide in Data.Hashable.Lifted have not been typechecked and are likely broken. I'll get to that soon.

Let me know at this point if there are any concerns so that the can be addressed before I write all the remaining instances.

RyanGlScott · 2016-10-19T14:28:52Z

Generics support for Hashable1?

andrewthad · 2016-10-19T14:38:12Z

@RyanGlScott I knew you would show up ;) Now that I think about it, that would be useful (especially if I'm working with Fix Expr, and I keep changing Expr). I'll attempt to implement this, and I would appreciate if you would review it.

RyanGlScott · 2016-10-19T14:42:35Z

I'll attempt to implement this, and I would appreciate if you would review it.

Sure thing! I recommend using a trick like aeson's ToArgs as a way to reuse the code between the code for the Generic- and Generic1-based machinery.

andrewthad · 2016-10-19T14:45:14Z

@RyanGlScott Thanks. That's a really good trick. I was just about to duplicate everything.

andrewthad · 2016-10-19T16:57:49Z

Generics for Hashable1 is now ready. Also, does anyone know why hashSum even takes size as a parameter? It looks like there some bitshifting arithmetic that happens on it, but ultimately, it's never used. Here it is as it currently exists: https://github.com/tibbe/hashable/blob/a5bb09f8ecd175e62118258ffd30ac4afaf35530/Data/Hashable/Generic.hs#L43-L61

I'm guessing that its leftover from a different approach to hashing sums that was removed.

RyanGlScott · 2016-10-19T17:25:34Z

@andrewthad I think you're missing a GHashable case for (:.:) (i.e., how would you generically handle something like newtype Compose f g a = Compose (f (g a))?)

RyanGlScott · 2016-10-19T17:30:31Z

Data/Hashable/Generic.hs

+    hashSum toHash !salt !code _ (M1 x) = ghashWithSalt toHash (hashWithSalt salt code) x
+    {-# INLINE hashSum #-}
+
+instance GSum One Par1 where


Is this case even necessary? It looks like GSum bottoms out at C1 c a (at which point it relies on a being a GHashable), so I don't believe this case is ever reachable, no?

RyanGlScott · 2016-10-19T17:31:38Z

Data/Hashable/Class.hs

+data Zero = Zero
+data One = One
+
+data ToHash arity a where


Minor bikeshedding: I'd call this datatype HashArgs instead (I used the name ToArgs in the aeson example simply to distinguish it from FromArgs, as the former was for ToJSON and the latter was for FromJSON).

andrewthad · 2016-10-19T18:24:44Z

@RyanGlScott Thanks for the feedback. I've address both of your comments.

RyanGlScott · 2016-10-20T13:10:58Z

Data/Hashable.hs

+
+-- This function is copied from Data.Functor.Classes, which does
+-- not export it.
+showsUnaryWith :: (Int -> a -> ShowS) -> String -> Int -> a -> ShowS


This function is copied from Data.Functor.Classes, which does not export it.

Doesn't it?

Whoops. Thanks for pointing that out.

andrewthad · 2016-10-21T15:26:35Z

Not sure what's causing the build failure. The failing line is:

Preprocessing test suite 'tests' for hashable-1.2.4.0...

: cannot satisfy -package-id hashable-1.2.4.0-inplace:
hashable-1.2.4.0-inplace is shadowed by package hashable-1.2.4.0-85f768408d56687a048b7dee7b04a672

Other than that, I still need to add tuple instances. Any other thoughts or concerns?

RyanGlScott · 2016-10-21T17:38:53Z

I've seen that "shadowed by package" error before. It's because some dependencies in the test suite depend on hashable as a transitive dependency (i.e. QuickCheck, which depends on semigroups, which depends on hashable), but you're also linking against the in-tree hashable by listing it as its own dependency in the test suite. Thus there are two versions of the hashable library it could choose to link against when building the dependencies, which confuses cabal-install.

Fixing these sorts of issues is never fun. For one thing, you absolutely cannot list hashable as a dependency the test suite—you need to rebuild the hashable source separately when building the test suite. This way, QuickCheck/semigroups/et al. will build against a separate copy of hashable (from Hackage), and the test suite will separately link against the in-tree hashable. As long as the two APIs never meet, Cabal should be able to keep all of this straight (i.e., don't do this).

Since you can't depend on the hashable library in the test suite, this means copying over all modules, build-depends, c-sources, etc. from the library stanza into the test-suite stanza. See the hashable benchmark stanza for an example, as it also has to work around this issue.

Now here's the kicker. Even after you do all this, cabal-install will still get confused if you try cabal install --only-dependencies --enable-tests on this project from a clean package database, because cabal-install will get into a dependency resolution loop. This means (until haskell/cabal#3422 is fixed) that as a workaround, you'll have to install all of the test suite dependencies beforehand, and then run cabal configure --enable-tests. Changing .travis.yml to do this is frequently a matter of guess-and-check; after several iterations, here's what I arrived on in order to install deepseq's dependencies before configuring its tests. You'd have to figure out something similar for hashable.

tl;dr We're kind of in a rut as far as configuring the tests go. I wouldn't recommend trying to fix the issue in this PR, as getting it right often involves several rounds of back-and-forth between Travis to get the dependency installation just right.

andrewthad · 2016-10-21T19:35:04Z

Thanks for clearing that up. I've wondered about test suites that use other-modules like that before. Definitely won't try to fix that in this PR.

I'll go ahead and get the tuple instances ready.

tibbe

Looks nice overall. Just some nits/questions.

tibbe · 2016-10-21T21:51:33Z

Data/Hashable.hs

+-- 'liftHashWithSalt' to ignore its first argument when a
+-- value is actually available for it to work on.
+instance Hashable1 Hashed where
+  liftHashWithSalt _ s (Hashed _ h) = defaultHashWithSalt s h


It's a bit unsettling indeed. I wonder if Hashed to cache the salt it used last time and make a comparison to it and rehash if it's not the same.

Since Hashed can only be created by hashed, that would require introducing another function hashedWithSalt and changing Hashed to:

data Hashed = Hashed a (Maybe Int)

That being said, I don't like that because it's easily to accidentally use incorrectly, circumventing the caching its supposed to provide. My preference would be to leave it as is and document the counterintuitive behavior of hashWithSalt on a Hashed. For the libraries I'm working on, caching is the most important thing that Hashed provides.

tibbe · 2016-10-21T21:54:07Z

Data/Hashable.hs

+-- This instance is a little unsettling. It is unusal for
+-- 'liftHashWithSalt' to ignore its first argument when a
+-- value is actually available for it to work on.
+instance Hashable1 Hashed where


Shouldn't this instance be in Lifted?

That would orphan the instance. I was trying to avoid doing that, but I could move Hashed into another module.

tibbe · 2016-10-21T21:55:02Z

Data/Hashable.hs

+-- instances for @Data.Functor.Classes@ higher rank typeclasses
+-- in base-4.9 and onward.
+#if MIN_VERSION_base(4,9,0)
+instance Eq1 Hashed where


Similarly for these instances. Shouldn't they be in Lifted? We might need to move Hashed to an internal module to avoid the instances being orphaned.

Do you think that putting it into Data.Hashable.Class would be alright? Or would you prefer a new internal module?

Class is fine.

This has now been moved.

tibbe · 2016-10-21T21:56:06Z

Data/Hashable/Class.hs


 -- | The class of types that can be generically hashed.
-class GHashable f where
-    ghashWithSalt :: Int -> f a -> Int
+class GHashable arity f where


We need to make sure that this doesn't introduce any badly generated Core for derived instances. Can you check e.g. that deriving for a simple data type yields as good Core as it did before?

Yeah, I don't really know how to look at Core, but I'm at Hac Phi, so I'll get someone to help me with it tomorrow.

Oddly, the changes to GHashable have improved performance. I'll detail this in a top level comment.

andrewthad · 2016-10-22T19:17:29Z

Ben Gamari was able to help look into the differences in the generated core, and the new generic implementation of hashWithSalt surprisingly results in fewer allocations. I've added benchmarks the generic machinery. Here are the results:

---------------------------
NEW GENERICS IMPLEMENTATION
---------------------------
> cabal bench --benchmark-options='Generic'

Preprocessing benchmark 'benchmarks' for hashable-1.2.4.0...
Running 1 benchmarks...
Benchmark benchmarks: RUNNING...
benchmarking Generic/product
time                 25.82 ns   (25.77 ns .. 25.88 ns)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 25.85 ns   (25.80 ns .. 25.98 ns)
std dev              257.0 ps   (118.7 ps .. 477.9 ps)

benchmarking Generic/sum
time                 5.756 ns   (5.749 ns .. 5.765 ns)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 5.757 ns   (5.751 ns .. 5.768 ns)
std dev              24.81 ps   (14.63 ps .. 42.26 ps)

benchmarking Generic/product and sum
time                 8.425 ns   (8.415 ns .. 8.435 ns)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 8.422 ns   (8.415 ns .. 8.432 ns)
std dev              27.64 ps   (20.11 ps .. 36.48 ps)

Benchmark benchmarks: FINISH

---------------------------
OLD GENERICS IMPLEMENTATION
---------------------------
> cabal bench --benchmark-options='Generic'

Preprocessing benchmark 'benchmarks' for hashable-1.2.4.0...
Running 1 benchmarks...
Benchmark benchmarks: RUNNING...
benchmarking Generic/product
time                 26.40 ns   (26.38 ns .. 26.44 ns)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 26.42 ns   (26.39 ns .. 26.48 ns)
std dev              128.1 ps   (91.03 ps .. 172.5 ps)

benchmarking Generic/sum
time                 9.844 ns   (9.832 ns .. 9.859 ns)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 9.841 ns   (9.832 ns .. 9.857 ns)
std dev              39.71 ps   (25.60 ps .. 64.43 ps)

benchmarking Generic/product and sum
time                 13.90 ns   (13.87 ns .. 13.93 ns)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 13.91 ns   (13.89 ns .. 13.93 ns)
std dev              70.92 ps   (53.18 ps .. 99.33 ps)

andrewthad · 2016-10-24T21:45:52Z

Also, just in case anyone wants to look at master but with the backported benchmarks, here it is: https://github.com/andrewthad/hashable/tree/master_with_examples. I think that I still need to correct some of the examples I gave in the haddocks.

@tibbe What were your thoughts on my response concerning Hashed?

andrewthad · 2016-10-28T12:49:58Z

Just wanted to check in and see if there is anything I can to do keep this PR going forward.

andrewthad and others added 2 commits October 18, 2016 21:46

add Hashable1 and Hashable2 with auxiliary functions

cfee24f

Add instances and documentation

46dc659

andrewthad mentioned this pull request Oct 19, 2016

Hashable1 #114

Closed

adapt generics support for Hashable1

f42e268

fix merge conflicts

e190657

RyanGlScott reviewed Oct 19, 2016

View reviewed changes

andrewthad added 2 commits October 19, 2016 14:10

rename ToHash to HashArgs

9779db9

add composition instance, remove Par1 GSum instance

d84cfa5

andrewthad added 5 commits October 19, 2016 14:50

added more instances for Hashable1

833efbf

add Hashable1 instance for Hashed. Add test that it abides by laws

aae100e

Add Hashable1 instances for Compose,Product,Sum

0c09055

improve warnings for old GHCs

ded8c87

add Eq1,Ord1,Show1 instances for Hashed, correct Show instance

7cd9bb4

RyanGlScott reviewed Oct 20, 2016

View reviewed changes

andrewthad added 2 commits October 20, 2016 09:11

remove unneeded constraint from Hashable Hashed instance

088d725

use showsUnaryWith instead of redefining it

acb9379

add higher rank tuple instances, similar to grind in MMORPG

39d5b29

tibbe reviewed Oct 21, 2016

View reviewed changes

Added examples for testing that hashes have not changed

fb9616b

andrewthad added 2 commits October 22, 2016 13:46

make generic hashable of sums same as before

77fd41a

add benchmarks for generics

a7f702b

moved Hashed into Class module

b6b9f6d

tibbe merged commit dfebc76 into haskell-unordered-containers:master Oct 29, 2016

RyanGlScott mentioned this pull request Nov 6, 2016

Add some hashed operations from classes that cannot be implemented #122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Higher kinded classes #124

Higher kinded classes #124

andrewthad commented Oct 19, 2016

RyanGlScott commented Oct 19, 2016

andrewthad commented Oct 19, 2016 •

edited

Loading

RyanGlScott commented Oct 19, 2016

andrewthad commented Oct 19, 2016

andrewthad commented Oct 19, 2016 •

edited

Loading

RyanGlScott commented Oct 19, 2016 •

edited

Loading

RyanGlScott Oct 19, 2016

RyanGlScott Oct 19, 2016

andrewthad commented Oct 19, 2016

RyanGlScott Oct 20, 2016

andrewthad Oct 20, 2016

andrewthad commented Oct 21, 2016

RyanGlScott commented Oct 21, 2016

andrewthad commented Oct 21, 2016

tibbe left a comment

tibbe Oct 21, 2016

andrewthad Oct 22, 2016

tibbe Oct 21, 2016

andrewthad Oct 22, 2016

tibbe Oct 21, 2016

andrewthad Oct 22, 2016

tibbe Oct 22, 2016

andrewthad Oct 22, 2016

tibbe Oct 21, 2016

andrewthad Oct 22, 2016

andrewthad Oct 22, 2016

andrewthad commented Oct 22, 2016

andrewthad commented Oct 24, 2016

andrewthad commented Oct 28, 2016

Higher kinded classes #124

Higher kinded classes #124

Conversation

andrewthad commented Oct 19, 2016

RyanGlScott commented Oct 19, 2016

andrewthad commented Oct 19, 2016 • edited Loading

RyanGlScott commented Oct 19, 2016

andrewthad commented Oct 19, 2016

andrewthad commented Oct 19, 2016 • edited Loading

RyanGlScott commented Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewthad commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewthad commented Oct 21, 2016

RyanGlScott commented Oct 21, 2016

andrewthad commented Oct 21, 2016

tibbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewthad commented Oct 22, 2016

andrewthad commented Oct 24, 2016

andrewthad commented Oct 28, 2016

andrewthad commented Oct 19, 2016 •

edited

Loading

andrewthad commented Oct 19, 2016 •

edited

Loading

RyanGlScott commented Oct 19, 2016 •

edited

Loading