Skip to content

Commit 11919bc

Browse files
xokdviummergify[bot]
authored andcommitted
{libexpr,libcmd}: Make debugger significantly faster
The underlying issue is that debugger code path was calling PosTable::operator[] in each eval method. This has become incredibly expensive since 5d9fdab. While we are it it, I've reworked the code to not use std::shared_ptr where it really isn't necessary. As I've documented in previous commits, this is actually more a workaround for recursive header dependencies now and is only necessary in `error.hh` code. Some ad-hoc benchmarking: After this commit: ``` Benchmark 1: nix eval nixpkgs#hello --impure --ignore-try --no-eval-cache --debugger Time (mean ± σ): 784.2 ms ± 7.1 ms [User: 561.4 ms, System: 147.7 ms] Range (min … max): 773.5 ms … 792.6 ms 10 runs ``` On master 3604c7c: ``` Benchmark 1: nix eval nixpkgs#hello --impure --ignore-try --no-eval-cache --debugger Time (mean ± σ): 22.914 s ± 0.178 s [User: 18.524 s, System: 4.151 s] Range (min … max): 22.738 s … 23.290 s 10 runs ``` (cherry picked from commit adbd083)
1 parent 593e0ee commit 11919bc

File tree

4 files changed

+47
-29
lines changed

4 files changed

+47
-29
lines changed

src/libcmd/repl.cc

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -140,16 +140,13 @@ static std::ostream & showDebugTrace(std::ostream & out, const PosTable & positi
140140
out << ANSI_RED "error: " << ANSI_NORMAL;
141141
out << dt.hint.str() << "\n";
142142

143-
// prefer direct pos, but if noPos then try the expr.
144-
auto pos = dt.pos
145-
? dt.pos
146-
: positions[dt.expr.getPos() ? dt.expr.getPos() : noPos];
143+
auto pos = dt.getPos(positions);
147144

148145
if (pos) {
149-
out << *pos;
150-
if (auto loc = pos->getCodeLines()) {
146+
out << pos;
147+
if (auto loc = pos.getCodeLines()) {
151148
out << "\n";
152-
printCodeLines(out, "", *pos, *loc);
149+
printCodeLines(out, "", pos, *loc);
153150
out << "\n";
154151
}
155152
}

src/libexpr/eval-error.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ EvalErrorBuilder<T> & EvalErrorBuilder<T>::withFrame(const Env & env, const Expr
4545
// TODO: check compatibility with nested debugger calls.
4646
// TODO: What side-effects??
4747
error.state.debugTraces.push_front(DebugTrace{
48-
.pos = error.state.positions[expr.getPos()],
48+
.pos = expr.getPos(),
4949
.expr = expr,
5050
.env = env,
5151
.hint = HintFmt("Fake frame for debugging purposes"),

src/libexpr/eval.cc

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -771,18 +771,26 @@ void EvalState::runDebugRepl(const Error * error, const Env & env, const Expr &
771771
if (!debugRepl || inDebugger)
772772
return;
773773

774-
auto dts =
775-
error && expr.getPos()
776-
? std::make_unique<DebugTraceStacker>(
777-
*this,
778-
DebugTrace {
779-
.pos = error->info().pos ? error->info().pos : positions[expr.getPos()],
774+
auto dts = [&]() -> std::unique_ptr<DebugTraceStacker> {
775+
if (error && expr.getPos()) {
776+
auto trace = DebugTrace{
777+
.pos = [&]() -> std::variant<Pos, PosIdx> {
778+
if (error->info().pos) {
779+
if (auto * pos = error->info().pos.get())
780+
return *pos;
781+
return noPos;
782+
}
783+
return expr.getPos();
784+
}(),
780785
.expr = expr,
781786
.env = env,
782787
.hint = error->info().msg,
783-
.isError = true
784-
})
785-
: nullptr;
788+
.isError = true};
789+
790+
return std::make_unique<DebugTraceStacker>(*this, std::move(trace));
791+
}
792+
return nullptr;
793+
}();
786794

787795
if (error)
788796
{
@@ -827,7 +835,7 @@ static std::unique_ptr<DebugTraceStacker> makeDebugTraceStacker(
827835
EvalState & state,
828836
Expr & expr,
829837
Env & env,
830-
std::shared_ptr<Pos> && pos,
838+
std::variant<Pos, PosIdx> pos,
831839
const Args & ... formatArgs)
832840
{
833841
return std::make_unique<DebugTraceStacker>(state,
@@ -1104,7 +1112,7 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
11041112
*this,
11051113
*e,
11061114
this->baseEnv,
1107-
e->getPos() ? std::make_shared<Pos>(positions[e->getPos()]) : nullptr,
1115+
e->getPos(),
11081116
"while evaluating the file '%1%':", resolvedPath.to_string())
11091117
: nullptr;
11101118

@@ -1330,9 +1338,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
13301338
state,
13311339
*this,
13321340
env2,
1333-
getPos()
1334-
? std::make_shared<Pos>(state.positions[getPos()])
1335-
: nullptr,
1341+
getPos(),
13361342
"while evaluating a '%1%' expression",
13371343
"let"
13381344
)
@@ -1401,7 +1407,7 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v)
14011407
state,
14021408
*this,
14031409
env,
1404-
state.positions[getPos()],
1410+
getPos(),
14051411
"while evaluating the attribute '%1%'",
14061412
showAttrPath(state, env, attrPath))
14071413
: nullptr;
@@ -1602,7 +1608,7 @@ void EvalState::callFunction(Value & fun, std::span<Value *> args, Value & vRes,
16021608
try {
16031609
auto dts = debugRepl
16041610
? makeDebugTraceStacker(
1605-
*this, *lambda.body, env2, positions[lambda.pos],
1611+
*this, *lambda.body, env2, lambda.pos,
16061612
"while calling %s",
16071613
lambda.name
16081614
? concatStrings("'", symbols[lambda.name], "'")
@@ -1737,9 +1743,7 @@ void ExprCall::eval(EvalState & state, Env & env, Value & v)
17371743
state,
17381744
*this,
17391745
env,
1740-
getPos()
1741-
? std::make_shared<Pos>(state.positions[getPos()])
1742-
: nullptr,
1746+
getPos(),
17431747
"while calling a function"
17441748
)
17451749
: nullptr;
@@ -2123,7 +2127,7 @@ void EvalState::forceValueDeep(Value & v)
21232127
try {
21242128
// If the value is a thunk, we're evaling. Otherwise no trace necessary.
21252129
auto dts = debugRepl && i.value->isThunk()
2126-
? makeDebugTraceStacker(*this, *i.value->payload.thunk.expr, *i.value->payload.thunk.env, positions[i.pos],
2130+
? makeDebugTraceStacker(*this, *i.value->payload.thunk.expr, *i.value->payload.thunk.env, i.pos,
21272131
"while evaluating the attribute '%1%'", symbols[i.name])
21282132
: nullptr;
21292133

src/libexpr/eval.hh

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,11 +171,28 @@ struct RegexCache;
171171
std::shared_ptr<RegexCache> makeRegexCache();
172172

173173
struct DebugTrace {
174-
std::shared_ptr<Pos> pos;
174+
/* WARNING: Converting PosIdx -> Pos should be done with extra care. This is
175+
due to the fact that operator[] of PosTable is incredibly expensive. */
176+
std::variant<Pos, PosIdx> pos;
175177
const Expr & expr;
176178
const Env & env;
177179
HintFmt hint;
178180
bool isError;
181+
182+
Pos getPos(const PosTable & table) const
183+
{
184+
return std::visit(
185+
overloaded{
186+
[&](PosIdx idx) {
187+
// Prefer direct pos, but if noPos then try the expr.
188+
if (!idx)
189+
idx = expr.getPos();
190+
return table[idx];
191+
},
192+
[&](Pos pos) { return pos; },
193+
},
194+
pos);
195+
}
179196
};
180197

181198
class EvalState : public std::enable_shared_from_this<EvalState>

0 commit comments

Comments
 (0)