-
Notifications
You must be signed in to change notification settings - Fork 236
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
Refactor how arguments are handled in process.start() #1854
Refactor how arguments are handled in process.start() #1854
Conversation
This allocator uses Lua userdatas for dynamic allocation that is automatically freed when the current scope exits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments I made on discord:
- not a fan of calling an arena instance
A
, but the naming is temporary. I sadly don't have a real alternative since it would just look likelxl_arena *arena = lxl_arena_init(L);
which uhhh... yeah not pretty arena
is how this type of allocation system is often called but since they aren't native to C it might be better to choose a more descriptive name such asregion
. I don't think its a deal breaker but worth mentioning.
Looks good from a quick glance 👍
I think it's worth separating this and the actual process refactoring into another PR. |
Agreed generally; I'm not super in favour of the arena allocator (realistically, I'd prefer doing things in process, as this is currently used nowhere else; if you have a compelling case as to where you want to use this elsewhere in core, I'm more in favour), but am definitely in favour of migrating a bunch of the logic to lua. |
I don't see any issues with the Arena allocator. It's simply a way to allocate memory while respecting Lua's GC and stuff. There's another PR #1855 that specifically addresses this. I just think that it's a good, safe way to write better bindings. It not being used now doesn't mean it won't be used later, the whole point of having this in the PR is to have a POC that this allocator is useful. |
To clarify the efforts of separating the allocator into a different PR is stalled, we are sticking with this. |
This PR started with great intentions but kinda languished in the end. I've ported most of the argument handling especially you Windows to Lua, and hoped that it would reduce the complexity of the C code and make it easier to debug. Midway through I was distracted by the arena allocator and focused some effort into it.
The boring stuff: moving argument parsing into Lua
The Windows code is a mess of spaghetti with encoding conversion at various places, and nightmares with memory allocation (especially when dealing with the luaL_check* functions, where it could jump out of the function, skipping memory management).
I moved the majority of the code into Lua, making
process.start()
(at least the unwrapped, C version) accept only what they need: A command line string on Windows, and the environment variables concatenated into a single string. This allows the arguments to be passed directly toCreateProcessW
with a bit of string manipulation on the Unix side.The more interesting stuff:
arena_allocator
This popped up mid-conversation with @Guldoman, but it is possible to use Lua userdata pushed to a table to control their lifetime. PUC Lua itself allows this (not sure about LuaJIT); even when this is not allowed, you can simply add all allocation to a Lua table, assign a finalizer for the said table, and free memory in the finalizer. There is no finer-grain control over this yet (at least I am not aware of, looking at to-be-closed variables here), but this GREATLY simplifies memory handling inside a C Lua API function. Return anywhere, and your memory is freed. It is semi-similar to golang arenas.
This arena allocator is implemented as a Lua table, with a userdata (the
lxl_arena
) in it. Whenlxl_arena_malloc
is called, we get the Lua state, allocate some userdata and store it in the table. This userdata is now "pinned" to the table and freed when the table gets freed. You can optionally free memory in an arena withlxl_arena_free
, but its absolutely optional and might be useful for implementing memory checking.lxl_arena_init
pushes the table onto a stack, and this means that unless you accidentally pop this item off the stack, it will be valid for the duration of the C function. You can also pin this table to the Lua Registry for some long-running processes.The limit of this system is that there's a fixed limit to the userdata size,
((size_t)(~(size_t)0)-2)
(maximum value ofsize_t
- 2), but that's honestly a pretty respectable limit.