stdenv: Move all stdenv lookups out of mkDerivation #431756
Conversation
All lookups and expressions that don't depend on derivation attrs are moved to the outer let block. Saves over 10% lookups for eval on x86_64-linux, along with 53MiB memory (over 1%). For some reason lists concats go 14% up, but defaultConfigurePlatformsFlags optimisation removes ~7% of them. Overall time is down ~3%.
|
I still have this on my list to review, but I won't be able to be at a computer until a little later this week. These sorts of PRs need careful attention…Although I have a strong belief that you have not introduced any regressions with this change. |
| extraNativeBuildInputs | ||
| extraBuildInputs | ||
| extraSandboxProfile | ||
| __extraImpureHostDeps |
There was a problem hiding this comment.
__extraImpureHostDeps dates to fa9c81f694680. In the Darwin stdenv, it's set to commonImpureHostDeps: 49a5791bc4f20. The Darwin stdenv appears to be the only user.
| # Target is not included by default because most programs don't care. | ||
| # Including it then would cause needless mass rebuilds. | ||
| # | ||
| # TODO(@Ericson2314): Make [ "build" "host" ] always the default / resolve #87909 |
There was a problem hiding this comment.
Currently a draft; needs substantial work to integrate into the current world.
| defaultConfigurePlatforms = | ||
| optionals (hostPlatform != buildPlatform || config.configurePlatformsByDefault) | ||
| [ | ||
| "build" | ||
| "host" | ||
| ]; |
There was a problem hiding this comment.
As I read it, defaultConfigurePlatforms is either [] or it is [ "build" "host" ].
| # | ||
| # TODO(@Ericson2314): Make [ "build" "host" ] always the default / resolve #87909 | ||
| defaultConfigurePlatforms = | ||
| optionals (hostPlatform != buildPlatform || config.configurePlatformsByDefault) |
There was a problem hiding this comment.
Let's extract hostPlatform != buildPlatform || config.configurePlatformsByDefault to useDefaultConfigurePlatforms.
| defaultConfigurePlatformsFlags = | ||
| optional (elem "build" defaultConfigurePlatforms) buildPlatformConfigureFlag | ||
| ++ optional (elem "host" defaultConfigurePlatforms) hostPlatformConfigureFlag | ||
| ++ optional (elem "target" defaultConfigurePlatforms) targetPlatformConfigureFlag; |
There was a problem hiding this comment.
I don't see a way for the target condition to be true, since (as I read it) defaultConfigurePlatforms will only contain "host" or "build" but never "target".
I think this simplifies to:
| defaultConfigurePlatformsFlags = | |
| optional (elem "build" defaultConfigurePlatforms) buildPlatformConfigureFlag | |
| ++ optional (elem "host" defaultConfigurePlatforms) hostPlatformConfigureFlag | |
| ++ optional (elem "target" defaultConfigurePlatforms) targetPlatformConfigureFlag; | |
| defaultConfigurePlatformsFlags = if useDefaultConfigurePlatforms then [ buildPlatformConfigureFlag hostPlatformConfigureFlag ] else [ ]; |
The code is copied and modified from a context where configurePlatforms might include "target" but in this context it's quite a bit simpler.
| @@ -521,9 +577,9 @@ let | |||
| # indeed more finely than Nix knows or cares about. The `system` | |||
| # attribute of `buildPlatform` matches Nix's degree of specificity. | |||
| # exactly. | |||
| inherit (stdenv.buildPlatform) system; | |||
| inherit system; | |||
There was a problem hiding this comment.
Prefer to revert this change.
|
|
||
| userHook = config.stdenv.userHook or null; | ||
| inherit userHook; |
There was a problem hiding this comment.
As per previous comment, I support removing this functionality.
| @@ -544,9 +600,14 @@ let | |||
| # This parameter is sometimes a string, sometimes null, and sometimes a list, yuck | |||
There was a problem hiding this comment.
This isn't true anymore, based on the usage of ++ below. That would cause an eval failure.
| # TODO(@oxij, @Ericson2314): This is here to keep the old semantics, remove when | ||
| # no package has `doCheck = true`. | ||
| doCheck' = doCheck && stdenv.buildPlatform.canExecute stdenv.hostPlatform; | ||
| doInstallCheck' = doInstallCheck && stdenv.buildPlatform.canExecute stdenv.hostPlatform; | ||
| doCheck' = doCheck && canExecuteHostOnBuild; | ||
| doInstallCheck' = doInstallCheck && canExecuteHostOnBuild; |
There was a problem hiding this comment.
Dates to August 2018:
| ++ optional (elem "build" configurePlatforms) "--build=${stdenv.buildPlatform.config}" | ||
| ++ optional (elem "host" configurePlatforms) "--host=${stdenv.hostPlatform.config}" | ||
| ++ optional (elem "target" configurePlatforms) "--target=${stdenv.targetPlatform.config}"; | ||
| ++ ( |
There was a problem hiding this comment.
We can avoid this list concatenation in the common case by testing the following booleans.
attrs ? configureFlags: the derivation specifiedconfigureFlagsattrs ? configurePlatforms: the derivation specifiedconfigurePlatforms
The common case is !attrs ? configureFlags and !attrs ? configurePlatforms, which ought to just be defaultConfigurePlatformsFlags.
Here's the truth table that I'm thinking of.
attrs ? configureFlags |
attrs ? configurePlatforms |
Meaning | Result |
|---|---|---|---|
false |
false |
Defaults in every case | defaultConfigurePlatformsFlags |
false |
true |
Non-default platforms, need to test for target | Do elem "target" logic on the platforms |
true |
false |
Some flags are set, default platforms (other common case) | configureFlags ++ defaultConfigurePlatformsFlags |
true |
true |
Rare case | configureFlags plus the elem target logic. |
What do you think?
All lookups and expressions that don't depend on derivation attrs are moved to the outer let block. Saves over 10% lookups for eval on x86_64-linux, along with 53MiB memory (over 1%).
For some reason lists concats go 14% up, but defaultConfigurePlatformsFlags optimisation removes ~7% of them.
Overall time is down ~3%.
Things done
Add a 👍 reaction to pull requests you find important.