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

Omit deriving Generic on LicenseId #5893

Closed
ndmitchell opened this Issue Feb 17, 2019 · 11 comments

Comments

Projects
None yet
4 participants
@ndmitchell
Copy link

ndmitchell commented Feb 17, 2019

Compiling the LicenseId file on my Windows GHC 6.8.3 takes ~30s (measured while running Hadrian). Experimenting with a cut down version, it turned out that most of the time was due to the Generic derivation on LicenseId. That generic derivation is used to provide a Binary instance, but can be replaced trivially by:

instance Binary LicenseId where
    put = put . fromEnum
    get = toEnum <$> get

In my measurements this reduces the compilation time significantly, from 9s to 3s at -O0 and 22s to 8s on my cutdown file. That seems a worthwhile speedup.

Given this is an exported type, it's technically a user visible API change, hence raising a bug rather than a PR.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 17, 2019

I believe it should be fine to apply this change to master.
6.8.3 is quite old though, how do more recent versions fare?

@ndmitchell

This comment has been minimized.

Copy link
Author

ndmitchell commented Feb 17, 2019

Sorry, I meant 8.6.3 - the latest released one 🤦‍♂️

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 17, 2019

OK, let's do it.

@ndmitchell

This comment has been minimized.

Copy link
Author

ndmitchell commented Feb 17, 2019

Hmm, actually, it's going to change the Binary instance for LicenseId, so things saved with the old won't still work. Is that an issue?

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 18, 2019

It should be fine, since we don't support reading local build infos generated by old Cabal versions (you get a message telling you to re-run configure). May be worth bumping the master version to 2.5.1 so that people using master snapshots don't get weird errors.

@hvr

This comment has been minimized.

Copy link
Member

hvr commented Feb 18, 2019

I'm strongly against this, as this would mean that we'd end up with orphan instances somewhere, cause Generic instances are useful especially for non-trivial types such as LicenseId. Compile times are no reason to cripple an API, and 30s more are not much in the overall build time of Cabal tbh. If anything, we should try to optimise GHC than to cause regressions in APIs.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 18, 2019

I don't understand the argument about orphan instances.

@harpocrates

This comment has been minimized.

Copy link
Collaborator

harpocrates commented Feb 18, 2019

I think Herbert is suggesting that consumers of Cabal are likely to be interested in Generic instances of non-trivial types such as LicenseId and may end up declaring their own orphan instances of Generic LicenseId.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 18, 2019

OK, makes sense.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 18, 2019

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 18, 2019

Since there are strong objections, I'm closing this as wontfix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.