nix-env.cc: make --query evaluate .outPaths#1771
Closed
oxij wants to merge 3 commits intoNixOS:masterfrom
Closed
Conversation
3 tasks
oxij
force-pushed
the
nix-env-query-force-outpath
branch
from
2e634df to
4bcd132
Compare
Member
|
Well, that's possible, but due to the recursive nature, my previous tests suggested that forcing |
Member
|
Except perhaps that |
oxij
force-pushed
the
nix-env-query-force-outpath
branch
from
4bcd132 to
f18f523
Compare
Member
Author
|
Okay, so I hacked it a little bit more and simplified the first patch considerably.
Well, that's possible, but due to the recursive nature, my previous tests suggested that forcing `.out` leads to a significant slowdown of `nix-env -qa` (roughly five-times in my year-old test). On the other hand, it improves semantics – even closure would get checked during `-qa`.
On my tests the first patch gives 7x slowdown (unless `--fast` is given).
However, now this patchset has the second patch, with it and the new patch in `nixpkgs` PR the slowdown is less than 2x.
I have to note, that 7x slowdown produced a nice effect: I found a couple of new bugs in expressions in `nixpkgs` on `-darwin` from `-linux` (because I could evaluate their `.outPath`s even though I couldn't build them) so I'm now seriously considering adding an option to skip setting `meta.skipme` in the other PR to get that 7x slowdown back.
Except perhaps that `queryOutPath` seems overkill, as it queries the binary cache if I understand it correctly.
I`m looking at `src/libexpr/get-drvs.cc:78` and see nothing that seems to be related to binary caches.
It seems that `.outPath` is just simply too expensive to compute.
|
oxij
force-pushed
the
nix-env-query-force-outpath
branch
from
f18f523 to
04dbea9
Compare
oxij
force-pushed
the
nix-env-query-force-outpath
branch
from
04dbea9 to
85cdf2e
Compare
Member
Author
|
After some more hacking it's now gives 4% slowdown on my machine with 99% of the end result (a small number of broken packages get listed).
Pedantic result does require 7x slowdown.
|
oxij
force-pushed
the
nix-env-query-force-outpath
branch
from
85cdf2e to
dec8353
Compare
Member
Author
|
I think this and the sibling in `nixpkgs` are perfect now, with both
patches applied its now indistinguishable by performance (with default
options) and is much better w.r.t. semantics.
Will add more documentation to the manual if there's a consensus on
merging this.
|
Member
|
This behaviour ( |
Member
Author
|
This behaviour (`-qa` not evaluating `outPath`) is intentional, see edd145d.
I don't understand. Is there any other motivation except speed there?
If not, then merging this and the sibling PR (NixOS/nixpkgs#33057) for `nixpkgs` makes things strictly not worse, and merging both and then setting `config.checkMetaRecursively` makes things much better (at the cost of 2x slowdown).
NixOS/nixpkgs#33057 can't be merged without this because it will make `nix-env -qa` list broken packages and you didn't want to have that in #22277.
Not merging NixOS/nixpkgs#33057 breaks NixOS/nixpkgs#32424 (see NixOS/nixpkgs#33006) and many people want to have that feature since NixOS/nixpkgs#9306.
|
Member
|
Yes, the motivation is speed. |
oxij
added a commit
to oxij/nixpkgs
that referenced
this pull request
Jan 14, 2018
This is a temporary workaround to make `nix-env -qa` and `nix search` ignore broken packages as they they did before this patchset. This patch should be reverted after `nix` gets a proper fix for this. See NixOS/nix#1771.
oxij
force-pushed
the
nix-env-query-force-outpath
branch
from
dec8353 to
e7f25a7
Compare
NixOS/nixpkgs#33057 delays `stdenv.mkDerivation` derivation checks (e.g. meta, brokennes, etc) to `.drvPath` and `.outPath` evaluation leaving other fields accessible. That has a side-effect of making `nix-env -qa` list broken packages. This patch allows to - evaluate all `.outPath`s in `-qa` output strictly (`--strict` option) at the cost of 5x-7x slowdown; - disable filtering completely with `--all` option at the cost of getting some junk in the output and some speedup; - filter based on `.meta.available` (the default, `--approximate` option) at the cost of getting a tiny amounts of junk in the output and 0.96x-1.04x slowdown. It is also possible to get near-perfect results with `--approximate` at the cost of 2x slowdown by setting `config.checkMetaRecursively` in nixpkgs.
`-i` has the same problem as `-qa` in previous patch.
oxij
force-pushed
the
nix-env-query-force-outpath
branch
from
e7f25a7 to
b6c847e
Compare
Member
Author
|
Updated and rebased onto master. Tested to work with 2.0.4 too. |
Member
Author
|
I tested this to fix both #1963 (with a new one-line change) and NixOS/nixpkgs#37350 as well my original issue. Other Now my conscience is clear. |
Member
|
@edolstra (triage) If I read correctly, currently this PR is a 4% slowdown in exchange for fixing #1963 and NixOS/nixpkgs#37350. If so, this sounds like a net gain to me? @oxij Can you confirm this version is the 4% slowdown you mentioned in #1771 (comment) and not the 7x slowdown indeed? |
Member
Author
|
Yes, last time I checked it was 4% slowdown for `nix-env -qa`. This also adds some small undetermined amount of slowdown to `nix-env -i` in cases where it should fail, but the latter slowdown can be completely ignored, IMHO.
Those 4% come from evaluating `.available`. But `stdenv` also adds `assert`s to `.name` evaluation to work around the initial issue which doesn't work perfectly and breaks things like `.pos` handling. Moreover, I think that by applying this and then removing the `stdenv` `.name` hack we can get most, if not all, of those 4% back (not immediately, as `nixpkgs` will need to support both ways for a while, but eventually).
Since `.available` is now a fairly well-accepted part of nixpkgs I don't see any reason in not applying this.
SLNOS has this applied for like a year already.
|
Member
Author
|
The first patch here is no longer needed with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NixOS/nixpkgs#33057 delays
stdenv.mkDerivationderivation checks (e.g.meta, brokennes, etc) to
.drvPathand.outPathevaluation leavingother fields accessible. That has a side-effect of making
nix-env -qalist broken packages.
This patch forces those checks.