evaluate inherit (from) exprs only once per directive

desugaring inherit-from to syntactic duplication of the source expr also
duplicates side effects of the source expr (such as trace calls) and
expensive computations (such as derivationStrict).

(cherry picked from commit cefd0302b55b3360dbca59cfcb4bf6a750d6cdcf)
Change-Id: Iff519f991adef2e51683ba2c552d37a3df7a179e
This commit is contained in:
eldritch horrors 2024-03-08 09:52:15 +01:00
parent 71e0114708
commit b667b4cded
9 changed files with 109 additions and 15 deletions

View file

@ -0,0 +1,7 @@
---
synopsis: "`inherit (x) ...` evaluates `x` only once"
prs: 9847
---
`inherit (x) a b ...` now evaluates the expression `x` only once for all inherited attributes rather than once for each inherited attribute.
This does not usually have a measurable impact, but side-effects (such as `builtins.trace`) would be duplicated and expensive expressions (such as derivations) could cause a measurable slowdown.

View file

@ -1217,6 +1217,18 @@ void ExprPath::eval(EvalState & state, Env & env, Value & v)
} }
Env * ExprAttrs::buildInheritFromEnv(EvalState & state, Env & up)
{
Env & inheritEnv = state.allocEnv(inheritFromExprs->size());
inheritEnv.up = &up;
Displacement displ = 0;
for (auto from : *inheritFromExprs)
inheritEnv.values[displ++] = from->maybeThunk(state, up);
return &inheritEnv;
}
void ExprAttrs::eval(EvalState & state, Env & env, Value & v) void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
{ {
v.mkAttrs(state.buildBindings(attrs.size() + dynamicAttrs.size()).finish()); v.mkAttrs(state.buildBindings(attrs.size() + dynamicAttrs.size()).finish());
@ -1228,6 +1240,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
Env & env2(state.allocEnv(attrs.size())); Env & env2(state.allocEnv(attrs.size()));
env2.up = &env; env2.up = &env;
dynamicEnv = &env2; dynamicEnv = &env2;
Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, env2) : nullptr;
AttrDefs::iterator overrides = attrs.find(state.sOverrides); AttrDefs::iterator overrides = attrs.find(state.sOverrides);
bool hasOverrides = overrides != attrs.end(); bool hasOverrides = overrides != attrs.end();
@ -1240,9 +1253,9 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
Value * vAttr; Value * vAttr;
if (hasOverrides && !i.second.inherited()) { if (hasOverrides && !i.second.inherited()) {
vAttr = state.allocValue(); vAttr = state.allocValue();
mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, &env2), i.second.e); mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, inheritEnv), i.second.e);
} else } else
vAttr = i.second.e->maybeThunk(state, *i.second.chooseByKind(&env2, &env, &env2)); vAttr = i.second.e->maybeThunk(state, *i.second.chooseByKind(&env2, &env, inheritEnv));
env2.values[displ++] = vAttr; env2.values[displ++] = vAttr;
v.attrs->push_back(Attr(i.first, vAttr, i.second.pos)); v.attrs->push_back(Attr(i.first, vAttr, i.second.pos));
} }
@ -1275,10 +1288,11 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
} }
else { else {
Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, env) : nullptr;
for (auto & i : attrs) { for (auto & i : attrs) {
v.attrs->push_back(Attr( v.attrs->push_back(Attr(
i.first, i.first,
i.second.e->maybeThunk(state, *i.second.chooseByKind(&env, &env, &env)), i.second.e->maybeThunk(state, *i.second.chooseByKind(&env, &env, inheritEnv)),
i.second.pos)); i.second.pos));
} }
} }
@ -1313,6 +1327,8 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
Env & env2(state.allocEnv(attrs->attrs.size())); Env & env2(state.allocEnv(attrs->attrs.size()));
env2.up = &env; env2.up = &env;
Env * inheritEnv = attrs->inheritFromExprs ? attrs->buildInheritFromEnv(state, env2) : nullptr;
/* The recursive attributes are evaluated in the new environment, /* The recursive attributes are evaluated in the new environment,
while the inherited attributes are evaluated in the original while the inherited attributes are evaluated in the original
environment. */ environment. */
@ -1320,7 +1336,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
for (auto & i : attrs->attrs) { for (auto & i : attrs->attrs) {
env2.values[displ++] = i.second.e->maybeThunk( env2.values[displ++] = i.second.e->maybeThunk(
state, state,
*i.second.chooseByKind(&env2, &env, &env2)); *i.second.chooseByKind(&env2, &env, inheritEnv));
} }
auto dts = state.debugRepl auto dts = state.debugRepl

View file

@ -78,7 +78,7 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co
return sa < sb; return sa < sb;
}); });
std::vector<Symbol> inherits; std::vector<Symbol> inherits;
std::map<Expr *, std::vector<Symbol>> inheritsFrom; std::map<ExprInheritFrom *, std::vector<Symbol>> inheritsFrom;
for (auto & i : sorted) { for (auto & i : sorted) {
switch (i->second.kind) { switch (i->second.kind) {
case AttrDef::Kind::Plain: case AttrDef::Kind::Plain:
@ -88,7 +88,8 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co
break; break;
case AttrDef::Kind::InheritedFrom: { case AttrDef::Kind::InheritedFrom: {
auto & select = dynamic_cast<ExprSelect &>(*i->second.e); auto & select = dynamic_cast<ExprSelect &>(*i->second.e);
inheritsFrom[select.e].push_back(i->first); auto & from = dynamic_cast<ExprInheritFrom &>(*select.e);
inheritsFrom[&from].push_back(i->first);
break; break;
} }
} }
@ -100,7 +101,7 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co
} }
for (const auto & [from, syms] : inheritsFrom) { for (const auto & [from, syms] : inheritsFrom) {
str << "inherit ("; str << "inherit (";
from->show(symbols, str); (*inheritFromExprs)[from->displ]->show(symbols, str);
str << ")"; str << ")";
for (auto sym : syms) str << " " << symbols[sym]; for (auto sym : syms) str << " " << symbols[sym];
str << "; "; str << "; ";
@ -326,6 +327,12 @@ void ExprVar::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> &
this->level = withLevel; this->level = withLevel;
} }
void ExprInheritFrom::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env)
{
if (es.debugRepl)
es.exprEnvs.insert(std::make_pair(this, env));
}
void ExprSelect::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env) void ExprSelect::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env)
{ {
if (es.debugRepl) if (es.debugRepl)
@ -349,6 +356,27 @@ void ExprOpHasAttr::bindVars(EvalState & es, const std::shared_ptr<const StaticE
i.expr->bindVars(es, env); i.expr->bindVars(es, env);
} }
std::shared_ptr<const StaticEnv> ExprAttrs::bindInheritSources(
EvalState & es, const std::shared_ptr<const StaticEnv> & env)
{
if (!inheritFromExprs)
return nullptr;
// the inherit (from) source values are inserted into an env of its own, which
// does not introduce any variable names.
// analysis must see an empty env, or an env that contains only entries with
// otherwise unused names to not interfere with regular names. the parser
// has already filled all exprs that access this env with appropriate level
// and displacement, and nothing else is allowed to access it. ideally we'd
// not even *have* an expr that grabs anything from this env since it's fully
// invisible, but the evaluator does not allow for this yet.
auto inner = std::make_shared<StaticEnv>(nullptr, env.get(), 0);
for (auto from : *inheritFromExprs)
from->bindVars(es, env);
return inner;
}
void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env) void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env)
{ {
if (es.debugRepl) if (es.debugRepl)
@ -366,8 +394,9 @@ void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv>
// No need to sort newEnv since attrs is in sorted order. // No need to sort newEnv since attrs is in sorted order.
auto inheritFromEnv = bindInheritSources(es, newEnv);
for (auto & i : attrs) for (auto & i : attrs)
i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, newEnv)); i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, inheritFromEnv));
for (auto & i : dynamicAttrs) { for (auto & i : dynamicAttrs) {
i.nameExpr->bindVars(es, newEnv); i.nameExpr->bindVars(es, newEnv);
@ -375,8 +404,10 @@ void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv>
} }
} }
else { else {
auto inheritFromEnv = bindInheritSources(es, env);
for (auto & i : attrs) for (auto & i : attrs)
i.second.e->bindVars(es, i.second.chooseByKind(env, env, env)); i.second.e->bindVars(es, i.second.chooseByKind(env, env, inheritFromEnv));
for (auto & i : dynamicAttrs) { for (auto & i : dynamicAttrs) {
i.nameExpr->bindVars(es, env); i.nameExpr->bindVars(es, env);
@ -444,8 +475,9 @@ void ExprLet::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> &
// No need to sort newEnv since attrs->attrs is in sorted order. // No need to sort newEnv since attrs->attrs is in sorted order.
auto inheritFromEnv = attrs->bindInheritSources(es, newEnv);
for (auto & i : attrs->attrs) for (auto & i : attrs->attrs)
i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, newEnv)); i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, inheritFromEnv));
if (es.debugRepl) if (es.debugRepl)
es.exprEnvs.insert(std::make_pair(this, newEnv)); es.exprEnvs.insert(std::make_pair(this, newEnv));

View file

@ -129,6 +129,18 @@ struct ExprVar : Expr
COMMON_METHODS COMMON_METHODS
}; };
struct ExprInheritFrom : ExprVar
{
ExprInheritFrom(PosIdx pos, Displacement displ): ExprVar(pos, {})
{
this->level = 0;
this->displ = displ;
this->fromWith = nullptr;
}
void bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env);
};
struct ExprSelect : Expr struct ExprSelect : Expr
{ {
PosIdx pos; PosIdx pos;
@ -189,6 +201,7 @@ struct ExprAttrs : Expr
}; };
typedef std::map<Symbol, AttrDef> AttrDefs; typedef std::map<Symbol, AttrDef> AttrDefs;
AttrDefs attrs; AttrDefs attrs;
std::unique_ptr<std::vector<Expr *>> inheritFromExprs;
struct DynamicAttrDef { struct DynamicAttrDef {
Expr * nameExpr, * valueExpr; Expr * nameExpr, * valueExpr;
PosIdx pos; PosIdx pos;
@ -202,6 +215,9 @@ struct ExprAttrs : Expr
PosIdx getPos() const override { return pos; } PosIdx getPos() const override { return pos; }
COMMON_METHODS COMMON_METHODS
std::shared_ptr<const StaticEnv> bindInheritSources(
EvalState & es, const std::shared_ptr<const StaticEnv> & env);
Env * buildInheritFromEnv(EvalState & state, Env & up);
void showBindings(const SymbolTable & symbols, std::ostream & str) const; void showBindings(const SymbolTable & symbols, std::ostream & str) const;
}; };

View file

@ -117,13 +117,24 @@ inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, Expr *
auto ae = dynamic_cast<ExprAttrs *>(e); auto ae = dynamic_cast<ExprAttrs *>(e);
auto jAttrs = dynamic_cast<ExprAttrs *>(j->second.e); auto jAttrs = dynamic_cast<ExprAttrs *>(j->second.e);
if (jAttrs && ae) { if (jAttrs && ae) {
if (ae->inheritFromExprs && !jAttrs->inheritFromExprs)
jAttrs->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
for (auto & ad : ae->attrs) { for (auto & ad : ae->attrs) {
auto j2 = jAttrs->attrs.find(ad.first); auto j2 = jAttrs->attrs.find(ad.first);
if (j2 != jAttrs->attrs.end()) // Attr already defined in iAttrs, error. if (j2 != jAttrs->attrs.end()) // Attr already defined in iAttrs, error.
dupAttr(ad.first, j2->second.pos, ad.second.pos); dupAttr(ad.first, j2->second.pos, ad.second.pos);
jAttrs->attrs.emplace(ad.first, ad.second); jAttrs->attrs.emplace(ad.first, ad.second);
if (ad.second.kind == ExprAttrs::AttrDef::Kind::InheritedFrom) {
auto & sel = dynamic_cast<ExprSelect &>(*ad.second.e);
auto & from = dynamic_cast<ExprInheritFrom &>(*sel.e);
from.displ += jAttrs->inheritFromExprs->size();
}
} }
jAttrs->dynamicAttrs.insert(jAttrs->dynamicAttrs.end(), ae->dynamicAttrs.begin(), ae->dynamicAttrs.end()); jAttrs->dynamicAttrs.insert(jAttrs->dynamicAttrs.end(), ae->dynamicAttrs.begin(), ae->dynamicAttrs.end());
if (ae->inheritFromExprs) {
jAttrs->inheritFromExprs->insert(jAttrs->inheritFromExprs->end(),
ae->inheritFromExprs->begin(), ae->inheritFromExprs->end());
}
} else { } else {
dupAttr(attrPath, pos, j->second.pos); dupAttr(attrPath, pos, j->second.pos);
} }

View file

@ -319,14 +319,17 @@ binds
} }
| binds INHERIT '(' expr ')' attrs ';' | binds INHERIT '(' expr ')' attrs ';'
{ $$ = $1; { $$ = $1;
/* !!! Should ensure sharing of the expression in $4. */ if (!$$->inheritFromExprs)
$$->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
$$->inheritFromExprs->push_back($4);
auto from = new nix::ExprInheritFrom(state->at(@4), $$->inheritFromExprs->size() - 1);
for (auto & i : *$6) { for (auto & i : *$6) {
if ($$->attrs.find(i.symbol) != $$->attrs.end()) if ($$->attrs.find(i.symbol) != $$->attrs.end())
state->dupAttr(i.symbol, state->at(@6), $$->attrs[i.symbol].pos); state->dupAttr(i.symbol, state->at(@6), $$->attrs[i.symbol].pos);
$$->attrs.emplace( $$->attrs.emplace(
i.symbol, i.symbol,
ExprAttrs::AttrDef( ExprAttrs::AttrDef(
new ExprSelect(CUR_POS, $4, i.symbol), new ExprSelect(CUR_POS, from, i.symbol),
state->at(@6), state->at(@6),
ExprAttrs::AttrDef::Kind::InheritedFrom)); ExprAttrs::AttrDef::Kind::InheritedFrom));
} }

View file

@ -1,2 +1 @@
trace: used trace: used
trace: used

View file

@ -1 +1 @@
[ 1 2 { __overrides = { y = { d = [ ]; }; }; c = [ ]; d = 4; x = { c = [ ]; }; y = «repeated»; } ] [ 1 2 { __overrides = { y = { d = [ ]; }; }; c = [ ]; d = 4; x = { c = [ ]; }; y = «repeated»; } { inner = { c = 3; d = 4; }; } ]

View file

@ -2,5 +2,15 @@ let
inherit (builtins.trace "used" { a = 1; b = 2; }) a b; inherit (builtins.trace "used" { a = 1; b = 2; }) a b;
x.c = 3; x.c = 3;
y.d = 4; y.d = 4;
merged = {
inner = {
inherit (y) d;
};
inner = {
inherit (x) c;
};
};
in in
[ a b rec { x.c = []; inherit (x) c; inherit (y) d; __overrides.y.d = []; } ] [ a b rec { x.c = []; inherit (x) c; inherit (y) d; __overrides.y.d = []; } merged ]