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

Migrate away from Eq1/Ord1/Show1 #27

Closed
Bodigrim opened this issue Feb 24, 2024 · 4 comments
Closed

Migrate away from Eq1/Ord1/Show1 #27

Bodigrim opened this issue Feb 24, 2024 · 4 comments

Comments

@Bodigrim
Copy link

Continuing on haskell/core-libraries-committee#190 (comment), I propose to migrate from

instance Eq1 f => Eq (Fix f)
instance Ord1 f => Ord (Fix f)
instance Show1 f => Show (Fix f)

to

instance Eq (f (Fix f)) => Eq (Fix f)
instance Ord (f (Fix f)) => Ord (Fix f)
instance Show (f (Fix f)) => Show (Fix f)

or if you fancy QuantifiedContraints

instance (forall a. Eq a => Eq (f a)) => Eq (Fix f)
instance (forall a. Ord a => Ord (f a)) => Ord (Fix f)
instance (forall a. Show a => Show (f a)) => Show (Fix f)

The motivation is that I recently migrated a codebase from a homegrown newtype Fix with instance Ord (f (Fix f)) => Ord (Fix f) to data-fix and hit performance issues. They were a combination of several factors:

  • I used Data.Functor.Classes.Generic to derive Ord1 instance generically. It appeared that from and to survived in Core, despite pretty aggressive optimization options. This is admittedly not a fault of data-fix, but a general issue with usability of Eq1 / Ord1 / Show1. I had to write all instances manually.

  • class Ord1 contains only liftCompare, but no liftLT, liftLE, liftGT, liftGE, so it has no access to <, <=, >=, > of the base functor. This implies that < on Fix f also necessarily goes through compare and takes a performance hit.

  • It does not seem that liftEq / liftCompare can be optimized away unless the base functor f is completely monomorphic and everything is forced to inline. E. g., (==) specialised to Fix (ListF Int) looks reasonable, but (==) for Fix (ListF a) goes through liftEq. The Core for

    instance Eq (f (Fix f)) => Eq (Fix f) where
      (==) = coerce ((==) @(f (Fix f)))

    looks more reasonable to me.


I understand that the proposed change is a breaking one. It is however not very pronounced: in recent base we have class (forall a. Eq f a) => Eq1 f, so the majority of users will not even notice. It's only if you require Eq (Fix f) as a constraint, but use liftEq inside, which will explode, because Eq1 f is no longer guaranteed. I think, however, that GHC would yell if encounters Eq (Fix f) constraint, suggesting to replace it with Eq1 f.

@phadej what do you think?

@phadej
Copy link
Collaborator

phadej commented Feb 24, 2024

Please write a patch and check what happens with at least few direct dependencies (from https://packdeps.haskellers.com/reverse/data-fix, e.g. dhall, hnix, liquidhaskell-boot) with various GHCs, including the oldest supported (GHC-8.4 or 8.6 it seems)

@phadej
Copy link
Collaborator

phadej commented Apr 24, 2024

Does #29 solve the optimisation problem? You'd still need Eq1/Ord1 instances, but that's should be an acceptable price (you already have them anyway).

@Bodigrim
Copy link
Author

Yes, looks good.

@phadej
Copy link
Collaborator

phadej commented Jul 4, 2024

Done in #29

@phadej phadej closed this as completed Jul 4, 2024
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 a pull request may close this issue.

2 participants