Make debugger significantly faster by xokdvium · Pull Request #12645 · NixOS/nix

xokdvium · 2025-03-13T13:19:12Z

Motivation

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

Context

cc @roberth for 50123f2
cc @tomberek, #12544 (comment)

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

All of this code doesn't actually depend on anything from libexpr. Because Pos is so tigtly coupled with Error, it makes sense to have in the same library.

This should provide context for follow-up commits in the patch series.

Previous implementation didn't actually check if std::get_if returned a nullptr: std::optional<SourcePath> getSourcePath() const { return *std::get_if<SourcePath>(&origin); }

roberth

❤️

roberth · 2025-03-13T14:29:08Z

src/libutil/pos-table.hh

+     * into memory each time. Call this only if absolutely necessary. Prefer
+     * to keep PosIdx around instead of needlessly converting it into Pos by
+     * using this lookup method.
+     */


thought:
Additionally it may make sense to keep a number of recent files in memory.
I doubt that it'd be noticeable after your fixes though.

Yeah, using an LRUCache might make sense if we want to speed up the debugger eval even further. Though I don't think it's strictly necessary for this PR. There's much mess to untangle and premature optimizations might introduce unnecessary complications. ~5 minor releases seem to have this performance problem, so sticking to several orders of magnitute speedup should suffice for now.

In my profiling operator[] only gets called 3 times for valgrind --tool=callgrind src/nix/nix eval nixpkgs#hello --impure --ignore-try --no-eval-cache --debugger: (after this PR)

src/libexpr/eval.hh

roberth · 2025-03-13T14:52:46Z

src/libexpr/eval-error.cc

-        .env = env,
-        .hint = HintFmt("Fake frame for debugging purposes"),
-        .isError = true});
+        /*pos=*/expr.getPos(),


Suggested change

/*pos=*/expr.getPos(),

/*pos=*/noPos,

IIUC this is equivalent and potentially slightly faster.

Not sure if we want to be saving on matches here. A virtual function call is basically free and this function seems to only be called in case an EvalError is thrown. Then all bets are off and stack unwinding shadows any possible virtual function call.

roberth · 2025-03-13T14:56:10Z

src/libexpr/eval.hh

+    std::variant<Pos, PosIdx> pos_;
+    const Expr & expr_;
+    const Env & env_;
+    HintFmt hint_;
+    bool isError_;


Normally we don't use special field names. Not a blocker for me but will check with the others.

Somewhat soon to say now, but I get the impression that we don't want this, or if we do, it'd be a bit of a project to use it consistently, handle some merge conflicts etc. Best to remove the _ suffixes for now. I feel like maybe opinions can be swayed, but let's keep that as something separate.

it'd be a bit of a project to use it consistently

Sure, I don't have any strong feelings on this.

I've also reduced the diff significantly and dropped the not strictly necessary getters. I feel like it'd be suggested either way, so I'm getting this out of the way early.

As a side note, in an ideal world there would just be a clang-tidy config for ensuring naming conventions. That way no developer time has to be spent on stuff that tooling can do automatically.

kjeremy · 2025-03-13T15:54:36Z

Should also fix #10796

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

Ericson2314 · 2025-03-13T21:41:58Z

src/libutil/error.hh

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


I am guessing experimental-features.hh is the easy thing to make more abstract

I was planning on doing that anyways for a new settings system.

Forgot to write down more thoughts.

While it would be good to slim down nix-util and have more things in libraries with well-defined scopes, I think it's ok to consider Pos to be part of a text handling component; not the evaluator. This frees it up to be used in nix.conf logic and other file types.

roberth · 2025-03-14T13:22:05Z

Thank you so much @xokdvium!

…2645 Make debugger significantly faster (backport #12645)

roberth · 2025-05-11T14:31:07Z

Released in

2.24.13
2.27.0
2.28.0

Backported but end-of-life

2.26

xokdvium requested a review from edolstra as a code owner March 13, 2025 13:19

github-actions bot added the repl The Read Eval Print Loop, "nix repl" command and debugger label Mar 13, 2025

xokdvium force-pushed the debugger-perf branch from b91a0f7 to a6443cf Compare March 13, 2025 13:24

xokdvium added 3 commits March 13, 2025 13:29

{libutil,libexpr}: Move pos-idx,pos-table code to libutil

a53b184

All of this code doesn't actually depend on anything from libexpr. Because Pos is so tigtly coupled with Error, it makes sense to have in the same library.

libutil: Document hacks and problems around Pos class

bf12aed

This should provide context for follow-up commits in the patch series.

libutil: Fix Pos::getSourcePath

50123f2

Previous implementation didn't actually check if std::get_if returned a nullptr: std::optional<SourcePath> getSourcePath() const { return *std::get_if<SourcePath>(&origin); }

xokdvium force-pushed the debugger-perf branch from a6443cf to 4351b59 Compare March 13, 2025 13:32

roberth reviewed Mar 13, 2025

View reviewed changes

xokdvium force-pushed the debugger-perf branch from 4351b59 to adbd083 Compare March 13, 2025 16:25

Ericson2314 reviewed Mar 13, 2025

View reviewed changes

roberth approved these changes Mar 14, 2025

View reviewed changes

roberth merged commit 1bff2ae into NixOS:master Mar 14, 2025
14 checks passed

roberth added performance backport 2.24-maintenance labels Mar 14, 2025

This was referenced Mar 14, 2025

Make debugger significantly faster (backport #12645) #12653

Merged

Make debugger significantly faster (backport #12645) #12654

Merged

Make debugger significantly faster (backport #12645) #12655

Merged

xokdvium deleted the debugger-perf branch March 14, 2025 13:44

mergify bot added a commit that referenced this pull request Mar 14, 2025

Merge pull request #12655 from NixOS/mergify/bp/2.27-maintenance/pr-1…

bdaa8c5

…2645 Make debugger significantly faster (backport #12645)

mergify bot added a commit that referenced this pull request Mar 14, 2025

Merge pull request #12653 from NixOS/mergify/bp/2.24-maintenance/pr-1…

56e113d

…2645 Make debugger significantly faster (backport #12645)

mergify bot added a commit that referenced this pull request Mar 14, 2025

Merge pull request #12654 from NixOS/mergify/bp/2.26-maintenance/pr-1…

74b786b

…2645 Make debugger significantly faster (backport #12645)

Mic92 added the backport 2.25-maintenance label Mar 24, 2025

mergify bot mentioned this pull request Mar 24, 2025

Make debugger significantly faster (backport #12645) #12746

Merged

mergify bot added a commit that referenced this pull request Mar 24, 2025

Merge pull request #12746 from NixOS/mergify/bp/2.25-maintenance/pr-1…

1d16259

…2645 Make debugger significantly faster (backport #12645)

roberth mentioned this pull request May 11, 2025

Calling :doc on values defined in repl segfaults #13157

Closed

2 tasks

xokdvium mentioned this pull request May 15, 2025

libexpr: Actually cache line information in PosTable #13211

Merged

This was referenced Jul 30, 2025

libexpr: Actually cache line information in PosTable (backport #13211) #13579

Closed

libexpr: Actually cache line information in PosTable (backport #13211) #13580

Merged

Uh oh!

Conversation

xokdvium commented Mar 13, 2025 • edited by roberth Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xokdvium Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjeremy commented Mar 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roberth commented Mar 14, 2025

Uh oh!

roberth commented May 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xokdvium commented Mar 13, 2025 •

edited by roberth

Loading

xokdvium Mar 13, 2025 •

edited

Loading