libexpr: save some performance in builtins.derivation

[llakala]

Saves two lookups and an environment definition on every single call to builtins.derivation.

Building firefox from nixpkgs, here's the stats diff for the first commit (comparing the output of NIX_SHOW_STATS, null = unchanged):

{
  "attrset": {
    "lookups": null,
    "merges": null,
    "mergeCopies": null
  },
  "list": {
    "concats": null
  },
  "parser": {
    "expressions": "-0%"
  },
  "memory": {
    "envs": "-0.31%",
    "list": null,
    "sets": null,
    "symbols": "-0%",
    "values": null,
    "total": "-0.03%"
  },
  "speed": {
    "primops": null,
    "functionCalls": null,
    "thunksMade": null,
    "thunksAvoided": "-0.61%"
  }
}

And for the second commit:

{
  "attrset": {
    "lookups": "-2.95%",
    "merges": null,
    "mergeCopies": null
  },
  "list": {
    "concats": null
  },
  "parser": {
    "expressions": "+0%"
  },
  "memory": {
    "envs": "+0%",
    "list": null,
    "sets": null,
    "symbols": null,
    "values": "+0%",
    "total": "+0%"
  },
  "speed": {
    "primops": null,
    "functionCalls": null,
    "thunksMade": "+0%",
    "thunksAvoided": "+0%"
  }
}

Likely minor improvement in real-time, but since it's such a critical function, we should squeeze every bit of performance out of it that we can.

Motivation

Context

---

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[llakala]

Added a few more commits, notably one that special-cases when the only output is out. If so, we can avoid an environment definition, a merge, and calls to map, head, and listToAttrs. Running hyperfine, this commit takes the real-time eval of firefox from:

  Time (mean ± σ):      1.554 s ±  0.045 s    [User: 0.965 s, System: 0.297 s]
  Range (min … max):    1.499 s …  1.656 s    25 runs

To:

  Time (mean ± σ):      1.538 s ±  0.030 s    [User: 0.952 s, System: 0.300 s]
  Range (min … max):    1.499 s …  1.615 s    25 runs

And gives us a meaningful stats diff.

{
  "attrset": {
    "lookups": "-1.11%",
    "merges": "-7.6%",
    "mergeCopies": "-1.02%"
  },
  "list": {
    "concats": null
  },
  "parser": {
    "expressions": "+0.01%"
  },
  "memory": {
    "envs": "-0.68%",
    "list": "-1.73%",
    "sets": "-1.23%",
    "symbols": null,
    "values": "-0.66%",
    "total": "-1.04%"
  },
  "speed": {
    "primops": "-4.96%",
    "functionCalls": "-0.77%",
    "thunksMade": "-1.07%",
    "thunksAvoided": "-1.35%"
  }
}

[roberth]

I assume test coverage is inadequate when it comes to corner cases here.

1, 2 and 4 are trivial to prove equivalent.

560bade could be proven perhaps?
fb3eca9 has rather questionable maintainability. Not that we're going to change it a lot.

Also worth noting, nixpkgs has its own variation of this output managing logic that it applies on its own accord, on top.
Worth looking into replacing with derivationStrict, e.g.

• Single package fixpoint step 2: introduce mkPackage nixpkgs#296769

or probably better

• Single package fixpoint step 2: add mkPackage nixpkgs#509008

With that in mind, I wouldn't go overboard on tech debt here, because this file can and probably should be bypassed altogether.

If you ask @Ericson2314 I guess he would suggest to only return the drvPath and let userland do outputOf, but that's for another day ;)

[llakala]

Yes, I'm planning to change mkDerivation to use derivationStrict directly in nixpkgs. Right now it uses extendDerivation which redoes a lot of the work done by builtins.derivation. But I still thought it was worth it to send some love back upstream.