libexpr: use allocBytes() to allocate StringData#14584
libexpr: use allocBytes() to allocate StringData#14584Ericson2314 merged 2 commits intoNixOS:masterfrom
Conversation
403b9af to
26fa5af
Compare
There was a problem hiding this comment.
Looks good, just should update the C API first and let Robert know.
As far as I can tell, this is an unavoidable breaking change of the C API.
CC @roberth
|
To expand on what @Ericson2314 said, we discussed it in the meeting and we can anticipate that a similar change will be necessary for all the c api functions that create Values (eventually, when values themselves are stored in managed memory), so we should make those changes all now, and split it off into a separate PR. I'll try to get to that later this week. |
We will need this argument in the future as we put things in managed memory. see NixOS#14584 for an example.
26fa5af to
f27b283
Compare
|
As discussed in #14592, we don't actually need to change the C API for this (and shouldn't). When we're done with managed heap changes, maybe we should come out with an API v2, but in the meantime we can work around it for now. This does come with some costs for the C API:
CC: @roberth @Ericson2314 |
f27b283 to
0e9ef38
Compare
This is usually insignificant. The number of values interacted with is generally not terribly big compared to the whole evaluation. For instance, something like
I see |
|
Currently My concern with (1) is that it's not 100% clear what the interface should look like when we're done with more of the changes. I'd rather redesign it once when we're more sure what the constraints are going to be, rather than to redesign a bit now and then realise it needs even more redesign a later. But that's a tradeoff. If you think it's better to do the intermediate redesign now I'll defer to you. |
this is a painful change. we should really add EvalState or EvalMemory as an argument to various functions as we need it, but because we want to preserve the stablity API, we hack it in as a field of nix_value.
0e9ef38 to
7cd3252
Compare
|
Got it. Can be done later, no problem. |
In NixOS/nix#14584 mkString was changed to require an `EvalMemory`, part of a larger effort to centralize memory allocation.
Motivation
EvalMemory::allocBytes() allows us to easily change how memory is allocated throughout the program. I have been using it to put all allocations in a bump allocator so I can more easily track memory leaks.
One problem is that this has to change the c bindings. I don't know how much of an issue this is as I haven't worked on the c bindings much until now.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.