Skip to content

Commit 68d2292

Browse files
Ericson2314Ericson2314
authored
Merge pull request #14539 from Radvendii/exprattrs-alloc-shvach
libexpr: move ExprAttrs data into Exprs::alloc (take 2)
2 parents 16f0279 + fcf3bdc commit 68d2292

File tree

5 files changed

+77
-55
lines changed

5 files changed

+77
-55
lines changed

‎src/libexpr/eval.cc‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ bool Value::isTrivial() const
205205
{
206206
return !isa<tApp, tPrimOpApp>()
207207
&& (!isa<tThunk>()
208-
|| (dynamic_cast<ExprAttrs *>(thunk().expr) && ((ExprAttrs *) thunk().expr)->dynamicAttrs.empty())
208+
|| (dynamic_cast<ExprAttrs *>(thunk().expr) && ((ExprAttrs *) thunk().expr)->dynamicAttrs->empty())
209209
|| dynamic_cast<ExprLambda *>(thunk().expr) || dynamic_cast<ExprList *>(thunk().expr));
210210
}
211211

@@ -1226,26 +1226,26 @@ Env * ExprAttrs::buildInheritFromEnv(EvalState & state, Env & up)
12261226

12271227
void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
12281228
{
1229-
auto bindings = state.buildBindings(attrs.size() + dynamicAttrs.size());
1229+
auto bindings = state.buildBindings(attrs->size() + dynamicAttrs->size());
12301230
auto dynamicEnv = &env;
12311231
bool sort = false;
12321232

12331233
if (recursive) {
12341234
/* Create a new environment that contains the attributes in
12351235
this `rec'. */
1236-
Env & env2(state.mem.allocEnv(attrs.size()));
1236+
Env & env2(state.mem.allocEnv(attrs->size()));
12371237
env2.up = &env;
12381238
dynamicEnv = &env2;
12391239
Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, env2) : nullptr;
12401240

1241-
AttrDefs::iterator overrides = attrs.find(state.s.overrides);
1242-
bool hasOverrides = overrides != attrs.end();
1241+
AttrDefs::iterator overrides = attrs->find(state.s.overrides);
1242+
bool hasOverrides = overrides != attrs->end();
12431243

12441244
/* The recursive attributes are evaluated in the new
12451245
environment, while the inherited attributes are evaluated
12461246
in the original environment. */
12471247
Displacement displ = 0;
1248-
for (auto & i : attrs) {
1248+
for (auto & i : *attrs) {
12491249
Value * vAttr;
12501250
if (hasOverrides && i.second.kind != AttrDef::Kind::Inherited) {
12511251
vAttr = state.allocValue();
@@ -1272,8 +1272,8 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
12721272
"while evaluating the `__overrides` attribute");
12731273
bindings.grow(state.buildBindings(bindings.capacity() + vOverrides->attrs()->size()));
12741274
for (auto & i : *vOverrides->attrs()) {
1275-
AttrDefs::iterator j = attrs.find(i.name);
1276-
if (j != attrs.end()) {
1275+
AttrDefs::iterator j = attrs->find(i.name);
1276+
if (j != attrs->end()) {
12771277
(*bindings.bindings)[j->second.displ] = i;
12781278
env2.values[j->second.displ] = i.value;
12791279
} else
@@ -1285,13 +1285,13 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
12851285

12861286
else {
12871287
Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, env) : nullptr;
1288-
for (auto & i : attrs)
1288+
for (auto & i : *attrs)
12891289
bindings.insert(
12901290
i.first, i.second.e->maybeThunk(state, *i.second.chooseByKind(&env, &env, inheritEnv)), i.second.pos);
12911291
}
12921292

12931293
/* Dynamic attrs apply *after* rec and __overrides. */
1294-
for (auto & i : dynamicAttrs) {
1294+
for (auto & i : *dynamicAttrs) {
12951295
Value nameVal;
12961296
i.nameExpr->eval(state, *dynamicEnv, nameVal);
12971297
state.forceValue(nameVal, i.pos);
@@ -1325,7 +1325,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
13251325
{
13261326
/* Create a new environment that contains the attributes in this
13271327
`let'. */
1328-
Env & env2(state.mem.allocEnv(attrs->attrs.size()));
1328+
Env & env2(state.mem.allocEnv(attrs->attrs->size()));
13291329
env2.up = &env;
13301330

13311331
Env * inheritEnv = attrs->inheritFromExprs ? attrs->buildInheritFromEnv(state, env2) : nullptr;
@@ -1334,7 +1334,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
13341334
while the inherited attributes are evaluated in the original
13351335
environment. */
13361336
Displacement displ = 0;
1337-
for (auto & i : attrs->attrs) {
1337+
for (auto & i : *attrs->attrs) {
13381338
env2.values[displ++] = i.second.e->maybeThunk(state, *i.second.chooseByKind(&env2, &env, inheritEnv));
13391339
}
13401340

‎src/libexpr/include/nix/expr/nixexpr.hh‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -395,9 +395,13 @@ struct ExprAttrs : Expr
395395
}
396396
};
397397

398-
typedef std::map<Symbol, AttrDef> AttrDefs;
399-
AttrDefs attrs;
400-
std::unique_ptr<std::vector<Expr *>> inheritFromExprs;
398+
typedef std::pmr::map<Symbol, AttrDef> AttrDefs;
399+
/**
400+
* attrs will never be null. we use std::optional so that we can call emplace() to re-initialize the value with a
401+
* new pmr::map using a different allocator (move assignment will copy into the old allocator)
402+
*/
403+
std::optional<AttrDefs> attrs;
404+
std::unique_ptr<std::pmr::vector<Expr *>> inheritFromExprs;
401405

402406
struct DynamicAttrDef
403407
{
@@ -409,13 +413,20 @@ struct ExprAttrs : Expr
409413
, pos(pos) {};
410414
};
411415

412-
typedef std::vector<DynamicAttrDef> DynamicAttrDefs;
413-
DynamicAttrDefs dynamicAttrs;
416+
typedef std::pmr::vector<DynamicAttrDef> DynamicAttrDefs;
417+
/**
418+
* dynamicAttrs will never be null. See comment on AttrDefs above.
419+
*/
420+
std::optional<DynamicAttrDefs> dynamicAttrs;
414421
ExprAttrs(const PosIdx & pos)
415422
: recursive(false)
416-
, pos(pos) {};
423+
, pos(pos)
424+
, attrs(AttrDefs{})
425+
, dynamicAttrs(DynamicAttrDefs{}) {};
417426
ExprAttrs()
418-
: recursive(false) {};
427+
: recursive(false)
428+
, attrs(AttrDefs{})
429+
, dynamicAttrs(DynamicAttrDefs{}) {};
419430

420431
PosIdx getPos() const override
421432
{

‎src/libexpr/include/nix/expr/parser-state.hh‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -126,20 +126,20 @@ inline void ParserState::addAttr(
126126
for (i = attrPath.begin(); i + 1 < attrPath.end(); i++) {
127127
ExprAttrs * nested;
128128
if (i->symbol) {
129-
ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol);
130-
if (j != attrs->attrs.end()) {
129+
ExprAttrs::AttrDefs::iterator j = attrs->attrs->find(i->symbol);
130+
if (j != attrs->attrs->end()) {
131131
nested = dynamic_cast<ExprAttrs *>(j->second.e);
132132
if (!nested) {
133133
attrPath.erase(i + 1, attrPath.end());
134134
dupAttr(attrPath, pos, j->second.pos);
135135
}
136136
} else {
137137
nested = exprs.add<ExprAttrs>();
138-
attrs->attrs[i->symbol] = ExprAttrs::AttrDef(nested, pos);
138+
(*attrs->attrs)[i->symbol] = ExprAttrs::AttrDef(nested, pos);
139139
}
140140
} else {
141141
nested = exprs.add<ExprAttrs>();
142-
attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(i->expr, nested, pos));
142+
attrs->dynamicAttrs->push_back(ExprAttrs::DynamicAttrDef(i->expr, nested, pos));
143143
}
144144
attrs = nested;
145145
}
@@ -148,7 +148,7 @@ inline void ParserState::addAttr(
148148
if (i->symbol) {
149149
addAttr(attrs, attrPath, i->symbol, ExprAttrs::AttrDef(e, pos));
150150
} else {
151-
attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(i->expr, e, pos));
151+
attrs->dynamicAttrs->push_back(ExprAttrs::DynamicAttrDef(i->expr, e, pos));
152152
}
153153

154154
auto it = lexerState.positionToDocComment.find(pos);
@@ -165,8 +165,8 @@ inline void ParserState::addAttr(
165165
inline void
166166
ParserState::addAttr(ExprAttrs * attrs, AttrPath & attrPath, const Symbol & symbol, ExprAttrs::AttrDef && def)
167167
{
168-
ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(symbol);
169-
if (j != attrs->attrs.end()) {
168+
ExprAttrs::AttrDefs::iterator j = attrs->attrs->find(symbol);
169+
if (j != attrs->attrs->end()) {
170170
// This attr path is already defined. However, if both
171171
// e and the expr pointed by the attr path are two attribute sets,
172172
// we want to merge them.
@@ -181,8 +181,8 @@ ParserState::addAttr(ExprAttrs * attrs, AttrPath & attrPath, const Symbol & symb
181181
// See https://github.com/NixOS/nix/issues/9020.
182182
if (jAttrs && ae) {
183183
if (ae->inheritFromExprs && !jAttrs->inheritFromExprs)
184-
jAttrs->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
185-
for (auto & ad : ae->attrs) {
184+
jAttrs->inheritFromExprs = std::make_unique<std::pmr::vector<Expr *>>();
185+
for (auto & ad : *ae->attrs) {
186186
if (ad.second.kind == ExprAttrs::AttrDef::Kind::InheritedFrom) {
187187
auto & sel = dynamic_cast<ExprSelect &>(*ad.second.e);
188188
auto & from = dynamic_cast<ExprInheritFrom &>(*sel.e);
@@ -192,12 +192,12 @@ ParserState::addAttr(ExprAttrs * attrs, AttrPath & attrPath, const Symbol & symb
192192
addAttr(jAttrs, attrPath, ad.first, std::move(ad.second));
193193
attrPath.pop_back();
194194
}
195-
ae->attrs.clear();
196-
jAttrs->dynamicAttrs.insert(
197-
jAttrs->dynamicAttrs.end(),
198-
std::make_move_iterator(ae->dynamicAttrs.begin()),
199-
std::make_move_iterator(ae->dynamicAttrs.end()));
200-
ae->dynamicAttrs.clear();
195+
ae->attrs->clear();
196+
jAttrs->dynamicAttrs->insert(
197+
jAttrs->dynamicAttrs->end(),
198+
std::make_move_iterator(ae->dynamicAttrs->begin()),
199+
std::make_move_iterator(ae->dynamicAttrs->end()));
200+
ae->dynamicAttrs->clear();
201201
if (ae->inheritFromExprs) {
202202
jAttrs->inheritFromExprs->insert(
203203
jAttrs->inheritFromExprs->end(),
@@ -210,7 +210,7 @@ ParserState::addAttr(ExprAttrs * attrs, AttrPath & attrPath, const Symbol & symb
210210
}
211211
} else {
212212
// This attr path is not defined. Let's create it.
213-
attrs->attrs.emplace(symbol, def);
213+
attrs->attrs->emplace(symbol, def);
214214
def.e->setName(symbol);
215215
}
216216
}

‎src/libexpr/nixexpr.cc‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,9 @@ void ExprOpHasAttr::show(const SymbolTable & symbols, std::ostream & str) const
7474

7575
void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) const
7676
{
77-
typedef const decltype(attrs)::value_type * Attr;
77+
typedef const AttrDefs::value_type * Attr;
7878
std::vector<Attr> sorted;
79-
for (auto & i : attrs)
79+
for (auto & i : *attrs)
8080
sorted.push_back(&i);
8181
std::sort(sorted.begin(), sorted.end(), [&](Attr a, Attr b) {
8282
std::string_view sa = symbols[a->first], sb = symbols[b->first];
@@ -122,7 +122,7 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co
122122
str << "; ";
123123
}
124124
}
125-
for (auto & i : dynamicAttrs) {
125+
for (auto & i : *dynamicAttrs) {
126126
str << "\"${";
127127
i.nameExpr->show(symbols, str);
128128
str << "}\" = ";
@@ -401,36 +401,47 @@ ExprAttrs::bindInheritSources(EvalState & es, const std::shared_ptr<const Static
401401

402402
void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env)
403403
{
404+
// Move storage into the Exprs arena
405+
{
406+
auto arena = es.mem.exprs.alloc;
407+
AttrDefs newAttrs{std::move(*attrs), arena};
408+
attrs.emplace(std::move(newAttrs), arena);
409+
DynamicAttrDefs newDynamicAttrs{std::move(*dynamicAttrs), arena};
410+
dynamicAttrs.emplace(std::move(newDynamicAttrs), arena);
411+
if (inheritFromExprs)
412+
inheritFromExprs = std::make_unique<std::pmr::vector<Expr *>>(std::move(*inheritFromExprs), arena);
413+
}
414+
404415
if (es.debugRepl)
405416
es.exprEnvs.insert(std::make_pair(this, env));
406417

407418
if (recursive) {
408419
auto newEnv = [&]() -> std::shared_ptr<const StaticEnv> {
409-
auto newEnv = std::make_shared<StaticEnv>(nullptr, env, attrs.size());
420+
auto newEnv = std::make_shared<StaticEnv>(nullptr, env, attrs->size());
410421

411422
Displacement displ = 0;
412-
for (auto & i : attrs)
423+
for (auto & i : *attrs)
413424
newEnv->vars.emplace_back(i.first, i.second.displ = displ++);
414425
return newEnv;
415426
}();
416427

417428
// No need to sort newEnv since attrs is in sorted order.
418429

419430
auto inheritFromEnv = bindInheritSources(es, newEnv);
420-
for (auto & i : attrs)
431+
for (auto & i : *attrs)
421432
i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, inheritFromEnv));
422433

423-
for (auto & i : dynamicAttrs) {
434+
for (auto & i : *dynamicAttrs) {
424435
i.nameExpr->bindVars(es, newEnv);
425436
i.valueExpr->bindVars(es, newEnv);
426437
}
427438
} else {
428439
auto inheritFromEnv = bindInheritSources(es, env);
429440

430-
for (auto & i : attrs)
441+
for (auto & i : *attrs)
431442
i.second.e->bindVars(es, i.second.chooseByKind(env, env, inheritFromEnv));
432443

433-
for (auto & i : dynamicAttrs) {
444+
for (auto & i : *dynamicAttrs) {
434445
i.nameExpr->bindVars(es, env);
435446
i.valueExpr->bindVars(es, env);
436447
}
@@ -486,18 +497,18 @@ void ExprCall::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> &
486497
void ExprLet::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env)
487498
{
488499
auto newEnv = [&]() -> std::shared_ptr<const StaticEnv> {
489-
auto newEnv = std::make_shared<StaticEnv>(nullptr, env, attrs->attrs.size());
500+
auto newEnv = std::make_shared<StaticEnv>(nullptr, env, attrs->attrs->size());
490501

491502
Displacement displ = 0;
492-
for (auto & i : attrs->attrs)
503+
for (auto & i : *attrs->attrs)
493504
newEnv->vars.emplace_back(i.first, i.second.displ = displ++);
494505
return newEnv;
495506
}();
496507

497508
// No need to sort newEnv since attrs->attrs is in sorted order.
498509

499510
auto inheritFromEnv = attrs->bindInheritSources(es, newEnv);
500-
for (auto & i : attrs->attrs)
511+
for (auto & i : *attrs->attrs)
501512
i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, inheritFromEnv));
502513

503514
if (es.debugRepl)

‎src/libexpr/parser.y‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ expr_function
211211
| WITH expr ';' expr_function
212212
{ $$ = state->exprs.add<ExprWith>(CUR_POS, $2, $4); }
213213
| LET binds IN_KW expr_function
214-
{ if (!$2->dynamicAttrs.empty())
214+
{ if (!$2->dynamicAttrs->empty())
215215
throw ParseError({
216216
.msg = HintFmt("dynamic attributes not allowed in let"),
217217
.pos = state->positions[CUR_POS]
@@ -413,23 +413,23 @@ binds1
413413
| binds[accum] INHERIT attrs ';'
414414
{ $$ = $accum;
415415
for (auto & [i, iPos] : $attrs) {
416-
if ($accum->attrs.find(i.symbol) != $accum->attrs.end())
417-
state->dupAttr(i.symbol, iPos, $accum->attrs[i.symbol].pos);
418-
$accum->attrs.emplace(
416+
if ($accum->attrs->find(i.symbol) != $accum->attrs->end())
417+
state->dupAttr(i.symbol, iPos, (*$accum->attrs)[i.symbol].pos);
418+
$accum->attrs->emplace(
419419
i.symbol,
420420
ExprAttrs::AttrDef(state->exprs.add<ExprVar>(iPos, i.symbol), iPos, ExprAttrs::AttrDef::Kind::Inherited));
421421
}
422422
}
423423
| binds[accum] INHERIT '(' expr ')' attrs ';'
424424
{ $$ = $accum;
425425
if (!$accum->inheritFromExprs)
426-
$accum->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
426+
$accum->inheritFromExprs = std::make_unique<std::pmr::vector<Expr *>>();
427427
$accum->inheritFromExprs->push_back($expr);
428428
auto from = state->exprs.add<ExprInheritFrom>(state->at(@expr), $accum->inheritFromExprs->size() - 1);
429429
for (auto & [i, iPos] : $attrs) {
430-
if ($accum->attrs.find(i.symbol) != $accum->attrs.end())
431-
state->dupAttr(i.symbol, iPos, $accum->attrs[i.symbol].pos);
432-
$accum->attrs.emplace(
430+
if ($accum->attrs->find(i.symbol) != $accum->attrs->end())
431+
state->dupAttr(i.symbol, iPos, (*$accum->attrs)[i.symbol].pos);
432+
$accum->attrs->emplace(
433433
i.symbol,
434434
ExprAttrs::AttrDef(
435435
state->exprs.add<ExprSelect>(state->exprs.alloc, iPos, from, i.symbol),

0 commit comments

Comments
 (0)