From 6b279cd10ea82813decc4451fd3842b49ee9deea Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Mon, 4 Mar 2024 07:37:45 +0100 Subject: [PATCH] Merge pull request #9658 from pennae/env-diet reduce the size of Env by one pointer (cherry picked from commit 83f5622545a2fc31eb7e7d5105f64ed6dd3058b3) Change-Id: I5636290526d0165cfc61aee1e7a5b94db4a26cef --- doc/manual/rl-next/env-size-reduction.md | 7 +++++ doc/manual/rl-next/with-error-reporting.md | 31 ++++++++++++++++++++++ src/libcmd/repl.cc | 2 +- src/libexpr/eval-inline.hh | 2 -- src/libexpr/eval.cc | 29 ++++++++++---------- src/libexpr/eval.hh | 5 ---- src/libexpr/nixexpr.cc | 18 ++++++++----- src/libexpr/nixexpr.hh | 13 ++++++--- src/libexpr/primops.cc | 2 +- 9 files changed, 75 insertions(+), 34 deletions(-) create mode 100644 doc/manual/rl-next/env-size-reduction.md create mode 100644 doc/manual/rl-next/with-error-reporting.md diff --git a/doc/manual/rl-next/env-size-reduction.md b/doc/manual/rl-next/env-size-reduction.md new file mode 100644 index 000000000..40a58bc28 --- /dev/null +++ b/doc/manual/rl-next/env-size-reduction.md @@ -0,0 +1,7 @@ +--- +synopsis: Reduce eval memory usage and wall time +prs: 9658 +--- + +Reduce the size of the `Env` struct used in the evaluator by a pointer, or 8 bytes on most modern machines. +This reduces memory usage during eval by around 2% and wall time by around 3%. diff --git a/doc/manual/rl-next/with-error-reporting.md b/doc/manual/rl-next/with-error-reporting.md new file mode 100644 index 000000000..10b020956 --- /dev/null +++ b/doc/manual/rl-next/with-error-reporting.md @@ -0,0 +1,31 @@ +--- +synopsis: Better error reporting for `with` expressions +prs: 9658 +--- + +`with` expressions using non-attrset values to resolve variables are now reported with proper positions. + +Previously an incorrect `with` expression would report no position at all, making it hard to determine where the error originated: + +``` +nix-repl> with 1; a +error: + … + + at «none»:0: (source not available) + + error: value is an integer while a set was expected +``` + +Now position information is preserved and reported as with most other errors: + +``` +nix-repl> with 1; a +error: + … while evaluating the first subexpression of a with expression + at «string»:1:1: + 1| with 1; a + | ^ + + error: value is an integer while a set was expected +``` diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index e6f0faa06..d25ad582e 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -109,7 +109,7 @@ NixRepl::NixRepl(const SearchPath & searchPath, nix::ref store, refstaticBaseEnv.get())) + , staticEnv(new StaticEnv(nullptr, state->staticBaseEnv.get())) , historyFile(getDataDir() + "/nix/repl-history") { } diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index 52aa75b5f..f7710f819 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -73,8 +73,6 @@ Env & EvalState::allocEnv(size_t size) #endif env = (Env *) allocBytes(sizeof(Env) + size * sizeof(Value *)); - env->type = Env::Plain; - /* We assume that env->values has been cleared by the allocator; maybeThunk() and lookupVar fromWith expect this. */ return *env; diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index deb5ea2b3..0fa686701 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -519,7 +519,7 @@ EvalState::EvalState( , env1AllocCache(std::allocate_shared(traceable_allocator(), nullptr)) #endif , baseEnv(allocEnv(128)) - , staticBaseEnv{std::make_shared(false, nullptr)} + , staticBaseEnv{std::make_shared(nullptr, nullptr)} { countCalls = getEnv("NIX_COUNT_CALLS").value_or("0") != "0"; @@ -788,7 +788,7 @@ void printStaticEnvBindings(const SymbolTable & st, const StaticEnv & se) // just for the current level of Env, not the whole chain. void printWithBindings(const SymbolTable & st, const Env & env) { - if (env.type == Env::HasWithAttrs) { + if (!env.values[0]->isThunk()) { std::cout << "with: "; std::cout << ANSI_MAGENTA; Bindings::iterator j = env.values[0]->attrs->begin(); @@ -842,7 +842,7 @@ void mapStaticEnvBindings(const SymbolTable & st, const StaticEnv & se, const En if (env.up && se.up) { mapStaticEnvBindings(st, *se.up, *env.up, vm); - if (env.type == Env::HasWithAttrs) { + if (!env.values[0]->isThunk()) { // add 'with' bindings. Bindings::iterator j = env.values[0]->attrs->begin(); while (j != env.values[0]->attrs->end()) { @@ -980,22 +980,23 @@ inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval) if (!var.fromWith) return env->values[var.displ]; + // This early exit defeats the `maybeThunk` optimization for variables from `with`, + // The added complexity of handling this appears to be similarly in cost, or + // the cases where applicable were insignificant in the first place. + if (noEval) return nullptr; + + auto * fromWith = var.fromWith; while (1) { - if (env->type == Env::HasWithExpr) { - if (noEval) return 0; - Value * v = allocValue(); - evalAttrs(*env->up, (Expr *) env->values[0], *v, noPos, ""); - env->values[0] = v; - env->type = Env::HasWithAttrs; - } + forceAttrs(*env->values[0], fromWith->pos, "while evaluating the first subexpression of a with expression"); Bindings::iterator j = env->values[0]->attrs->find(var.name); if (j != env->values[0]->attrs->end()) { if (countCalls) attrSelects[j->pos]++; return j->value; } - if (!env->prevWith) + if (!fromWith->parentWith) error("undefined variable '%1%'", symbols[var.name]).atPos(var.pos).withFrame(*env, var).debugThrow(); - for (size_t l = env->prevWith; l; --l, env = env->up) ; + for (size_t l = fromWith->prevWith; l; --l, env = env->up) ; + fromWith = fromWith->parentWith; } } @@ -1854,9 +1855,7 @@ void ExprWith::eval(EvalState & state, Env & env, Value & v) { Env & env2(state.allocEnv(1)); env2.up = &env; - env2.prevWith = prevWith; - env2.type = Env::HasWithExpr; - env2.values[0] = (Value *) attrs; + env2.values[0] = attrs->maybeThunk(state, env); body->eval(state, env2, v); } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 4cbd45013..6adf6d066 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -115,11 +115,6 @@ struct Constant struct Env { Env * up; - /** - * Number of of levels up to next `with` environment - */ - unsigned short prevWith:14; - enum { Plain = 0, HasWithExpr, HasWithAttrs } type:2; Value * values[0]; }; diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 76ce62f6c..edab297f2 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -333,6 +333,8 @@ void ExprVar::bindVars(EvalState & es, const std::shared_ptr & if (es.debugRepl) es.exprEnvs.insert(std::make_pair(this, env)); + fromWith = nullptr; + /* Check whether the variable appears in the environment. If so, set its level and displacement. */ const StaticEnv * curEnv; @@ -344,7 +346,6 @@ void ExprVar::bindVars(EvalState & es, const std::shared_ptr & } else { auto i = curEnv->find(name); if (i != curEnv->vars.end()) { - fromWith = false; this->level = level; displ = i->second; return; @@ -360,7 +361,8 @@ void ExprVar::bindVars(EvalState & es, const std::shared_ptr & .msg = hintfmt("undefined variable '%1%'", es.symbols[name]), .errPos = es.positions[pos] }); - fromWith = true; + for (auto * e = env.get(); e && !fromWith; e = e->up) + fromWith = e->isWith; this->level = withLevel; } @@ -393,7 +395,7 @@ void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr es.exprEnvs.insert(std::make_pair(this, env)); if (recursive) { - auto newEnv = std::make_shared(false, env.get(), recursive ? attrs.size() : 0); + auto newEnv = std::make_shared(nullptr, env.get(), recursive ? attrs.size() : 0); Displacement displ = 0; for (auto & i : attrs) @@ -435,7 +437,7 @@ void ExprLambda::bindVars(EvalState & es, const std::shared_ptr es.exprEnvs.insert(std::make_pair(this, env)); auto newEnv = std::make_shared( - false, env.get(), + nullptr, env.get(), (hasFormals() ? formals->formals.size() : 0) + (!arg ? 0 : 1)); @@ -471,7 +473,7 @@ void ExprLet::bindVars(EvalState & es, const std::shared_ptr & if (es.debugRepl) es.exprEnvs.insert(std::make_pair(this, env)); - auto newEnv = std::make_shared(false, env.get(), attrs->attrs.size()); + auto newEnv = std::make_shared(nullptr, env.get(), attrs->attrs.size()); Displacement displ = 0; for (auto & i : attrs->attrs) @@ -490,6 +492,10 @@ void ExprWith::bindVars(EvalState & es, const std::shared_ptr & if (es.debugRepl) es.exprEnvs.insert(std::make_pair(this, env)); + parentWith = nullptr; + for (auto * e = env.get(); e && !parentWith; e = e->up) + parentWith = e->isWith; + /* Does this `with' have an enclosing `with'? If so, record its level so that `lookupVar' can look up variables in the previous `with' if this one doesn't contain the desired attribute. */ @@ -506,7 +512,7 @@ void ExprWith::bindVars(EvalState & es, const std::shared_ptr & es.exprEnvs.insert(std::make_pair(this, env)); attrs->bindVars(es, env); - auto newEnv = std::make_shared(true, env.get()); + auto newEnv = std::make_shared(this, env.get()); body->bindVars(es, newEnv); } diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index cb952f5bb..2b5b72bc0 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -139,6 +139,7 @@ std::ostream & operator << (std::ostream & str, const Pos & pos); struct Env; struct Value; class EvalState; +struct ExprWith; struct StaticEnv; @@ -221,8 +222,11 @@ struct ExprVar : Expr Symbol name; /* Whether the variable comes from an environment (e.g. a rec, let - or function argument) or from a "with". */ - bool fromWith; + or function argument) or from a "with". + + `nullptr`: Not from a `with`. + Valid pointer: the nearest, innermost `with` expression to query first. */ + ExprWith * fromWith; /* In the former case, the value is obtained by going `level` levels up from the current environment and getting the @@ -380,6 +384,7 @@ struct ExprWith : Expr PosIdx pos; Expr * attrs, * body; size_t prevWith; + ExprWith * parentWith; ExprWith(const PosIdx & pos, Expr * attrs, Expr * body) : pos(pos), attrs(attrs), body(body) { }; PosIdx getPos() const override { return pos; } COMMON_METHODS @@ -473,14 +478,14 @@ extern ExprBlackHole eBlackHole; runtime. */ struct StaticEnv { - bool isWith; + ExprWith * isWith; const StaticEnv * up; // Note: these must be in sorted order. typedef std::vector> Vars; Vars vars; - StaticEnv(bool isWith, const StaticEnv * up, size_t expectedSize = 0) : isWith(isWith), up(up) { + StaticEnv(ExprWith * isWith, const StaticEnv * up, size_t expectedSize = 0) : isWith(isWith), up(up) { vars.reserve(expectedSize); }; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 6c8a1f5a8..e107db228 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -233,7 +233,7 @@ static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * v Env * env = &state.allocEnv(vScope->attrs->size()); env->up = &state.baseEnv; - auto staticEnv = std::make_shared(false, state.staticBaseEnv.get(), vScope->attrs->size()); + auto staticEnv = std::make_shared(nullptr, state.staticBaseEnv.get(), vScope->attrs->size()); unsigned int displ = 0; for (auto & attr : *vScope->attrs) {