module system: rework module merging #45038

symphorien · 2018-08-14T19:42:25Z

The asymptotic complexity is now much lower.

Here is a comparision (before on the left, after on the right) of

NIX_COUNT_CALLS=1 NIX_SHOW_STATS=1 nix-instantiate nixos/release.nix -A tests.xfce --arg supportedSystems "[builtins.currentSystem]"

All the functions called more than 100000 times are gone, time decreased by more than 20% and memory allocations are down by 5%.

evaluation statistics:                                  │  evaluation statistics:
  time elapsed: 4.60492                                 │    time elapsed: 3.46439
  size of a value: 24                                   │    size of a value: 24
  size of an attr: 24                                   │    size of an attr: 24
  environments allocated count: 2712839                 │    environments allocated count: 2002955
  environments allocated bytes: 72248584                │    environments allocated bytes: 55187536
  list elements count: 1740105                          │    list elements count: 2148240
  list elements bytes: 13920840                         │    list elements bytes: 17185920
  list concatenations: 93532                            │    list concatenations: 101331
  values allocated count: 4759837                       │    values allocated count: 4190928
  values allocated bytes: 114236088                     │    values allocated bytes: 100582272
  sets allocated: 514598 (105587848 bytes)              │    sets allocated: 573743 (120355624 bytes)
  right-biased unions: 139348                           │    right-biased unions: 171069
  values copied in right-biased unions: 2733790         │    values copied in right-biased unions: 3287668
  symbols in symbol table: 84721                        │    symbols in symbol table: 84726
  size of symbol table: 2645078                         │    size of symbol table: 2645123
  number of thunks: 2897657                             │    number of thunks: 2976651
  number of thunks avoided: 2461400                     │    number of thunks avoided: 2503617
  number of attr lookups: 2132515                       │    number of attr lookups: 1330072
  number of primop calls: 1130033                       │    number of primop calls: 1142590
  number of function calls: 2498497                     │    number of function calls: 1787020
  total allocations: 305993360 bytes                    │    total allocations: 293311352 bytes
  current Boehm heap size: 402718720 bytes              │    current Boehm heap size: 402718720 bytes
  total Boehm heap allocations: 381001232 bytes         │    total Boehm heap allocations: 359379232 bytes
calls to 64 primops:                                    │  calls to 64 primops:
    271292 map                                          │      254183 map
    152902 elemAt                                       │      152902 elemAt
     82968 length                                       │       82968 length
     76741 isAttrs                                      │       76741 isAttrs
--------------------------------------------------------│       71367 attrNames
--------------------------------------------------------│       70573 foldl'
     68000 head                                         │       68000 head
     51625 attrNames                                    │  ---------------------------------------------------------
     49651 listToAttrs                                  │       49651 listToAttrs
     43540 foldl'                                       │+ +-- 56 lines: 32388 elem·································
     33324 concatLists                                  │  calls to 2781 functions:
+--- 55 lines: 32388 elem·······························│  ---------------------------------------------------------
calls to 2772 functions:                                │  ---------------------------------------------------------
    465426 anonymous function at lib/modules.nix:202:30 │  ---------------------------------------------------------
    266781 anonymous function at lib/modules.nix:208:30 │       57750 'chooseDevOutputs' at lib/attrsets.nix:461:22
    122302 anonymous function at lib/modules.nix:217:22 │       54241 anonymous function at lib/attrsets.nix:200:25
     57750 'chooseDevOutputs' at lib/attrsets.nix:461:22│       43264 'fold'' at lib/lists.nix:39:15
--------------------------------------------------------│       39072 'bothHave' at lib/modules.nix:274:22
     43264 'fold'' at lib/lists.nix:39:15               │  ---------------------------------------------------------
     42397 anonymous function at lib/attrsets.nix:200:25│       33479 'optional' at lib/lists.nix:199:14
     39072 'bothHave' at lib/modules.nix:253:22         │       33479 'optional' at lib/lists.nix:199:20
     33479 'optional' at lib/lists.nix:199:14           │       31721 anonymous function at lib/modules.nix:218:17
     33479 'optional' at lib/lists.nix:199:20           │       31721 anonymous function at lib/modules.nix:218:24
     31580 anonymous function at lib/lists.nix:103:38   │       30882 anonymous function at lib/attrsets.nix:333:23
     31580 anonymous function at lib/lists.nix:103:41   │       28420 'and' at lib/trivial.nix:35:9
     30882 anonymous function at lib/attrsets.nix:333:23│       28420 'and' at lib/trivial.nix:35:12
     28420 'and' at lib/trivial.nix:35:9                │       28420 anonymous function at lib/attrsets.nix:426:71

