Skip to content
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

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

takase1121
Copy link
Member

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 to CreateProcessW 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. When lxl_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 with lxl_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 of size_t - 2), but that's honestly a pretty respectable limit.

Copy link
Contributor

@Jan200101 Jan200101 left a 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 like lxl_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 as region. I don't think its a deal breaker but worth mentioning.

Looks good from a quick glance 👍

@takase1121
Copy link
Member Author

I think it's worth separating this and the actual process refactoring into another PR.

@adamharrison
Copy link
Member

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.

@takase1121
Copy link
Member Author

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.

@takase1121
Copy link
Member Author

To clarify the efforts of separating the allocator into a different PR is stalled, we are sticking with this.

@Guldoman Guldoman merged commit a5d466d into lite-xl:master Oct 22, 2024
10 checks passed
takase1121 pushed a commit to takase1121/lite-xl that referenced this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants