Skip to content

module system: small eval optimization#54637

Merged
danbst merged 3 commits intoNixOS:masterfrom
danbst:small-eval-optimization
Jan 30, 2019
Merged

module system: small eval optimization#54637
danbst merged 3 commits intoNixOS:masterfrom
danbst:small-eval-optimization

Conversation

@danbst
Copy link
Contributor

@danbst danbst commented Jan 26, 2019

Motivation for this change

Small NixOS eval optimization (~2.5% runtime, ~5% memory)

  1. Replace foldl'+foldl'+ attrNames -> foldl' + mapAttrs
  2. Replace listToAttrs + mapAttrsToList -> mapAttrs
  3. Deprecate finally types.optionSet and remove some (most?) related code
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 26, 2019
@infinisil
Copy link
Member

infinisil commented Jan 26, 2019

Oh nice! I didn't know the options attribute of mkOption could be fully replaced by just setting the type. This might enable me to finish my small experiment of having a proper NixOps module, bringing lots of cool stuff.

@infinisil
Copy link
Member

infinisil commented Jan 28, 2019 •
edited
Loading

Can you put a more fitting description for the first 2 commits? Seems good to merge then

@danbst danbst force-pushed the small-eval-optimization branch from 37da1fc to 5747041 Compare January 28, 2019 13:23
@danbst danbst requested a review from thoughtpolice as a code owner January 28, 2019 13:23
@danbst danbst force-pushed the small-eval-optimization branch from 5747041 to 30748a8 Compare January 28, 2019 13:25
@danbst danbst removed the request for review from thoughtpolice January 28, 2019 13:26
@danbst
Copy link
Contributor Author

danbst commented Jan 28, 2019

@infinisil should be good now

infinisil
infinisil approved these changes Jan 28, 2019
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

I'd like to take @nbp, @Profpatsch or @rycee to take a look at this as well.

One thing that could be changed is to throw an error when people pass an options argument to mkOption, this then corresponds to optionSet's behavior

@danbst
Copy link
Contributor Author

danbst commented Jan 28, 2019

options and optionSet were deprecated for 5 years at least (70a2c54#diff-2d2cc23a01923ed9f342b937c087d5b1R23). I've mentioned in release doc that we finally remove types.optionSet. Maybe I should also mention that options argument to mkOption is removed as well.

rycee
rycee reviewed Jan 29, 2019
View reviewed changes
nixos/doc/manual/release-notes/rl-1903.xml Outdated
Copy link
Member

@rycee rycee Jan 29, 2019

Choose a reason for hiding this comment

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

I think this needs more strong wording since "deprecated" might imply that it is still actually working. I would suggest something like

<literal>types.optionSet</literal> has been removed after a long deprecation period. Use <literal>types.submodule</literal> instead.

I guess "removed" is not technically correct but I think it is morally correct 😉

rycee
rycee reviewed Jan 29, 2019
View reviewed changes
nixos/modules/testing/service-runner.nix Outdated
Copy link
Member

@rycee rycee Jan 29, 2019

Choose a reason for hiding this comment

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

I'd suggest changing the formatting to

opts = { config, name, ... }: {
  options.runner = mkOption {
    internal = true;
    description = ''
      A script that runs the service outside of systemd,
      useful for testing or for using NixOS services outside
      of NixOS.
    '';
  };
  config.runner = makeScript name config;
};

rycee
rycee reviewed Jan 29, 2019
View reviewed changes
Copy link
Member

@rycee rycee left a comment

Choose a reason for hiding this comment

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

Haven't checked super carefully but I'm liking it a lot! Very nice to clean up this old options code.

@danbst
Copy link
Contributor Author

danbst commented Jan 30, 2019

@rycee @infinisil is release doc good now?

@GrahamcOfBorg GrahamcOfBorg added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 30, 2019
@danbst danbst force-pushed the small-eval-optimization branch from 7dac02f to 5f8106d Compare January 30, 2019 13:31
@GrahamcOfBorg GrahamcOfBorg removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 30, 2019
@danbst danbst force-pushed the small-eval-optimization branch from 5f8106d to 27982b4 Compare January 30, 2019 22:41
@danbst danbst merged commit 30c3123 into NixOS:master Jan 30, 2019
@danbst danbst deleted the small-eval-optimization branch January 30, 2019 22:42
@Profpatsch
Copy link
Member

Profpatsch commented Jan 31, 2019

Looks good. Am not particularly deep into the implementation of the module system. Nix needs an hlint. :P

@rycee
Copy link
Member

rycee commented Jan 31, 2019

Agreed that it looks good 👍

delroth added a commit to delroth/nixpkgs that referenced this pull request Feb 2, 2019
@delroth
nixos/tasks/encrypted-devices: fix regression from NixOS#54637
3674bdf 
27982b4 introduced a bug when
refactoring the encrypted-devices module, causing some encrypted
filesystem options to not be recognized anymore.

See e.g. https://hydra.nixos.org/build/88145490
Ekleog added a commit that referenced this pull request Feb 3, 2019
@Ekleog
Merge branch 'pr-55088'
9cd7762 
* pr-55088:
  nixos/tasks/encrypted-devices: fix regression from #54637
@ck3d ck3d mentioned this pull request Feb 7, 2019
Closed
@infinisil infinisil added the 6.topic: module system About "NixOS" module system internals label Mar 19, 2020
@infinisil infinisil mentioned this pull request Sep 4, 2020
Merged
1 task
@infinisil infinisil mentioned this pull request Mar 17, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rycee rycee rycee left review comments

@infinisil infinisil infinisil approved these changes

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

Assignees

No one assigned

Labels

6.topic: module system About "NixOS" module system internals 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@danbst @infinisil @Profpatsch @rycee @matthewbauer @GrahamcOfBorg