Finding a good way to store a pointer to a std.mem.Allocator for Ziglua

Help
1

Seeing Should allocators be passed in as a value or as a pointer? several days ago reminded me of a question I have regarding Ziglua.

Lua supports using a custom allocator for it’s internal allocations. For this to work, Ziglua currently uses a pointer to a std.mem.Allocator for initalization, and I’m wondering if there is a better way to more closely follow Zig conventions. I’d appreciate any feedback you wonderful smart people might have.

Some context for those not familiar with Lua’s internals: A Lua state is created with lua_newstate. Which accepts a lua_Alloc function and custom data void pointer that is passed to the allocator function.

Because this custom data needs to be a pointer, the Ziglua init function uses a *std.mem.Allocator so that can be stored by Lua internally and passed to the custom allocator function

So when initializing a Lua state with Ziglua the pointer to the allocator should be stable to prevent issues (you don’t want to use a pointer to stack memory that may be invalidated). But this is unconventional and I don’t want this to lead to issues.

A possible issue in the wild

I was reading through the comlink source recently and noticed the code passes an allocator pointer to the Lua.init() function. It seems to work fine (maybe this function is inlined by llvm or the allocator argument is optimized to a pointer?) but I want to remove/minimize this possible error. I don’t point this out to call out comlink. I just want to show that this is a possible issue

Here are some ideas I have considered:

  1. Documenting the function better to hopefully avoid mistakes. This is simple, but doesn’t prevent mistakes

  2. Ziglua previously stored an allocator pointer inside a state struct, and the init function would create a *std.mem.Allocator to store a pointer internally (source here). When I switched from a struct to an opaque I removed the internal allocator pointer.

    A motivator for changing to an opaque was to remove the allocator field entirely because it was causing confusion. I could switch back to a struct though. I’m leaning toward this because it seems the least bad.

    I could possibly even create an allocator pointer, pass it to lua_newstate and then not store it as an extra field on the Lua struct, instead using getAllocFn to get the pointer to free it when the Lua state is closed. But that also isn’t perfect because setAllocFn could overwrite the data pointer and cause a leak.

  3. Require the Lua object to exist through a stable pointer so the init function has an address to store the allocator at. Something like this

    var lua: Lua = undefined;
try lua.init(allocator);

    but if someone decides to initialize Lua inside a function and then return the Lua state then there are still issues.

  4. Lua has a concept of “extra space” which is a memory region associated with each Lua state. I could maybe store the Zig allocator pointer there, but that feels convoluted (and not much different from idea #2) and it could be overwritten by someone using the lua_getextraspace function.

tldr: I need a good way to have a stable pointer to a std.mem.Allocator that Lua can access safely. Maybe I’m just overthinking things…

4 Likes
2

I’d suggest the standard library solution, which is to use an Allocator, not a *Allocator. You’ll see this in the structs which come in Unmanaged and implicitly-managed variants: an ArrayListAligned has a field allocator: Allocator.

If you visit the source code for std.mem.Allocator, you’ll discover that it’s just two pointers, one to the implementation, and one to the VTable struct. So a *Allocator is a pointer-to-pointers, and there’s no real advantage in doing that, it causes extra complexity (since something has to manage the pointed-at Allocator) and it saves one usize in the struct.

I’m a big Lua fan, btw, so thanks for working on Ziglua! The two languages seem like a natural fit.

Edited to add: I did see that this isn’t a complete solution, and meant to include something about that in this reply. You’ll spot how to give Lua the interface it needs by reading through std.mem.Allocator. It’s the Zig context struct which should just hold an Allocator directly. I think this will make sense to you pretty fast.

3 Likes
3

Thanks for taking the time to reply!

I’m not sure if using a plain Allocator struct will work though. Lua needs a pointer (unless I’m missing something very obvious!)

When initializing the Lua state, the second parameter is a void pointer for custom data passed to the alloc function: c.lua_newstate(allocfn, &allocator). Lua (the C code) will store the function pointer, and the void pointer internally. At any time Lua can call the alloc function, passing it that pointer for any data that alloc function needs. So that pointer should always be valid so Lua can use it

So in my case, the alloc function expects that void pointer to be a pointer to a Zig allocator so I can cast it back

fn alloc(data: ?*anyopaque, ptr: ?*anyopaque, osize: usize, nsize: usize) callconv(.C) ?*align(alignment) anyopaque {
    // Get the allocator pointer from the opaque/void pointer
    const allocator_ptr: *Allocator = @ptrCast(@alignCast(data.?));

    // the rest of this function doesn't matter for this context ...
}

So with this, I’m not sure how I could use a plain Allocator and allow Lua to access that data.

But this does create some weirdness because using a *Allocator in zig is not conventional. That’s why I’m trying to think of

4

I wanted to point you toward std.mem.Allocator specifically, because there are some things there which should help you figure that out.

For instance, this is the Allocator struct:

const Allocator = struct {
     ptr: *anyopaque,
     vtable: *const VTable,
}

Re-written because the whole file is the struct definition. But those are the fields.

Then the vtable has function signatures like this:

    alloc: *const fn (ctx: *anyopaque, len: usize, ptr_align: u8, ret_addr: usize) ?[*]u8,

And the Allocator provides some functions like this one:

/// This function is not intended to be called except from within the
/// implementation of an Allocator
pub inline fn rawAlloc(self: Allocator, len: usize, ptr_align: u8, ret_addr: usize) ?[*]u8 {
    return self.vtable.alloc(self.ptr, len, ptr_align, ret_addr);
}

So without knowing more about Ziglua than I do, I don’t know much about how you can store the Allocator pair, but I expect you can pass in a function which uses the Allocator’s context pointer more directly, and make that the void * which the interface expects. Or maybe it makes more sense to construct a unique context struct out of the parts you can find in the Allocator interface, that’s probably the case actually.

It’s the extra level of indirection here, which sort of cancels out, if that makes sense.

1 Like
5

@natecraddock, can you clarify something for me - are you using a global allocator that then gets passed around as a pointer? Or are you allowing people to create custom allocators like they do in Zig and then make interfaces to them?

In other words, is the allocator in question a kind of “stand-in” for malloc?

3 Likes
6

I remember seeing this in the Ziglua source and wondering if it could be done better, but after thinking for a while I wasn’t able to come up with anything. But taking a peek at the Lua source code, I think I’ve got something now. Fair warning - it’s pretty cursed.

lua_newstate allocates memory for the *Lua it returns using the given function, so you could write a wrapper function that hijacks the first allocation and allocates 16 bytes more than requested, store the allocator in there, and update the pointer using lua_setallocf.
You’d also have to hijack the lua_close function so it gets freed properly.

I really want to try this out now but it’s way too late. I’ll give it a shot in the morning.

Edit: This is basically a more convoluted version of number 4, except it doesn’t interfere with usage of lua_getextraspace.

1 Like
7

This would be my personal preference, since it would avoid compromising on the design of the State type (it can remain opaque, and the *State pointer is exactly the same as the lua_State* in C land) and doesn’t impact the rest of the user’s potential usage of the library (as option 4 would).

I would also not even expose the getAllocFn and setAllocFn functions in the bindings at all, since they conflict with the original intention of using a Zig Allocator to handle all the memory allocation. If a user were to create a State using one allocator initially and then later decide to call setAllocFn and change that, the new allocator would have no idea how to resize or free allocations created by the first allocator. Any potential use-case for changing the allocation strategy, tracing, etc. on an existing State I feel would be better served by just making the user construct their own Allocator implementation which supports that, and then pass that to init.

3 Likes
8

Ahh I get what you mean now. Thank you for clarifying.

This does seem like a possible idea to extract the internal ptr and vtable of an Allocator parameter and then wrap them inside my own custom type that I can then pass on to Lua. At that point though, it’s easier to just internally allocate a *Allocator and pass that to Lua like I used to do.

I’ll have to give this more thought and test it to see what I think!

1 Like
9

No, not a global allocator. Here is an example from the Ziglua repo (see here for the full context)

const std = @import("std");
const ziglua = @import("ziglua");
const Lua = ziglua.Lua;

pub fn main() anyerror!void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = gpa.allocator();
    defer _ = gpa.deinit();

    // Initialize The Lua vm and get a reference to the main thread
    //
    // Passing a Zig allocator to the Lua state requires a stable pointer
    var lua = try Lua.init(&allocator);
    defer lua.deinit();
}

So this should support any regular and custom Zig Allocator.

10

Take this with a grain of salt, but I’m not sure that changing it to an allocator instance instead of a pointer in this case is going to alter your code to the point of being drastically better.

We recommend this as a design decision because allocators are lightweight and the less indirect you can be, the better. I’m curious to see if after all this work is said and done if you would see any statistically significant performance improvements. If not, it sounds like (in my opinion) that it’s the presence of a pointer itself that is the issue. In that case, I’d weigh the pros and cons here.

1 Like
11

@efjimm Feel free to open an issue/PR on the repo if you have any good ideas, even cursed ones! I think I follow what you mean

I actually have an issue for this! Remove newState, newStateLibc, and setAllocF · Issue #51 · natecraddock/ziglua · GitHub. But thank you for your reply. I think you put my thoughts into words better than I can. I agree that exposing the alloc functions really isn’t useful from the context of Zig bindings.

I’ll think a bit more on the ideas proposed here, but right now I’m leaning toward removing those functions you mentioned. That will simplify passing the allocator, and remove ways to break things by changing the allocator.

Yeah I really don’t expect any performance difference either way. The only goal here is to make the Lua.init() function more conventional by not requiring a *Allocator for one of the parameters.

At some point during initialization a pointer to the Zig Allocator does need to be passed to Lua, but there have been some good ideas shared here on how to go about that.

Thanks all for the feedback! It’s nice to bounce design ideas around

2 Likes
12

How about going all in on lua, also a bit cursed:

  1. Take a std.mem.Allocator in init
  2. Create the lua state with no custom allocator
  3. With the state created, store the Allocator.ptr and Allocator.vtable as lightuserdata in the global registry index
  4. Call lua_setallocf with a custom alloc fn, and the lua state pointer as the userdata. In the alloc function, you have the lua state now which you can retrieve the pointers to the original Allocator, and then recreate it.

I’m not sure if we end up leaking…maybe lua allocates when it stores in the registry index? Hopefully it has capacity for 2 pointers without allocating?

[EDIT]: I realized that you could create the state using a pointer to the passed in Allocator, and a different allocator function. Any allocations that happen before you call lua_setallocf would be done with the same allocator, you just need a separate function so you can cast the userdata into the correct type. As long as you replace the userdata before the function exits all should be good. This is pretty similar to @efjimm’s method.

  1. Take a std.mem.Allocator in init
  2. Create the lua state with a reference to this allocator and the current alloc fn
  3. With the state created, store the Allocator.ptr and Allocator.vtable as lightuserdata in the global registry index
  4. Call lua_setallocf with a different alloc fn, and the lua state pointer as the userdata. In the alloc function, you have the lua state now which you can retrieve the pointers to the original Allocator, and then recreate it

Also lol that you found this in comlink :slight_smile: I’m not sure how I haven’t run into an issue yet :/…will have to fix it before I do.

2 Likes
13

Oh this is also a nice idea I hadn’t considered. Thanks… I’ll have to see if it works or if it is too cursed :joy:

I’m thinking storing it in Lua as lightuserdata would have more overhead than just a pointer created by Zig. Because then you would have to use toUserdata which is more work than simply casting a pointer. Probably only a trivial amount of overhead, but I’d rather make things optimal if possible.

I hadn’t thought of doing things in stages. Thanks for these ideas, they definitely give me some things to try to see what I like best.

I was looking through the source trying to see more real code that uses vaxis. And then I came across it and I was baffled!

I ended up disassembling comlink and it looks like all the init code is inlined which is why there are no issues. That’s at least my best understanding.

14

It took me a while to get around to resolving this, but I decided on approach #2 after @ianprime0509 made good points about not needing the getAllocFn and setAllocFn functions.

Here’s the diff if anyone is interested: Remove pointer from Lua.init allocator parameter · natecraddock/ziglua@3f1454a · GitHub

Thanks again for everyone’s input and suggestions :slight_smile:

3 Likes