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

[WIP] Lazy attribute names #4154

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from
Draft

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Oct 16, 2020

This is an initial prototype of lazy attribute names, allowing things like:

(throw "" // { x = 0; }).x
-> 0

See #4090 for more info.

This is a work-in-progress. I opened this PR to keep track of what needs to be done still and how far I've gotten, and maybe get some help with it too. In particular, the main problem right now is that this currently makes Nix about 40% slower.

Ping @edolstra

Todo

  • Figure out why it's slower. For NIX_SHOW_STATS=1 nix-instantiate '<nixpkgs>' -A firefox, this branch has better numbers in all categories, except for bytes allocated for Value's since those are now 40 instead of 24 bytes (will have to fix that), and the CPU time (that's what's 40% slower). I doubt these extra bytes are the reason though, but this doesn't make it easy to debug. It's not anymore!
  • Ideally deduplicate the methods that were mostly copied (if it can be done without worse performance)
  • Try to remove the maybeThunk in ExprSelect::{eval,evalAttr}, not sure why that's needed. Maybe that's even the thing that causes worse performance -> Didn't cause the worse performance, but now gotten rid with 48511ea
  • Implement the remaining unimplemented Expr*::evalAttr methods
  • Implement ExprOpHasAttr, builtins.{get,has}Attr using evalValueAttr
  • Add evalAttr support for primops, to allow primops to be lazy in attribute names
  • Do infinite recursion detection for evalValueAttr (this might be tricky, example: let a = {} // a; in a.foo)
  • Implement findAlongAttrPath using evalValueAttr (makes it do looks up lazy), similar for other attribute-traversing functions, find those
  • Make sure the tLazyBinOp Value pointers are nulled so the values get picked up by the GC
  • Split all the evalAttr-related methods into a separate file, eval.cc is too big. Not needed anymore
  • Write tests!
  • Add some comments on how the implementation works, as it's very much non-trivial and took me a while to grok. And this feature makes it even more complicated.
  • Ensure that proper position information is present in all errors

This code should also be generic enough that it should allow lazy lists and strings in the future (e.g. being able to do builtins.elemAt ([ 0 ] ++ throw "") 0). This could be done in a future PR once this is merged.

And for future reference, previous (and later) worse attempts of implementing this are in my branches lazy-attr-names and lazy-attr-names-v2 lazy-attr-names-v4

Infinisil added 2 commits Oct 19, 2020
This is a version of Expr::eval that doesn't necessarily have to
evaluate the value into a non-thunk, while returning a specific
attribute

The evalAttr of expressions that evaluate to a subexpression should call
evalAttr on that subexpression, therefore bubbling up any potential
thunks
@Infinisil Infinisil force-pushed the lazy-attr-names-v3 branch from 0a7ccf2 to 6d7d037 Oct 19, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Oct 20, 2020

Cleaned up the commits into smallish changes, undoing some smarts along the way, which unexpectedly fixed a bug I was encountering with this, nice!

I also did some proper performance measurement, and the results are not as bad as I thought (presumably the smarts I undid were also causing the worse performance). I wrote a little measurement and plotting script to do this:

measure:

#!/usr/bin/env bash
set -euo pipefail

nixCommand() {
  #nix-instantiate '<nixpkgs/nixos>' \
  #  --arg configuration '<nixpkgs/nixos/modules/profiles/demo.nix>' \
  #  -A vm
  nix-instantiate ~/src/nixpkgs -A firefox
}

run() {
  local stats=$(mktemp)

  PATH=$PWD/bin:$PATH NIX_SHOW_STATS=1 NIX_SHOW_STATS_PATH="$stats" nixCommand >/dev/null 2>/dev/null

  jq -r '.cpuTime' "$stats"
  rm "$stats"
}

measure() {
  local name=$1
  local duration=$2
  local dest="times/$name"

  echo "Making sure binary is up-to-date"
  nix-shell --run "make install -j8"

  echo "Clearing any previous results in $dest"
  mkdir -p "$(dirname "$dest")"
  > "$dest"

  echo "Warming up with a single run"
  run >/dev/null

  epochStart=$(date +%s)
  epochEnd=$(( epochStart + duration ))

  echo "Measuring for at least $duration seconds"
  while now=$(date +%s) && [[ "$now" -le "$epochEnd" ]]; do
    result=$(run)
    echo "Measured $result seconds, writing to file. $(( epochEnd - now )) seconds left"
    echo "$result" >> "$dest"
  done
}

collectdata() {
  for f in times/*; do
    jq --arg name "$(basename "$f")" '[ ., inputs ] | map({ "CPU Time" : ., "Version" : $name })' -R "$f"
  done | jq -s '. | map(.[])'
}

plot() {
  collectdata > "data.json"
  # Last nixpkgs version where vega_lite wasn't broken
  vegaLite=$(nix-build --no-out-link https://github.com/NixOS/nixpkgs/archive/e1773ee0bb99e6785e2e06c0931cc8ffa9341d2a.tar.gz -A nodePackages.vega-lite)
  "$vegaLite/bin/vl2svg" plot.json plot.svg
  xdg-open plot.svg
}

case "$1" in
  plot)
    plot
    ;;
  *)
    measure "$1" "$2"
esac

plot.json:

{
  "$schema": "https://vega.github.io/schema/vega-lite/v4.json",
  "description": "Nix performance on `nix-instantiate '<nixpkgs/nixos>' --arg configuration '<nixpkgs/nixos/modules/profiles/demo.nix>' -A vm`",
  "data": {"url": "data.json"},
  "mark": {
    "type": "boxplot",
    "extent": "min-max"
  },
  "encoding": {
    "x": {"field": "Version", "type": "nominal"},
    "color": {"field": "Version", "type": "nominal", "legend": null},
    "y": {
      "field": "CPU Time",
      "type": "quantitative",
      "scale": {"zero": false}
    }
  }
}

To use:

  • Check out the code version you'd like to test
  • Run ./measure some-id 300, which runs the nixCommand in the script for 300 seconds, storing the results in times/some-id
  • Repeat with all the other commits you want to compare against, changing the some-id for each of them
  • Run ./measure plot, which uses Vega Lite to render all the measurements as a box plot

With 0 representing the base commit of this PR (master), 1 the first commit of this PR, 2 the second commit, etc., the plot looks as follows (with ~275 samples):
plot

Note how this is only a tiny bit slower, from ~0.62 to ~0.64! And I do believe some things can be optimized still, so I hope to be able to improve performance with this PR in the end :)

@Infinisil Infinisil force-pushed the lazy-attr-names-v3 branch from 6d7d037 to fd71421 Oct 21, 2020
Infinisil added 4 commits Oct 21, 2020
This introduces an expression base class that can be used for lazy
binary operations, along with a value type for storing partial results
of such expressions
This makes the // operator lazy in attribute names, only evaluates what
is necessary to get a specific attribute.
So as to not increase the sizeof(Value) from 24 bytes to 40 bytes
@Infinisil Infinisil force-pushed the lazy-attr-names-v3 branch from fd71421 to 152048d Oct 21, 2020
Infinisil added 4 commits Oct 21, 2020
I think this is needed so that any variables on the sides get updated
properly

Without this,

  bin/nix-instantiate ~/src/nixpkgs/nixos --arg configuration ~/src/nixpkgs/nixos/modules/profiles/demo.nix -A vm

fails with a segfault

Not sure why this doesn't happen with the commit that makes // lazy
though
The previous implementation relied on uninitialized memory not being a
certain value for it to work
Since it throws inf rec when it doesn't have to sometimes
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Oct 27, 2020

Performance testing with the latest changes reveals that there's pretty much no measurable decrease in performance, yay! I heard you like plots? I give you plot (0 is the base of this PR, each number is an additional commit)
plot

I'll consider the performance problem solved, though some fine tuning may still be done.


Infinite recursion detection

The main problem now is that infinite recursion detection is going to be much trickier, and I haven't figured that out. Previously if you had an unevaluated thunk (a Value with type = tThunk) which you want to evaluate, you'd set type = tBlackhole, then evaluate the expression the thunk points to, and throw an inf rec error if the evaluation tries to evaluate a tBlackhole (therefore trying to evaluate something while you're evaluating it already). Once it's evaluated, you set type = tAttrs or whatever the result is. This worked previously because a Value could either be unevaluated (tThunk, tApp, ...) or it could be evaluated (tAttrs, tInt, ...).

But with lazy attribute names, there's the new tLazyBinOp Value type, which is a value that can be partially evaluated, and that multiple times. E.g. if you have a = { x = 0; } // { y = 1; }, and you evaluate first a.y then a itself, you'll transform the Value multiple times:

  • Initially: type = tThunk
  • After a.y: type = tLazyBinOp; left.type = tThunk; right.type = tAttrs; by calling evalLazyBinOpAttrs
  • After a: type = tAttrs by calling evalLazyBinOp

With this, you can't just set type = tBlackhole the first time you evaluate a, because you may encountered a many more times after that, without having to encounter infinite recursion.

Here's some tricky examples of when inf rec should be triggered and when it shouldn't (currently the ones that should just give a stack overflow without position information):

{
  # Should throw inf rec
  a = let x = {} // x; in x.y;

  # Should not throw inf rec
  b = let x = x // { y = 0; }; in x.y;

  # Should throw inf rec
  c = let x = x // { y = 0; }; in x.z;

  # Should throw inf rec
  d = let x = { y = x.z; } // { z = x.y; }; in x.y;

  # Should not throw inf rec
  e = let x = ({ y = x.z; } // { z = x.y; }) // { y = 0; }; in x.z;

  # Should not throw inf rec, even with --strict
  f = let x = x.y // { y = {}; }; in x;
}

If it's possible to implement this well, it would probably involve tracking which sides of a tLazyBinOp are currently being evaluated. Note that a tLazyBinOp can have an arbitrary Value on either side, including another tLazyBinOp.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Nov 5, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-4/9862/1

This time it also works with lazy binops

The previous optimizations for prevention of allocation had to be undone

Code still needs cleanup, but it should be sound
@Infinisil Infinisil force-pushed the lazy-attr-names-v3 branch from cfd86dc to f99249d Dec 1, 2020
Infinisil added 2 commits Dec 2, 2020
With the previous commit, passing left/right is now unnecessary
@Infinisil Infinisil force-pushed the lazy-attr-names-v3 branch from 5d50971 to d1d9f1e Dec 4, 2020
Infinisil added 6 commits Dec 4, 2020
Every evaluation can now pass a handler, which is called once the
resulting value is either a tAttrs or a tLazyAttrs

There are two handlers:
- One for weak head normal form (changes tLazyAttrs into tAttrs)
- One for getting an attribute (lazily gets attributes from tLazyAttrs,
  strictly from tAttrs)
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 4, 2020

I made some good progress this week! I had to pretty much redesign this feature again, in order to deduplicate some function definitions i previously duplicated. I also fixed the infinite recursion detection, so that works again now. I think I have reached the final design of this, it's looking very promising now.

Unfortunately I'm now pretty sure that this does cause an evaluation slow-down of about 5% in the end. So I think it's best to make this an opt-in feature instead. So the next thing I'll work on for this PR is introducing a new primop, builtins.lazyUpdate (or builtins.lazyAttrsUpdate), which implements this lazy attribute name behavior. This is good enough for most purposes anyways.

While it would be possible to create a nix.conf option to opt-into the lazy attribute name behavior for //, this is probably a bad idea, since enabling it leads to expressions that require that option to be turned on to evaluate without error.

We could also consider making this the default again in the future in case the ~5% overhead can be removed.

@Infinisil Infinisil force-pushed the lazy-attr-names-v3 branch from ddfe9a1 to c3bbb4d Dec 5, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Jul 8, 2021

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-15/13975/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants