Skip to content

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

Closed
mergify[bot] wants to merge 2 commits into2.28-maintenancefrom
mergify/bp/2.28-maintenance/pr-13211
Closed

libexpr: Actually cache line information in PosTable (backport #13211)#13579
mergify[bot] wants to merge 2 commits into2.28-maintenancefrom
mergify/bp/2.28-maintenance/pr-13211

Conversation

@mergify
Copy link
Contributor

@mergify mergify bot commented Jul 30, 2025

Motivation

Previous code had a sneaky bug due to which no caching
actually happened:

auto linesForInput = (*lines)[origin->offset];

That should have been:

auto & linesForInput = (*lines)[origin->offset];

See 1.

Now that it also makes sense to make the cache bound in size
in order not to memoize all the sources without freeing any memory.
The default cache size has been chosen somewhat arbitrarily to be ~64k
origins. For reference, 25.05 nixpkgs has ~50k .nix files.

Simple benchmark:

let
  pkgs = import <nixpkgs> { };
in
builtins.foldl' (acc: el: acc + el.line) 0 (
  builtins.genList (x: builtins.unsafeGetAttrPos "gcc" pkgs) 10000
)

(After)

$ hyperfine "result/bin/nix eval -f ./test.nix"
Benchmark 1: result/bin/nix eval -f ./test.nix
  Time (mean ± σ):     292.7 ms ±   3.9 ms    [User: 131.0 ms, System: 120.5 ms]
  Range (min … max):   288.1 ms … 300.5 ms    10 runs

(Before)

hyperfine "nix eval -f ./test.nix"
Benchmark 1: nix eval -f ./test.nix
  Time (mean ± σ):     666.7 ms ±   6.4 ms    [User: 428.3 ms, System: 191.2 ms]
  Range (min … max):   659.7 ms … 681.3 ms    10 runs

If the origin happens to be a all-packages.nix or similar in size then the
difference is much more dramatic.

Context

#12645 (comment)

@roberth As it turns out the PosTable was indeed supposed to do the caching, but didn't due to the bug I described above.


Add 👍 to pull requests you find important.

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


This is an automatic backport of pull request #13211 done by [Mergify](https://mergify.com).

xokdvium added 2 commits July 30, 2025 11:51
For heavier objects it doesn't make sense to return
a std::optional with the copy of the data, when it
can be used by const reference.

(cherry picked from commit 4711720)

# Conflicts:
#	src/libutil/include/nix/util/lru-cache.hh
Previous code had a sneaky bug due to which no caching
actually happened:

```cpp
auto linesForInput = (*lines)[origin->offset];
```

That should have been:
```cpp
auto & linesForInput = (*lines)[origin->offset];
```

See [1].

Now that it also makes sense to make the cache bound in side
in order not to memoize all the sources without freeing any memory.
The default cache size has been chosen somewhat arbitrarily to be ~64k
origins. For reference, 25.05 nixpkgs has ~50k .nix files.

Simple benchmark:

```nix
let
  pkgs = import <nixpkgs> { };
in
builtins.foldl' (acc: el: acc + el.line) 0 (
  builtins.genList (x: builtins.unsafeGetAttrPos "gcc" pkgs) 10000
)
```

(After)

```
$ hyperfine "result/bin/nix eval -f ./test.nix"
Benchmark 1: result/bin/nix eval -f ./test.nix
  Time (mean ± σ):     292.7 ms ±   3.9 ms    [User: 131.0 ms, System: 120.5 ms]
  Range (min … max):   288.1 ms … 300.5 ms    10 runs
```

(Before)

```
hyperfine "nix eval -f ./test.nix"
Benchmark 1: nix eval -f ./test.nix
  Time (mean ± σ):     666.7 ms ±   6.4 ms    [User: 428.3 ms, System: 191.2 ms]
  Range (min … max):   659.7 ms … 681.3 ms    10 runs
```

If the origin happens to be a `all-packages.nix` or similar in size then the
difference is much more dramatic.

[1]: https://www.github.com/lix-project/lix/commit/22e3f0e9875082be7f4eec8e3caeb134a7f1c05f

(cherry picked from commit 5ea81f5)
@mergify mergify bot added the automatic backport This PR is a backport produced by automation (does not trigger backporting) label Jul 30, 2025
@mergify mergify bot requested a review from edolstra as a code owner July 30, 2025 11:51
@mergify
Copy link
Contributor Author

mergify bot commented Jul 30, 2025

Cherry-pick of 4711720 has failed:

On branch mergify/bp/2.28-maintenance/pr-13211
Your branch is up to date with 'origin/2.28-maintenance'.

You are currently cherry-picking commit 4711720ef.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   src/libutil/include/nix/util/lru-cache.hh

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@mergify mergify bot added the automatic backport This PR is a backport produced by automation (does not trigger backporting) label Jul 30, 2025
@xokdvium
Copy link
Contributor

@roberth, to backport this you'd also need other patches I did for the LRUCache.

@roberth
Copy link
Member

roberth commented Jul 30, 2025

I think the risk is covered, but not sure if worth the effort.

@xokdvium xokdvium closed this Aug 31, 2025
@xokdvium xokdvium deleted the mergify/bp/2.28-maintenance/pr-13211 branch October 7, 2025 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automatic backport This PR is a backport produced by automation (does not trigger backporting) conflicts merge-queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants