perf: Remove the quadratic behavior in haskellPackages.ghcWithPackages #194391

guibou · 2022-10-04T10:25:03Z

Description of changes

This code was using lib.closePropagation which internally do a recursion on the dependencies of ghcWithPackages and returns all the haskell dependencies.

lib.closeProgation is implemented in pure nix and uses an unique function for list which is quadratic and does "true" equality, which needs deep set comparison.

Things done

Instead, we use the builtins.genericClosure which is implemented as a builtin and uses a more efficient sorting feature.

Note that genericClosure needs a key to discriminate the values, we used the outPath which is unique and orderable.

On benchmarks, it performs up to 15x time faster.

Testing / Benchmark

I've tested this implementation on a large haskell codebase where I'm using nix as a build system. I have something like 250 calls to ghcWithPackages. When building everything, nix build --dry-run was tacking up to 20s, now it runs in 1.5s.

In order to compare, I've created the following benchmark, drop this file (name it fiz.nix) at the root of nixpkgs

let
  pkgs = import ./. { };

  haskellPackages = p:
    with p; [
      ad
      aeson
      AesonBson
      aeson-pretty
      algebraic-graphs
      async
      attoparsec
      barbies
      base
      base16
      base64-bytestring
      base-noprelude
      bazel-runfiles
      binary
      blaze-html
      bson
      bytestring
      case-insensitive
      casing
      cassava
      cereal
      cereal-text
      cereal-unordered-containers
      cereal-uuid
      cereal-vector
      clock
      conduit
      conduit-extra
      containers
      criterion
      cryptohash-sha1
      data-default
      deepseq
      directory
      erf
      exceptions
      extra
      filepath
      foldl
      formatting
      generic-data
      generics-sop
      ghc-prim
      githash
      Glob
      gloss
      hashable
      hedgehog
      hedis
      heredoc
      hmatrix
      hmatrix-nlopt
      hspec
      hspec-core
      hspec-wai
      http-client
      http-conduit
      http-media
      http-types
      hvega
      hxt
      integration
      JuicyPixels
      katip
      KdTree
      lens
      lexer-applicative
      linear
      megaparsec
      mime-types
      mmorph
      monad-classes
      monad-control
      mono-traversable
      mtl
      mwc-random
      openapi3
      optparse-applicative
      optparse-generic
      ordered-containers
      pandoc
      parallel
      parsec
      parser-combinators
      pqueue
      pretty
      pretty-show
      pretty-simple
      primitive
      process
      profunctors
      PyF
      QuickCheck
      quickcheck-instances
      quickcheck-text
      random
      random-fu
      reflection
      regex
      regex-applicative
      regex-compat
      resourcet
      retry
      safe
      safe-exceptions
      scientific
      selda
      semigroups
      servant
      servant-blaze
      servant-client
      servant-client-core
      servant-conduit
      servant-openapi3
      servant-server
      servant-swagger-ui
      singletons
      singletons-th
      split
      splitmix
      srcloc
      statistics
      stm
      storable-tuple
      streaming
      streaming-bytestring
      strict
      string-conv
      tagged
      tardis
      tasty
      tasty-expected-failure
      tasty-golden
      tasty-hedgehog
      tasty-hunit
      tasty-quickcheck
      tasty-wai
      template-haskell
      temporary
      text
      time
      tls
      transformers
      transformers-base
      trie-simple
      tuples-homogenous-h98
      typed-process
      uglymemo
      unicode-transforms
      unix
      unliftio
      unliftio-core
      unordered-containers
      url
      utf8-string
      uuid
      uuid-types
      vector
      vector-algorithms
      versions
      vinyl
      wai
      wai-extra
      warp
      yaml
      zip
      zlib
    ];
in builtins.genList (n:
  pkgs.haskellPackages.ghcWithPackages
  (p: pkgs.lib.lists.take n (haskellPackages p))) 167

Then I'm benchmarking with time nix build -f fiz.nix --dry-run

Without this change: 18.6s
With this change: 1.2s

Note that nix will still announce that it will fetch the same numbers of paths, but before that patch, it generates 94 ghc-xxx-with-packages.drv and with this patch it generates 147 distinct derivations. I don't understand why. I suppose that the ordering of the results is changing. I was however able to confirm that it behaves the same (by building my 250 packages and running their test suite).

This change should not (as far as I'm aware) trigger a mass rebuild because it only impacts shell / dynamic environments.

sternenseemann · 2022-10-04T10:44:38Z

@ofborg build tests.haskell.shellFor

guibou · 2022-10-04T11:02:45Z

Damned, thank you @sternenseemann, it fails on shellFor, let's try to understand why and fix that.

sternenseemann · 2022-10-04T11:06:20Z

shellFor has some extra code to merge multiple ghcWithPackages environment, maybe that gets tripped up somehow.

FRidh · 2022-10-04T13:47:59Z

Could this function be put in lib so we can convert other users as well?

guibou · 2022-10-04T14:10:48Z

@FRidh It could yes. In which lib subfile do you think it will be at home?

FRidh · 2022-10-04T14:13:22Z

If the behaviour is the same it could replace the original function. Or is there any reason for not doing that?

guibou · 2022-10-04T14:29:12Z

Will debugging, the new function is missing some values:

I'm trying to understand. That's weird because it is able to build my packages, so there may be a weird corner case I did not anticpiated.

@FRidh I'm not sure the new function is as general as the old one. Because the old was able to return anything which is a set, but the new can only return things which are set with an outPath.

1d0
< /nix/store/014dicp6v8l44jwx0qiadmrk23j4xyk3-hspec-core-2.8.5
3,5d1
< /nix/store/0hgxgp65g0mg5mw58hdikhj0zzwwx7ni-hspec-discover-2.8.5
< /nix/store/1nv8b47dm0z8xf55jfzpmzsrz9fhkyjs-ansi-terminal-0.11.3
< /nix/store/1v197gcfjm47549wadipcbkbv2cmbiv3-profunctors-5.6.2
8,10d3
< /nix/store/3rmmdpg12m6ryrjdpwspyrcbhpwjzq51-StateVar-1.2.2
< /nix/store/4q8w5bsn9phpkz3wbhhxpci1lgjbhjf6-tf-random-0.5
< /nix/store/4ydclrfs2kxbq9195n7hw1qm8nbmnssy-hspec-expectations-0.8.2
12,16d4
< /nix/store/64xbi7926z9sv977p3gv421dnfb50xhi-strict-0.4.0.1
< /nix/store/6dx6pwy5pqx08b72filzbb3b0k5zlyj3-call-stack-0.4.0
< /nix/store/6i2fvi1akhfbm006dm3fgvf9xg2p9cw8-contravariant-1.5.5
< /nix/store/6qmqark6jxbglzbjwmyvn8l25a8ibgzm-quickcheck-io-0.2.0
< /nix/store/6vicys7f43ikc0ych1hmaln7702bbrxy-bifunctors-5.5.13
18d5
< /nix/store/846xg53r9fivzkdmpmr5yh6vkfy196f1-transformers-base-0.4.6
20,23d6
< /nix/store/8rwafhh7nrzifzb2m58rm7naxkarpza5-extensible-exceptions-0.1.1.4
< /nix/store/8vbs4n20qbh95m22sixy279k52cyzcj5-these-1.1.1.1
< /nix/store/99sr0597428hb7ib0b13ijw243agkngn-scientific-0.3.7.0
< /nix/store/bhwzkqg404gnlj70cjgjqcphbqxwfs0p-colour-2.3.6
26,27d8
< /nix/store/cq2i4wjw061sgn2qhzcgf3hky6mw991b-setenv-0.1.1.3
< /nix/store/dfv4ai9hi2f9avlammpf1qyhhslhmjxb-assoc-1.0.2
29,30d9
< /nix/store/fh0dc4ndn60b475hb1l36kpym5i7yhkk-regex-tdfa-1.3.2
< /nix/store/fnh9rwax3x0r8qjdh6jiv5yyy44ignbg-splitmix-0.1.0.4
33,34d11
< /nix/store/gdarb3wg3y2wi824wbnl2hvsc3v7np65-clock-0.8.3
< /nix/store/h9z3amqgmrpdlrc8bmcms6mf3nqy8dhj-old-locale-1.0.0.7
37d13
< /nix/store/hzg99hggqw6g48zlfwwqa6mrkhp6jawf-OneTuple-0.3.1
39,40d14
< /nix/store/i4rylsmkbkgzvffkvpbgdppacbf8wz7v-QuickCheck-2.14.2
< /nix/store/ipq5z3xa1d84ica0qfn15z96r9hv9xmh-xml-1.3.14
42d15
< /nix/store/jsxhrq9yjfq8s84xdrdad8zhrhzkqpqn-th-abstraction-0.4.5.0
44,45d16
< /nix/store/kgi26v8anani7ja7svrqj94dn1hgyri0-kan-extensions-5.2.5
< /nix/store/kh71s1yn68wib6kzm8m3r24v222ymabj-integer-logarithms-1.0.3.1
47,48d17
< /nix/store/l89yqkcjssmjwiggj84znl2ac8mkaadv-regex-posix-0.96.0.1
< /nix/store/lq8ilf068imv54np2l1s4zh91n1vkgv6-parallel-3.2.2.0
50d18
< /nix/store/mvfvj06vpiyjv4g25i4p490kjnfr4jpg-free-5.1.9
54d21
< /nix/store/p8cr9aav175i6lqicjffjjva58zkcyi0-regex-base-0.94.0.2
56d22
< /nix/store/w4bia9by4srqb0zz9zn01d64hawnhrd3-invariant-0.5.6
58,63d23
< /nix/store/wasm9bckfrxm9qgwfng0xvpc3nlikfxi-primitive-0.7.3.0
< /nix/store/wqxdrq2bi7wzjh561gl94k2bnik0v68z-indexed-traversable-instances-0.1.1.1
< /nix/store/yhpsk4bxql6dfnc8m86wd29y237in2zm-binary-orphans-1.0.3
< /nix/store/yp5fr91qkgh0vh3bvkgfgzkzrxiksq7g-ansi-wl-pprint-0.6.9
< /nix/store/z1xnays4jk1c09b70728rw3hpnwzsjqq-hostname-1.0
< /nix/store/z653l9xrs9hnv16ymiaa3naj5q6xcjqv-pretty-terminal-0.1.0.0
65d24
< /nix/store/zgqkpq7j5yrvnyjh09988i2iy42mahbp-comonad-5.0.8

guibou · 2022-10-04T14:48:13Z

Haha, I'm just stupid. Simple mistake when copy pasting (I've refactored the item.propagatedNativeBuildInputs or [ ] and missed the item.val).

It is a bit slower, 1.2s to 1.5s, but nothing dramatic.

guibou · 2022-10-04T15:00:16Z

@ofborg build tests.haskell.shellFor

guibou · 2022-10-04T15:03:45Z

The tests should now pass (at least they pass locally).

As suggested by @FRidh, I replaced the lib.closePropagation by the new implementation (with a conditional based on the existence of builtins.genericClosure).

Looks like the behavior should be highly similar, except for the ordering of the results. There are not much usage of this function:

nixos/modules/services/audio/mopidy.nix:    paths = closePropagation cfg.extensionPackages;
pkgs/applications/editors/eclipse/default.nix:            filter (x: x ? isEclipsePlugin) (closePropagation plugins);
pkgs/development/compilers/yosys/default.nix:      paths = lib.closePropagation plugins;
pkgs/development/haskell-modules/hoogle.nix:  docPackages = lib.closePropagation
pkgs/development/haskell-modules/with-packages-wrapper.nix:  paths         = lib.filter (x: x ? isHaskellLibrary) (lib.closePropagation packages);
pkgs/development/idris-modules/with-packages.nix:let paths = lib.closePropagation packages;
pkgs/development/tools/misc/hydra/unstable.nix:    paths = with perlPackages; lib.closePropagation
pkgs/top-level/perl-packages.nix:  makeFullPerlPath = deps: makePerlPath (lib.misc.closePropagation deps);

A quick look makes me think that we may be safe to do this change on the complete nixpkgs. However it may trigger a massive rebuild, so perhaps it should be merged to another branch. What do you think?

lovesegfault · 2022-10-04T15:29:16Z

Yeah, as-is, this now needs to target staging due to the mass rebuilds

sternenseemann · 2022-10-05T12:10:21Z

@guibou Please add a changelog entry for this to the release notes for 21.11, since it is a breaking change (which is probably not too worrysome, since probably very few downstream users will be using it without having an outPath attribute). It should mention:

Improved performance of lib.closePropagation for e.g. ghcWithPackages.
The breakage, i.e. that every set needs an outPath now.

guibou · 2022-10-05T14:05:50Z

@sternenseemann documentation added (note, I changed the notes for 22.11 instead of 21.11 as you suggested. Please tell me if you were really meaning 21.11). Thank you @teto who also told me to regenerate the db thing.