Testing: the hashes of tests are the same according to nox-review --with-tests

The change to foldAttrs is unrelated but the code is more readable and as marginally faster as before.

infinisil

Could you maybe also add some explanatory comments? The old code had some lines as well, would be good to not decrease this amount, as the code is rather hard to get already

Other than these things, I don't see a reason not to merge this.

Ping @edolstra @nbp @Profpatsch

infinisil · 2018-08-19T21:25:36Z

lib/modules.nix

+          ) acc (attrNames module.${attr})
+        ) {} modules;
+      declsByName = byName "options"
+        (module: option: [{ inherit (module) file; options= option; }])


Since this is really only a single option afaik, I think it makes sense to rename options to option here, so options= option -> inherit option

This does not work. The attrset here must be a module for recursive calls, and a module has its options in the attribute... options.

globin

LGTM!

globin · 2018-08-19T23:39:42Z

lib/modules.nix

+        (module: value: [{ inherit (module) file; inherit value; }])
+        configs;
+    in
+    (mapAttrs (name: decls:


flip mapAttrs declsByName (name: decls:... would be more readable

symphorien · 2018-08-20T19:32:30Z

@infinisil you are right about the comments. I readded them as such. notably I don't understand the third. I have no idea what decls' is for.

samueldr · 2018-08-27T02:38:06Z

@infinisil could you refresh your review?

@vcunat @edolstra any opinions? Thinking of merging this before branching off.

infinisil · 2018-08-27T13:25:27Z

lib/modules.nix

+        ) {} modules;
+      # an attrset 'name' => list of submodules that declare ‘name’.
+      declsByName = byName "options"
+        (module: option: [{ inherit (module) file; options= option; }])


I was hoping you would fix the spacing here, but oh well

The asymptotic complexity is now much lower.

symphorien requested review from edolstra and nbp as code owners August 14, 2018 19:42

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Aug 14, 2018

samueldr added this to the 18.09 milestone Aug 14, 2018

infinisil requested changes Aug 19, 2018

View reviewed changes

globin approved these changes Aug 19, 2018

View reviewed changes

symphorien force-pushed the optopt branch from ebc68a7 to 59476fd Compare August 20, 2018 19:30

infinisil approved these changes Aug 27, 2018

View reviewed changes

module system: rework module merging

526d604

The asymptotic complexity is now much lower.

symphorien force-pushed the optopt branch from 59476fd to 526d604 Compare August 27, 2018 15:12

infinisil merged commit 9113696 into NixOS:master Aug 30, 2018

infinisil added the 6.topic: module system About NixOS module system internals label Mar 19, 2020

symphorien deleted the optopt branch March 21, 2020 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module system: rework module merging #45038

module system: rework module merging #45038

symphorien commented Aug 14, 2018 •

edited

infinisil left a comment •

edited

infinisil Aug 19, 2018

symphorien Aug 20, 2018

infinisil Aug 20, 2018

globin left a comment

globin Aug 19, 2018

symphorien Aug 20, 2018

symphorien commented Aug 20, 2018

samueldr commented Aug 27, 2018

infinisil Aug 27, 2018

symphorien Aug 27, 2018

module system: rework module merging #45038

module system: rework module merging #45038

Conversation

symphorien commented Aug 14, 2018 • edited

infinisil left a comment • edited

Choose a reason for hiding this comment

infinisil Aug 19, 2018

Choose a reason for hiding this comment

symphorien Aug 20, 2018

Choose a reason for hiding this comment

infinisil Aug 20, 2018

Choose a reason for hiding this comment

globin left a comment

Choose a reason for hiding this comment

globin Aug 19, 2018

Choose a reason for hiding this comment

symphorien Aug 20, 2018

Choose a reason for hiding this comment

symphorien commented Aug 20, 2018

samueldr commented Aug 27, 2018

infinisil Aug 27, 2018

Choose a reason for hiding this comment

symphorien Aug 27, 2018

Choose a reason for hiding this comment

symphorien commented Aug 14, 2018 •

edited

infinisil left a comment •

edited