Skip to content

Commit

Permalink
Make path values lazy
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
roberth authored and tomberek committed Aug 24, 2024
1 parent 109b8e6 commit 9c11c58
Show file tree
Hide file tree
Showing 7 changed files with 58 additions and 13 deletions.
13 changes: 11 additions & 2 deletions src/libexpr/eval.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1978,6 +1978,7 @@ void EvalState::concatLists(Value & v, size_t nrLists, Value * const * lists, co
v.mkList(list);
}

// FIXME limit recursion
Value * resolveOutPath(EvalState & state, Value * v, const PosIdx pos)
{
state.forceValue(*v, pos);
Expand Down Expand Up @@ -2335,6 +2336,8 @@ BackedStringView EvalState::coerceToString(
v.payload.path.path
: copyToStore
? store->printStorePath(copyPathToStore(context, v.path()))
: v.path().accessor->toStringReturnsStorePath()
? store->printStorePath(copyPathToStore(context, SourcePath(v.path().accessor, CanonPath::root))) + v.path().path.absOrEmpty()
: std::string(v.path().path.abs());
}

Expand Down Expand Up @@ -2447,10 +2450,14 @@ SourcePath EvalState::coerceToPath(const PosIdx pos, Value & v, NixStringContext
if (v.type() == nPath)
return v.path();

/* Similarly, handle __toString where the result may be a path
/* Similarly, handle outPath and __toString where the result may be a path
value. */
if (v.type() == nAttrs) {
auto i = v.attrs()->find(sToString);
auto i = v.attrs()->find(sOutPath);
if (i != v.attrs()->end()) {
return coerceToPath(pos, *i->value, context, errorCtx);
}
i = v.attrs()->find(sToString);
if (i != v.attrs()->end()) {
Value v1;
callFunction(*i->value, v, v1, pos);
Expand Down Expand Up @@ -3115,6 +3122,8 @@ std::optional<SourcePath> EvalState::resolveLookupPathPath(const LookupPath::Pat
store,
fetchSettings,
EvalSettings::resolvePseudoUrl(value));
// Traditional search path lookups use the absolute path space for
// historical consistency.
auto storePath = fetchToStore(*store, SourcePath(accessor), FetchMode::Copy);
res.emplace(rootPath(CanonPath(store->toRealPath(storePath))));
} catch (Error & e) {
Expand Down
5 changes: 4 additions & 1 deletion src/libexpr/nixexpr.hh
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,10 @@ struct ExprPath : Expr
ExprPath(SourcePath && path)
: path(path)
{
v.mkPath(&*path.accessor, strdup(path.path.abs().c_str()));
v.mkPath(
&*path.accessor,
// TODO: GC_STRDUP
strdup(path.path.abs().c_str()));
}
Value * maybeThunk(EvalState & state, Env & env) override;
COMMON_METHODS
Expand Down
33 changes: 23 additions & 10 deletions src/libexpr/primops/fetchTree.cc
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ struct FetchTreeParams {
bool emptyRevFallback = false;
bool allowNameArgument = false;
bool isFetchGit = false;
bool returnPath = true; // whether to return a SourcePath or a StorePath
bool returnPath = true; // whether to return a SourcePath instead of a StorePath
};

static void fetchTree(
Expand Down Expand Up @@ -212,18 +212,31 @@ static void fetchTree(

state.checkURI(input.toURLString());

auto [storePath, input2] = input.fetchToStore(state.store);
if (params.returnPath) {
// Clang16+: change to `auto [accessor, input2] =`
auto pair = input.getAccessor(state.store);
auto & accessor = pair.first;
auto & input2 = pair.second;

emitTreeAttrs(state, input2, v,
[&](Value & vOutPath) {
state.registerAccessor(accessor);
vOutPath.mkPath(SourcePath { accessor, CanonPath::root });
},
params.emptyRevFallback, false);
} else {
auto pair = input.fetchToStore(state.store);
auto & storePath = pair.first;
auto & input2 = pair.second;

state.allowPath(storePath);
state.allowPath(storePath);

emitTreeAttrs(state, input2, v,
[&](Value & vOutPath) {
if (params.returnPath)
vOutPath.mkPath(state.rootPath(state.store->toRealPath(storePath)));
else
emitTreeAttrs(state, input2, v,
[&](Value & vOutPath) {
state.mkStorePathString(storePath, vOutPath);
},
params.emptyRevFallback, false);
},
params.emptyRevFallback, false);
}
}

static void prim_fetchTree(EvalState & state, const PosIdx pos, Value * * args, Value & v)
Expand Down
5 changes: 5 additions & 0 deletions src/libutil/posix-source-accessor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -195,4 +195,9 @@ ref<SourceAccessor> makeFSSourceAccessor(std::filesystem::path root)
{
return make_ref<PosixSourceAccessor>(std::move(root));
}

bool PosixSourceAccessor::toStringReturnsStorePath() const {
return false;
}

}
2 changes: 2 additions & 0 deletions src/libutil/posix-source-accessor.hh
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ struct PosixSourceAccessor : virtual SourceAccessor
*/
static SourcePath createAtRoot(const std::filesystem::path & path);

virtual bool toStringReturnsStorePath() const override;

private:

/**
Expand Down
4 changes: 4 additions & 0 deletions src/libutil/source-accessor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,8 @@ CanonPath SourceAccessor::resolveSymlinks(
return res;
}

bool SourceAccessor::toStringReturnsStorePath() const {
return true;
}

}
9 changes: 9 additions & 0 deletions src/libutil/source-accessor.hh
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,15 @@ struct SourceAccessor : std::enable_shared_from_this<SourceAccessor>

virtual std::string showPath(const CanonPath & path);

/**
* System paths: `toString /foo/bar = "/foo/bar"`
* Virtual paths: fetched to the store
*
* In both cases, the returned string functionally identifies the path,
* and can still be read.
*/
virtual bool toStringReturnsStorePath() const;

/**
* Resolve any symlinks in `path` according to the given
* resolution mode.
Expand Down

0 comments on commit 9c11c58

Please sign in to comment.