Lazy `fetchTree` `outPath` path values #10252

roberth · 2024-03-15T19:02:11Z

Motivation

Improve performance, and make the fetchTree interface more capable while keeping it clean.

Description

This makes fetchTree return lazy InputAccessor-based SourcePaths instead of "cowardly" fetching them to the store and returning absolute "system" paths.
It stays close to existing path semantics, including support for readFile "${toString p}/..", which some expressions rely on.
It does not go as far as lazy-trees, but judging from the amount of change I could reuse, and how little of my own I had to add, lazy-trees will be a natural extension of this PR.

Done:

Packages and NixOS evaluate as usual
- no hash changes, based on my limited testing
Flake is not added to store unless e.g.
- "${flake.outPath}"
- uses module system (needs clever lazy source strings, or a change to the module system)
Note that the above is already an improvement over the status quo - always fetching to the store
iirc 0.1s reduction on nixpkgs#hello, and 1s reduction on nixosTests.simple

Conclusion so far:
Viable

TODO:

Check the TODO and FIXMEs
Update the test suite
- probably some intended behavior change, such as removal of narHash in some observable places
- possibly finds a bug
Check performance again
Cherry-pick the toString path behavior to lazy-trees

Context

Priorities and Process

Add 👍 to pull requests you find important.

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

roberth · 2024-03-15T19:03:00Z

src/libcmd/common-eval-args.cc

These changes are close to lazy-trees.

roberth · 2024-03-15T19:04:33Z

src/libexpr/eval.cc

+void EvalState::registerAccessor(const ref<InputAccessor> accessor)
+{
+    inputAccessors.push_back(accessor);
+}


This is like lazy-trees, except we only do it so that we don't destroy them when we put a non-smart pointer in Value, which has no finalizer because of GC and performance.

roberth · 2024-03-15T19:05:13Z

src/libexpr/eval.cc

@@ -1973,6 +1978,17 @@ void EvalState::concatLists(Value & v, size_t nrLists, Value * * lists, const Po
    }
 }

+// FIXME limit recursion


Known issue, solve later with #10240

roberth · 2024-03-15T19:07:23Z

src/libexpr/eval.cc

+        Value & vTmp0 = *vTmpP++;
+        i->eval(state, env, vTmp0);
+        Value & vTmp = *resolveOutPath(state, &vTmp0, i_pos);

        /* If the first element is a path, then the result will also
           be a path, we don't copy anything (yet - that's done later,
           since paths are copied when they are used in a derivation),
           and none of the strings are allowed to have contexts. */
        if (first) {
            firstType = vTmp.type();
+            if (firstType == nPath) {
+                accessor = vTmp.path().accessor;
+            }


Similar but not equal to lazy-trees.

I've kept the diff smaller by keeping vTmp as a reference

Not adding the first name to the path, because it is added again later. Probably a bug in lazy-trees.

roberth · 2024-03-15T19:09:22Z

src/libexpr/eval.cc

+            : v.path().accessor->toStringReturnsStorePath()
+            ? store->printStorePath(copyPathToStore(context, SourcePath(v.path().accessor, CanonPath::root))) + v.path().path.absOrEmpty()


This is new, in order for readFile "${toString ./.}/.." to work, just as it did before.
Would not recommend to write that, but similar usages of paths exist in the wild.

roberth · 2024-03-15T19:10:58Z

src/libexpr/eval.cc

+        auto i = v.attrs->find(sOutPath);
+        if (i != v.attrs->end()) {
+            return coerceToPath(pos, *i->value, context, errorCtx);
+        }


This rule is not new. Previously, it would have worked by falling through to the coerceToString + rootPath code down below.

roberth · 2024-03-15T19:11:36Z

src/libexpr/nixexpr.hh

+        v.mkPath(
+            &*path.accessor,
+            // TODO: GC_STRDUP
+            strdup(path.path.abs().c_str()));


Like lazy-trees, but I had to add strdup to avoid corruption.

roberth · 2024-03-15T19:12:45Z

src/libexpr/primops/fetchTree.cc

@@ -201,18 +201,31 @@ static void fetchTree(

    state.checkURI(input.toURLString());

-    auto [storePath, input2] = input.fetchToStore(state.store);
+    if (params.returnPath) {


Like #10225, but clang was lagging behind GCC's C++20.

roberth · 2024-03-15T19:14:20Z

src/libutil/source-accessor.hh

+     * In both cases, the returned string functionally identifies the path,
+     * and can still be read.
+     */
+    virtual bool toStringReturnsStorePath() const;


The only truly new semantics.

Either way, both kinds of paths are virtual in the sense that they haven't been copied yet.
If you have copied it, it'd be a string. (Not if and only if, although Nix does discourage that.)

In lazy-trees, this could be changed to a virtual store path string without a problem.

TODO: forward port this rule to prove it.

#10511 (comment) I think rather contain information with which to construct the path, if I understand what is going on here correctly.

@Ericson2314 these are solutions to different problems.

Here I am adding a property to distinguish the behavior between "system" path values and virtual path values in the language.

Linked comment seems to be about optimizing away a copy operation, which iiuc is internal to a fetcher and not perceptible by users.

Both solutions (to different problems!) are trying to solve concerns in the upper layers though.

I'll check if this one can be moved into the evaluator. We could probably just special case the system (ie posix accessor) paths there.

Btw, here I've picked the least breaking semantics:

The return value still uniquely identifies the source

Still deterministic

Still readable when converted to a system path with /. + x or /${x}

Performance is at least as good as the status quo, but not as good as lazy trees, because for toString x to be instant, you need to sacrifice one of the above, or come up with a clever scheme, like opaque placeholders in strings or something.

ConnorBaker · 2024-04-04T18:55:11Z

@roberth is there anything I (or someone in general) could do to try to help move this forward? Thank you for working on this :)

This picks a number of changes from the lazy-trees branch. As it is hand picked, and does not include some other necessary changes, it does not build. Subsequent commits will fix that. I have added a couple of comments of my own as well. Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

This fixes the double copy problem and improves performance for expressions that don't force the whole source to be added to the store. Rules for fast expressions: - Use path literals where possible - import ./foo.nix - Use + operator with slash in string - src = fetchTree foo + "/src"; - Use source filtering, lib.fileset - AVOID toString - If possible, AVOID interpolations ("${./.}") - If possible, move slashes into the interpolation to add less to the store - "${./src}/foo" -> "${./src/foo}" toString may be improved later as part of lazy-trees, so these recommendations are a snapshot. Path values are quite nice though.

This allows clever editors/IDEs to discern the path more easily for Ctrl+Click navigate to functionality, e.g. when building .?ref=HEAD

This showPath is getting a little too ad hoc, but it works for now.

roberth · 2024-04-16T16:21:29Z

@roberth is there anything I (or someone in general) could do to try to help move this forward? Thank you for working on this :)

Hi @ConnorBaker,

Sorry for the late response; I had to take another look at this first, and it took a while to get around to it.
I've rebased the branch, but I seem to have broken the search path somehow.

I see two ways forward, either

re-do the PR with reduced scope
- don't remove the narHash, if possible; should keep more flakes code the same
- carefully look whether other changes were absolutely necessary
keep going; just fix the remaining test failures, and also remove the commit delay the flake outPath semantics change for now

nixos-discourse · 2024-05-07T13:06:04Z

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

https://discourse.nixos.org/t/2024-04-29-nix-team-meeting-minutes-142/45020/1

github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world labels Mar 15, 2024

roberth commented Mar 15, 2024

View reviewed changes

thufschmitt added this to the fetch-tree stabilisation milestone Mar 19, 2024

roberth mentioned this pull request Mar 22, 2024

nix flake {update,metadata,...}: shows input's narHash #10249

Open

tomberek added idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. and removed idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. labels Apr 8, 2024

roberth mentioned this pull request Apr 14, 2024

Add outputHashMode argument attribute to bulitins that could support it #10480

Open

4 tasks

roberth force-pushed the lazy-fetchTree branch 2 times, most recently from a29ade5 to 8cb0fd4 Compare April 16, 2024 13:54

edolstra and others added 8 commits April 16, 2024 15:55

fetchTree: Return a path instead of a store path

202b188

SourceAccessor: insert colon after prefix

9707d2f

This allows clever editors/IDEs to discern the path more easily for Ctrl+Click navigate to functionality, e.g. when building .?ref=HEAD

Fix evalState::rootFS paths' to_string()

bde36d8

This showPath is getting a little too ad hoc, but it works for now.

Fix nix flake init eval for path value

6f01e4b

WIP please revert

446d59b

WIP: delay the flake outPath semantics change for now

4332b9a

roberth force-pushed the lazy-fetchTree branch from 8cb0fd4 to 4332b9a Compare April 16, 2024 13:58

roberth mentioned this pull request May 13, 2024

Virtual path semantics that are deterministic, lazy and suitable for performing deduplication of values by a path value key. #10689

Open

roberth mentioned this pull request May 20, 2024

Path coercion in Flakes causes files to be added to the store twice #9428

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy `fetchTree` `outPath` path values #10252

Lazy `fetchTree` `outPath` path values #10252

roberth commented Mar 15, 2024 •

edited

roberth Mar 15, 2024

roberth Mar 15, 2024

roberth Mar 15, 2024

roberth Mar 15, 2024

roberth Mar 15, 2024

roberth Mar 15, 2024

roberth Mar 15, 2024

roberth Mar 15, 2024

roberth Mar 15, 2024

roberth Mar 15, 2024

Ericson2314 Apr 15, 2024 •

edited

roberth Apr 16, 2024

roberth Apr 16, 2024

ConnorBaker commented Apr 4, 2024

roberth commented Apr 16, 2024

nixos-discourse commented May 7, 2024

		: v.path().accessor->toStringReturnsStorePath()
		? store->printStorePath(copyPathToStore(context, SourcePath(v.path().accessor, CanonPath::root))) + v.path().path.absOrEmpty()

Lazy fetchTree outPath path values #10252

Are you sure you want to change the base?

Lazy fetchTree outPath path values #10252

Conversation

roberth commented Mar 15, 2024 • edited

Motivation

Description

Context

Priorities and Process

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Apr 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConnorBaker commented Apr 4, 2024

roberth commented Apr 16, 2024

nixos-discourse commented May 7, 2024

Lazy `fetchTree` `outPath` path values #10252

Lazy `fetchTree` `outPath` path values #10252

roberth commented Mar 15, 2024 •

edited

Ericson2314 Apr 15, 2024 •

edited