sternenseemann

LGTM, but please do update your commit message to conform to CONTRIBUTING.md, i.e. it should probably start with lib.closePropagation:

…gation The code of `lib.closePropagation` was internally using a recursion on the dependencies and returns all the derivation directly or indirectly referenced by buildInputs. `lib.closeProgation` is implemented in pure nix and uses an unique function for list which is quadratic and does "true" equality, which needs deep set comparison. Instead, we use the `builtins.genericClosure` which is implemented as a builtin and uses a more efficient sorting feature. Note that `genericClosure` needs a `key` to discriminate the values, we used the `outPath` which is unique and orderable. On benchmarks, it performs up to 15x time faster on a benchmark related to haskellPackages.ghcWithPackages.

guibou · 2022-10-07T16:06:00Z

@sternenseemann done.

spacekitteh · 2022-10-23T21:44:40Z

So, this breaks my dev shell, and I'm not sure how to fix it.

{ pkgs ? import <nixpkgs> {},
  ghc ? pkgs.haskell.packages.ghc924,
llvm ? pkgs.llvmPackages_13.llvm,
}:

with pkgs;

let
  ghc' = ghc.ghcWithPackages(ps: with ps; [
haskell-language-server cabal-install
profiterole profiteur 
ghc
  ]);
in
pkgs.stdenv.mkDerivation {
  name = "cosmothought";
  nativeBuildInputs = [pkg-config pcre];
  buildInputs = [
    zlib icu ncurses.dev llvm icu racket pcre haskellPackages.alex haskellPackages.happy
    bzip2 ghc'
    ];
  shellHook = "eval $(egrep ^export ${ghc'}/bin/ghc)";
}

The last few lines of the trace are:

❯ nix-shell --show-trace
error: attribute 'outPath' missing

       at /nix/store/r2v7sig14g93wwf6ljr0jb0q1v2d1ngh-nixpkgs/nixpkgs/lib/deprecated.nix:171:15:

          170|       startSet = builtins.map (x: {
          171|         key = x.outPath;
             |               ^
          172|         val = x;

       … while evaluating 'closePropagationFast'

       at /nix/store/r2v7sig14g93wwf6ljr0jb0q1v2d1ngh-nixpkgs/nixpkgs/lib/deprecated.nix:168:26:

          167|   # See https://github.com/NixOS/nixpkgs/pull/194391 for details.
          168|   closePropagationFast = list:
             |                          ^
          169|     builtins.map (x: x.val) (builtins.genericClosure {

       … from call site

       at /nix/store/r2v7sig14g93wwf6ljr0jb0q1v2d1ngh-nixpkgs/nixpkgs/pkgs/development/haskell-modules/with-packages-wrapper.nix:57:57:

           56|   packageCfgDir = "${libDir}/package.conf.d";
           57|   paths         = lib.filter (x: x ? isHaskellLibrary) (lib.closePropagation packages);
             |                                                         ^
           58|   hasLibraries  = lib.any (x: x.isHaskellLibrary) paths;

       … while evaluating anonymous lambda

       at /nix/store/r2v7sig14g93wwf6ljr0jb0q1v2d1ngh-nixpkgs/nixpkgs/pkgs/development/haskell-modules/with-packages-wrapper.nix:17:1:

           16| #   (hpkgs: [ hpkgs.mtl hpkgs.lens ])
           17| selectPackages:
             | ^
           18|

       … from call site

       at /home/spacekitteh/code/cosmothought/shell.nix:11:10:

           10| let
           11|   ghc' = ghc.ghcWithPackages(ps: with ps; [
             |          ^

sternenseemann · 2022-10-23T21:49:23Z

@spacekitteh

ghc ? pkgs.haskell.packages.ghc924 introduces the ghc name into the scope statically…
…which takes precedence over dynamic resolution via with. Specifically with ps; [ /* … */ ghc ] will then evaluate to effectively [ haskell.packages.ghc94 ] which of course lacks an outPath attribute.

spacekitteh · 2022-10-23T21:59:11Z

@sternenseemann Ok, removing the ghc fixed it, but why does it not have an outPath attribute?

sternenseemann · 2022-10-23T22:00:52Z

haskell.packages.ghc94 is a package set of derivations, not a single derivation. ps.ghc is a derivation, but shadowed due to Nix's scoping rules.

spacekitteh · 2022-10-23T22:03:28Z

Ah. So what would it have been bringing in before this patch?

sternenseemann · 2022-10-23T22:05:42Z

Nothing, as it would have been filtered out by this line in with-packages-wrapper.nix:

  paths         = lib.filter (x: x ? isHaskellLibrary) (lib.closePropagation packages);

guibou requested review from cdepillabout, sternenseemann and maralorn as code owners October 4, 2022 10:25

github-actions bot added the 6.topic: haskell label Oct 4, 2022

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Oct 4, 2022

sternenseemann mentioned this pull request Oct 4, 2022

🦝 NixOS 22.11 - Feature Freeze & Release Blockers #194208

Closed

31 tasks

guibou force-pushed the fast_haskell_ghc_with_packages branch from 6bc2c86 to 74455b6 Compare October 4, 2022 14:59

guibou requested review from edolstra, nbp and infinisil as code owners October 4, 2022 14:59

guibou force-pushed the fast_haskell_ghc_with_packages branch from 74455b6 to aa5454e Compare October 4, 2022 14:59

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ and removed 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Oct 4, 2022

guibou changed the base branch from master to staging October 4, 2022 15:51

teto removed request for a team, adisbladis, mweinelt, jbedo, jtojnar, edolstra, FRidh, alyssais, jonringer, mkg20001 and infinisil October 4, 2022 16:07

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Oct 5, 2022

sternenseemann approved these changes Oct 6, 2022

View reviewed changes

bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 6, 2022

guibou added 2 commits October 7, 2022 18:03

nixos/doc: add release-notes entries for lib.closePropagation changes

a2cd604

guibou force-pushed the fast_haskell_ghc_with_packages branch from ef5663c to a2cd604 Compare October 7, 2022 16:05

lovesegfault merged commit 34c73b3 into NixOS:staging Oct 7, 2022

guibou deleted the fast_haskell_ghc_with_packages branch October 11, 2022 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Remove the quadratic behavior in haskellPackages.ghcWithPackages #194391

perf: Remove the quadratic behavior in haskellPackages.ghcWithPackages #194391

guibou commented Oct 4, 2022

sternenseemann commented Oct 4, 2022

guibou commented Oct 4, 2022

sternenseemann commented Oct 4, 2022

FRidh commented Oct 4, 2022

guibou commented Oct 4, 2022

FRidh commented Oct 4, 2022

guibou commented Oct 4, 2022

guibou commented Oct 4, 2022 •

edited

Loading

guibou commented Oct 4, 2022

guibou commented Oct 4, 2022

lovesegfault commented Oct 4, 2022 •

edited

Loading

sternenseemann commented Oct 5, 2022

guibou commented Oct 5, 2022

sternenseemann left a comment

guibou commented Oct 7, 2022

spacekitteh commented Oct 23, 2022

sternenseemann commented Oct 23, 2022

spacekitteh commented Oct 23, 2022

sternenseemann commented Oct 23, 2022

spacekitteh commented Oct 23, 2022

sternenseemann commented Oct 23, 2022

perf: Remove the quadratic behavior in haskellPackages.ghcWithPackages #194391

perf: Remove the quadratic behavior in haskellPackages.ghcWithPackages #194391

Conversation

guibou commented Oct 4, 2022

Description of changes

Things done

Testing / Benchmark

sternenseemann commented Oct 4, 2022

guibou commented Oct 4, 2022

sternenseemann commented Oct 4, 2022

FRidh commented Oct 4, 2022

guibou commented Oct 4, 2022

FRidh commented Oct 4, 2022

guibou commented Oct 4, 2022

guibou commented Oct 4, 2022 • edited Loading

guibou commented Oct 4, 2022

guibou commented Oct 4, 2022

lovesegfault commented Oct 4, 2022 • edited Loading

sternenseemann commented Oct 5, 2022

guibou commented Oct 5, 2022

sternenseemann left a comment

Choose a reason for hiding this comment

guibou commented Oct 7, 2022

spacekitteh commented Oct 23, 2022

sternenseemann commented Oct 23, 2022

spacekitteh commented Oct 23, 2022

sternenseemann commented Oct 23, 2022

spacekitteh commented Oct 23, 2022

sternenseemann commented Oct 23, 2022

guibou commented Oct 4, 2022 •

edited

Loading

lovesegfault commented Oct 4, 2022 •

edited

Loading