Skip to content

Conversation

@Radvendii
Copy link
Contributor

@Radvendii Radvendii commented Sep 28, 2025

Motivation

See this tracking issue for big-picture plan and motivation

Where does this fall in that picture?

  • Removes 24-32 bytes from each ExprPath (size of std::string)
    • This is not very much. ~500KB
  • Cut excess allocations off the ends of strings
    • This is more than I would have expected. ~5MB (unless there's some other memory reduction I'm not accounting for)
  • Once Exprs live in a std::vector, they will need to be trivially moveable. Because of the small string optimization of std::string, moving an ExprPath invalidates its Value's pointer.

Note that we've added a bunch of copies that weren't necessary before. This doesn't seem to impact performance. In the future, we can address this by passing the allocator down into functions like getHome() and having them allocate directly there instead.

Rant about plans for removing the `accessor` field as well I would like to also remove `ref accessor`. It does take up 16 needless bytes, but the bigger reason is that eventually I want `ExprInt`, `ExprFloat`, `ExprString`, and `ExprPath`, all the "literals" to be referred to **as** Values directly (See tracking issue's todo item "Combine `Expr`s and `Value`s into one address space...") This would remove a layer of indirection and the need to store the Expr at all, but it requires that they not have any other fields (i.e. `accessor`).

The only thing accessor is doing is incrementing/decrementing a refcount for the SourceAccessor. I scoured the internet and could not find a reasonable way to manually increment / decrement the refcount of an std::shared_ptr. I did come up with a janky way that involves using memset and memcpy to skip constructors/destructors, but that seemed like a bad path to go down.

I'm now thinking boost::intrusive_ptr is the way to go here, but I think we should tackle that down the road when it actually becomes relevant.

Context


Add 👍 to pull requests you find important.

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

@Mic92
Copy link
Member

Mic92 commented Sep 29, 2025

cc @NaN-git @xokdvium

@edolstra
Copy link
Member

We can remove the accessor field entirely. It was introduced to support lazy trees, but it's no longer needed there since we mount every input on top of /nix/store using MountedSourceAccessor. So almost every path is using rootFS anyway.

I have an unfinished PR stashed away somewhere that does this. I'll have a look at reviving it.

@Radvendii
Copy link
Contributor Author

I'm working on reviving Eelco's changes (thanks!) In the meantime that shouldn't impact this PR.

@Mic92 Mic92 merged commit a5b35ec into NixOS:master Sep 30, 2025
15 checks passed
@edolstra
Copy link
Member

edolstra commented Oct 2, 2025

BTW, is this stuff thread-safe? I'd hate to move away from the goal of having a multi-threaded evaluator.

@Mic92
Copy link
Member

Mic92 commented Oct 2, 2025

It doesn't look like it, but a thread-safe bumper allocator sounds easy to implement.
This is the current implementation: https://github.com/gcc-mirror/gcc/blob/264a575765e0985fdc8a00b16c1d5bb4a46ed4db/libstdc%2B%2B-v3/include/std/memory_resource#L427

All we need is an atomic for bumping the size.

@Mic92
Copy link
Member

Mic92 commented Oct 2, 2025

Opened up: #14140

}
Path path(getHome() + std::string($1.p + 1, $1.l - 1));
$$ = new ExprPath(ref<SourceAccessor>(state->rootFS), std::move(path));
$$ = new ExprPath(state->alloc, ref<SourceAccessor>(state->rootFS), path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we somehow make state->alloc thread local? That would also solve the issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt having more global variables is a good idea. Maybe as a last resort, but it seems unnecessary at this point if there's other avenues we can explore first?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants