Skip to content

Commit bdaa8c5

Browse files
mergify[bot]mergify[bot]
authored
Merge pull request NixOS#12655 from NixOS/mergify/bp/2.27-maintenance/pr-12645
Make debugger significantly faster (backport NixOS#12645)
2 parents 96f0fd3 + 11919bc commit bdaa8c5

File tree

14 files changed

+123
-73
lines changed

14 files changed

+123
-73
lines changed

‎maintainers/flake-module.nix‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,6 @@
127127
''^src/libexpr/nixexpr\.cc$''
128128
''^src/libexpr/nixexpr\.hh$''
129129
''^src/libexpr/parser-state\.hh$''
130-
''^src/libexpr/pos-table\.hh$''
131130
''^src/libexpr/primops\.cc$''
132131
''^src/libexpr/primops\.hh$''
133132
''^src/libexpr/primops/context\.cc$''

‎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>

‎src/libexpr/meson.build‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,8 +172,6 @@ headers = [config_h] + files(
172172
# internal: 'lexer-helpers.hh',
173173
'nixexpr.hh',
174174
'parser-state.hh',
175-
'pos-idx.hh',
176-
'pos-table.hh',
177175
'primops.hh',
178176
'print-ambiguous.hh',
179177
'print-options.hh',

‎src/libexpr/nixexpr.cc‎

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -601,41 +601,6 @@ void ExprLambda::setDocComment(DocComment docComment) {
601601
}
602602
};
603603

604-
605-
606-
/* Position table. */
607-
608-
Pos PosTable::operator[](PosIdx p) const
609-
{
610-
auto origin = resolve(p);
611-
if (!origin)
612-
return {};
613-
614-
const auto offset = origin->offsetOf(p);
615-
616-
Pos result{0, 0, origin->origin};
617-
auto lines = this->lines.lock();
618-
auto linesForInput = (*lines)[origin->offset];
619-
620-
if (linesForInput.empty()) {
621-
auto source = result.getSource().value_or("");
622-
const char * begin = source.data();
623-
for (Pos::LinesIterator it(source), end; it != end; it++)
624-
linesForInput.push_back(it->data() - begin);
625-
if (linesForInput.empty())
626-
linesForInput.push_back(0);
627-
}
628-
// as above: the first line starts at byte 0 and is always present
629-
auto lineStartOffset = std::prev(
630-
std::upper_bound(linesForInput.begin(), linesForInput.end(), offset));
631-
632-
result.line = 1 + (lineStartOffset - linesForInput.begin());
633-
result.column = 1 + (offset - *lineStartOffset);
634-
return result;
635-
}
636-
637-
638-
639604
/* Symbol table. */
640605

641606
size_t SymbolTable::totalSize() const

‎src/libutil/error.hh‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,14 @@ struct LinesOfCode {
5050
std::optional<std::string> nextLineOfCode;
5151
};
5252

53+
/* NOTE: position.hh recursively depends on source-path.hh -> source-accessor.hh
54+
-> hash.hh -> config.hh -> experimental-features.hh -> error.hh -> Pos.
55+
There are other such cycles.
56+
Thus, Pos has to be an incomplete type in this header. But since ErrorInfo/Trace
57+
have to refer to Pos, they have to use pointer indirection via std::shared_ptr
58+
to break the recursive header dependency.
59+
FIXME: Untangle this mess. Should there be AbstractPos as there used to be before
60+
4feb7d9f71? */
5361
struct Pos;
5462

5563
void printCodeLines(std::ostream & out,

‎src/libutil/meson.build‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,7 @@ sources = files(
155155
'memory-source-accessor.cc',
156156
'mounted-source-accessor.cc',
157157
'position.cc',
158+
'pos-table.cc',
158159
'posix-source-accessor.cc',
159160
'references.cc',
160161
'serialise.cc',
@@ -225,6 +226,8 @@ headers = [config_h] + files(
225226
'muxable-pipe.hh',
226227
'os-string.hh',
227228
'pool.hh',
229+
'pos-idx.hh',
230+
'pos-table.hh',
228231
'position.hh',
229232
'posix-source-accessor.hh',
230233
'processes.hh',

‎src/libexpr/pos-idx.hh‎ renamed to ‎src/libutil/pos-idx.hh‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#pragma once
2+
///@file
23

34
#include <cinttypes>
45
#include <functional>

0 commit comments

Comments
 (0)