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.

Sign up for GitHub

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

Jump to bottom

add more NFData instances #305

Closed
wants to merge 3 commits into from
Closed

add more NFData instances #305

wants to merge 3 commits into from

Conversation

LightAndLight
Copy link
Contributor

@LightAndLight LightAndLight commented Feb 6, 2021 •
edited
Loading

adds:

  • NFData2 HashMap
  • NFData k => NFData1 (HashMap k)
  • NFData1 HashSet

closes #304

@treeowl
Copy link
Collaborator

treeowl commented Feb 6, 2021

I'm all in favor of the new instances. Please make sure performance is not lost for existing instances. Either

  1. Check that they produce identical Core (including unfoldings, arity, and unfolding guidance)
  2. Benchmark with several instances, especially including cheap ones (e.g., ones where deepseq = seq, where rnf = const (), and where rnf just forces one field of a record), where the map is various sizes and already in normal form.
  3. Leave the existing instances alone and just add the new ones on top. This is a bit silly, of course, but it's certainly the easiest way to be confident you won't goof anything up.

@LightAndLight
Copy link
Contributor Author

LightAndLight commented Feb 6, 2021 via email

@treeowl
Copy link
Collaborator

treeowl commented Feb 6, 2021 via email

@LightAndLight
Copy link
Contributor Author

Given that, I decided to leave the existing instances as-is and only add the new ones.

@sjakobi
Copy link
Member

sjakobi commented Feb 8, 2021

Can you add some CPP, so the new instances are only defined with recent enough versions of deepseq?

@treeowl
Copy link
Collaborator

treeowl commented Feb 8, 2021 via email

@sjakobi
Copy link
Member

sjakobi commented Feb 8, 2021

Or should we bump the minimum deepseq bound?

I think that might limit GHC compatibility too much for people who can't easily upgrade boot libraries separately from the compiler.

@treeowl
Copy link
Collaborator

treeowl commented Feb 8, 2021 via email

@sjakobi
Copy link
Member

sjakobi commented Feb 8, 2021

The latest deepseq supports base>=4.5. Even we don't go back that far!

True. But AFAIK using the latest deepseq with e.g. GHC 8.2 is rather tricky with e.g. Nix.

@treeowl
Copy link
Collaborator

treeowl commented Feb 8, 2021 via email

@sjakobi
Copy link
Member

sjakobi commented Feb 8, 2021

I don't remember the details. AFAIK it's non-trivial to update boot libraries with Nix. I think I once tried to update containers or bytestring and it was a PITA.

@treeowl
Copy link
Collaborator

treeowl commented Feb 8, 2021 via email

@sjakobi
Copy link
Member

sjakobi commented Feb 8, 2021

OK. If that's easier we'll need to update the deepseq bound and fix CI.

@treeowl
Copy link
Collaborator

treeowl commented Feb 8, 2021 via email

@treeowl
Copy link
Collaborator

treeowl commented Feb 12, 2021

CI is failing.

@LightAndLight
Copy link
Contributor Author

LightAndLight commented Feb 12, 2021 via email

@treeowl
Copy link
Collaborator

treeowl commented Feb 12, 2021

That's definitely my inclination, yes.

@sjakobi sjakobi mentioned this pull request Feb 12, 2021
Open
@sjakobi
Copy link
Member

sjakobi commented Feb 16, 2021

Have you reached consensus on how you'd like me to fix the version issue? It looks like it leans toward "increase the deepseq bound".

My preference is for retaining backward compatibility with deepseq < 1.4.3, but raising the bound doesn't seem too bad either.

@sjakobi
Copy link
Member

sjakobi commented Mar 14, 2021

On third thought, I'm fine with raising the deepseq bound for now. If this ends up causing problems for users we can probably improve compatibility later on.

@LightAndLight Could you raise the bound and fix CI, please?

sjakobi
sjakobi reviewed Mar 15, 2021
View reviewed changes
Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

I believe --installed=-deepseq needs to be added to the command that generates our Travis config, so it can use a non-bundled version.

https://github.com/haskell-unordered-containers/unordered-containers/blob/master/.travis.yml#L3

unordered-containers.cabal Outdated Show resolved Hide resolved
@sjakobi
Copy link
Member

sjakobi commented Mar 27, 2021 •
edited
Loading

I believe bytestring is the last package where the installed constraint needs to be removed. You can check this locally by using the cabal.project.local that is generated during the Travis run and building with GHC-8.0.2, GHC-7.10.3 and GHC-7.8.4.

@sjakobi
Copy link
Member

sjakobi commented Apr 29, 2021

I'd like to cut a release fairly soon and ideally include this PR. Do you think you can finish this PR in the next, say, 2 weeks, @LightAndLight? Otherwise I can take a stab at it, but I'd probably take the CPP route.

@LightAndLight
Copy link
Contributor Author

@sjakobi I copied the cabal.project.local from the failing CI build and cabal new-build -w {ghc-7.8.4,ghc-7.10.3,ghc-8.0.2} all succeeded on my machine. I don't understand what's wrong with the CI build- can you help me?

@sjakobi
Copy link
Member

sjakobi commented May 9, 2021

I can reproduce the error from https://travis-ci.com/github/haskell-unordered-containers/unordered-containers/jobs/495652251 by using the following cabal.project.local:

constraints: Cabal installed
constraints: array installed
constraints: base installed
constraints: directory installed
constraints: filepath installed
constraints: ghc installed
constraints: ghc-boot installed
constraints: ghc-boot-th installed
constraints: ghc-prim installed
constraints: ghci installed
constraints: haskeline installed
constraints: hoopl installed
constraints: hpc installed
constraints: integer-gmp installed
constraints: pretty installed
constraints: rts installed
constraints: template-haskell installed
constraints: terminfo installed
constraints: transformers installed
constraints: xhtml installed

and the following command:

cabal build -w ghc-8.0.2 --enable-benchmarks --enable-tests

I think the error message is pretty unhelpful, but I noticed that the process dependency might be constrained by its directory and filepath dependencies. Indeed removing these constraints from the cabal.project.local makes the command succeed.

With 7.10.3 I also have to remove the constraint on pretty and increase --max-backjumps.

I don't have 7.8.4 installed, so I haven't been able to test with it.

I'm not sure how much more pain it would take to fix the CI setup. We can probably take a shortcut and disable the benchmarks for GHC < 8.

I'm slightly worried that some users might end up going down this constraint-fighting path too though, so I think we should rather switch to the CPP route. I'm sorry that I didn't insist on it in the first place.

@sjakobi sjakobi mentioned this pull request May 13, 2021
Closed
@sjakobi
Copy link
Member

sjakobi commented May 13, 2021

Merged via #314. Thank you, @LightAndLight!

@sjakobi sjakobi closed this May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjakobi sjakobi sjakobi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

instance NFData k => NFData1 (HashMap k) and instance NFData2 HashMap
3 participants
@LightAndLight @treeowl @sjakobi