Skip to content

libexpr: Include derivation names in the call stack profile #13261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 25, 2025

Conversation

xokdvium
Copy link
Contributor

Motivation

This makes the profiler much more useful by actually distiguishing
different derivations being evaluated. This does make the implementation
a bit more convoluted, but I think it's worth it.

Example evaluation of `flutter`
$ nix eval nixpkgs#flutter --no-eval-cache --eval-profiler flamegraph

image

image

Context

Follow-up to #13220.
Current output is useful, but hard to interpret for end-users NixOS/nixpkgs#320528 (comment)


Add 👍 to pull requests you find important.

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

xokdvium added 3 commits May 25, 2025 15:52
Sometimes the profiler might want to do evaluation (e.g. for getting
derivation names). This is not ideal, but is really necessary
to make the profiler stack traces useful for end users.
This makes the profiler much more useful by actually distiguishing
different derivations being evaluated. This does make the implementation
a bit more convoluted, but I think it's worth it.
@xokdvium xokdvium requested review from roberth and edolstra as code owners May 25, 2025 15:58
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 25, 2025
@xokdvium xokdvium changed the title Eval profiler derivations libexpr: Include derivation names in the call stack profile May 25, 2025
@roberth
Copy link
Member

roberth commented May 25, 2025

This seems very useful.

does make the implementation a bit more convoluted

Maybe a nicer way to frame and implement this, is as a general way to add data to traces.
We'd have at least these two instances that could be unified in such a framework

  • derivationStrict
  • addErrorContext

and potentially more, e.g. a thunk type that remembers the mapAttrs attribute name that get computed would also provide valuable context in both error traces and profiling traces.

Doesn't have to all be done in this PR of course, but I feel that by developing this functionality more, it becomes less convoluted.

@xokdvium
Copy link
Contributor Author

Maybe a nicer way to frame and implement this, is as a general way to add data to traces.

That could be done by probably just making FrameInfo just a polymorphic base class. I initially avoided that to skip allocations and going through virtual functions, but that should probably be fine.

Generally I'm not too keen on forcing arbitrary Values and adding that to traces, because that would skew with the thunk evaluation. The profiler should be as non-intrusive as possible, but the special case of derivations is too important.

@Mic92 Mic92 merged commit 4777734 into NixOS:master May 25, 2025
13 checks passed
@xokdvium xokdvium deleted the eval-profiler-derivations branch May 25, 2025 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants