-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Improve Allocator interface so client can avoid needless copying #4431
Comments
I think you have a misunderstanding on what The data copy is only necessary if the allocator cannot expand the current memory object. Then it is forced to relocate that memory object (move it to a completly different address, not sharing a single byte of memory with the previous location) where there is enough space to store the requested data size. This also means that you are required to copy all bytes from the previous location to the new one as the new location has no defined memory content. Your description of points 1. … 4. are all insert operations that operate on a fixed memory block. If you actually need to resize the data and the allocator is required to relocate the memory block, you alway have to copy all bytes, otherwise you result with invalid data. |
No, I understand that perfectly and I believe that's not the job of the allocator to do that for the reasons I stated above. There are many situation where it's not just a blind copy and you actually want to have the data in the new memory positioned slightly differently.
No. I am saying it should fail in that case, because copying the data can be expensive and blindly copying everything might not actually be what you want.
3 and 4 aren't. But yes if you're just appending one by one to a list/array then it just so happens that it's doing the right thing. In all the other cases probably not.
Copy all the bytes, yes probably. But not necessarily in the exact same layout. And sometimes not even all the bytes, because some are unused or maybe some need to change. |
Just to be clear, do you want a |
Yes, that's what I meant.
Not entirely sure what you mean here, but I think yes. If it can't change the allocated size (and maybe smaller is always possible?) it should tell the caller "no can't resize it". |
I ran into this exact same situation in my alternative standard library for D (https://github.com/dragon-lang/mar). It's pretty easy to solve though. Just have the underlying To implement the current behavior is as simple as creating a function that takes an allocator, calls realloc, then performs the copy if it sees the address was changed. This follows the "separation of concerns" principle where the realloc is concerned with reserving space, and another function can be concerned with preserving data, and both can be swapped out independently for different behavior. |
This will not work as a successful The moving must happen before the allocator releases the old memory, but after allocating the new memory. This could probably be solved by passing an optional function to |
Oh that's right. I'm starting to remember it wasn't quite that simple :) Looking at my code now what I did was have the underlying function implement a |
See also Rust's (Side note: jemalloc and TCMalloc also both provide a sized deallocation function Personally, I'm a big fan of having in-place reallocation functions. From an allocator implementation standpoint, the only case I've heard of for implementing From a user perspective, I'd argue that |
Cool, thanks for the links with examples. I did know about those. |
Here's my proposal:
This proposal has synergy with #3803 / #3804. As example usage, ArrayList will switch to call resizeFn for ensureCapacity, followed by reallocFn with previous length of 0 if that fails, and do its own copying, avoiding copying the undefined memory, as noted in @BarabasGitHub's example, followed by shrinkFn to 0 on the old memory. |
With // note: I'm assuming resizeFn returns error{OutOfMemory}!void
pub fn reallocFn(...) Error![]u8 {
self.resizeFn(...) catch {
if (newSize < originalSize) return Error.OutOfMemory;
var newBuf = try self.alloc(...);
mem.cpy(newBuf, oldBuf, oldBuf.len);
self.free(oldBuf);
return newBuf;
};
return oldBuf; // resizeFn worked, return original buffer
} |
No, as As @Tetralux noted on Discord: |
For
I would propose that the "Allocator interface" (just the function pointers) limit each function to one operation and that we use "composition" to combine operations. Instead of having the one function that performs all 3 operations (
And Note that |
@marler8997 I like your amendment. Two questions for you:
|
Your questions show that my proposal was incomplete as I didn't clarify how shrink and free would work. Just like allocation, I see that "de-allocation" has 3 analogous operations:
Operation 2 (shrink memory in place) would be handled by Since resizing in place is handled by In order to get today's semantics of So to summarize the full Allocator interface: /// allocate a new buffer
allocFn: fn(self: *Allocator, len: usize, alignment: u29) Error![]u8,
/// frees buffer returned by allocFn, must succeed
freeFn: fn(self: *Allocator, mem: []u8) void,
/// optional function that takes a buffer returned by allocFn and attempts to resize it in place
resizeFn: fn(self: *Allocator, mem: []u8, new_len: usize) Error!void,
/// resize the buffer holding the data in `old_mem`. If the buffer is aligned, realloc
/// will attempt to resize the buffer in place. If that fails, it will create a new buffer,
/// copy the data and free the original.
pub fn realloc(self: *Allocator, old_mem: []u8, old_alignment: u29, new_byte_count: usize, new_alignment: u29) {
// TODO: implement with a combination of resizeFn/allocFn/freeFn
} So to answer your questions:
With the full interface, yes, it looks like there is a
Since the |
@marler8997 this would also mean that something like It also makes writing an allocator easier as you only have to provide both I like this! |
Good to see the discussion and I'm happy with the proposal. Cool. 👍 |
What work would still be necessary for this issue, given that #5064 (and some subsequent fixes) have been merged? |
In my opinion the current Allocator interface is suboptimal. Mainly because it does more than just allocate memory, it will also copy data for you. I think it's important to get something like this right, as it will be used pretty much everywhere. I'm giving my opinion because I'd like to see it done right, especially in Zig, which seems to get so many things right already.
Currently the Allocator interface is as follows (I left out some documentation to keep it shorter):
I have issues with the semantics of
reallocFn
(shrinkFn
is fine). It is essentially the same as the C libraryrealloc
function in that it can both modify (grow/shrink) the existing memory area, or allocate a totally new area and copy the data to there. The last part is where I think this is suboptimal. There are many cases where copying 'the data' is not what the caller wants.reallocFn
semantics, all elements after the insertion point will end up being copied twice.reallocFn
will copy all elements, while actually the rows need to be copied separately to slightly different locations. In addition to having to move the rows, the resulting code will be more complicated, because now the new memory comes already filled with the rows and the old memory block is no longer available. This means that the elements in the rows have to be copied backwards (starting from the end).reallocFn
will happily copy all the 100 elements to the new memory, while actually only a few need to be copied and the rest is undefined data.reallocFn
is completely useless and will actually destroy the data-structure.What I would like to see is in the basis an
allocate
andfree
, with on top arealloc
which only tries to modify the existing memory block and fails if that's not possible. In no case should it try to be smart and move data around, that's better left to the user, who has more knowledge and can move the data to it's correct position.Please let me know what you think.
The text was updated successfully, but these errors were encountered: