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

Add lua_getuserdatadtor #870

Merged
merged 7 commits into from
Apr 11, 2023

Conversation

petrihakkinen
Copy link
Contributor

@petrihakkinen petrihakkinen commented Mar 19, 2023

Some userdata objects may need to support manual destruction in addition to automatic GC. For example, files, threads, GPU resources and objects with large external allocations.

With Lua, a finalizer can be generically called by invoking the __gc metamethod manually, but this is currently not possible with tagged userdata in Luau because it's not possible to query the destructor associated with an userdata. While it is possible to workaround this by duplicating the destructor table locally on client side (*), it's more convenient to deduplicate the data and get the destructor using the API instead.

(*) Note: a separate destructor table for each VM may be required if the VMs use different set of tags.

Implementation notes:

  1. I first considered adding a typedef for lua_Destructor but unfortunately there are two kinds of destructors, one with and one without the lua_State* argument, so I decided against it at this point. Maybe it should be added later if the destructor API is unified (by dropping the Lua state pointer argument?).

  2. For some reason the conformance test produced warning "qualifier applied to function type has no meaning; ignored" on VS2017 (possibly because the test framework does not like function pointers for some reason?). I silenced this by pulling out the test expressions from those CHECKs.

@petrihakkinen
Copy link
Contributor Author

Hi! Would it be possible to either accept or reject this pull request, please? I'm currently using a modified version of Luau with this change, and I'd prefer not having to maintain an incompatible version in the long run. It's not the end of the world if this is rejected (I can replicate the destructor table on the client side with some extra work), but at the same time I cannot think of any reason why this couldn't be in the official API... I'm happy to answer any questions.

VM/include/lua.h Outdated Show resolved Hide resolved
@vegorov-rbx
Copy link
Collaborator

Not really sure what the need for this is, calling 'destructor' multiple times on one object doesn't sound like a great idea.

I guess it's not a real destructor and might be like an fclose for a FILE, so it's not an UB from the host side (although calling fclose twice is also an UB and some mark has to be placed behind), it raises question why doesn't the userdata just have a separate function to free the resource and correctly mark it as empty.

@petrihakkinen
Copy link
Contributor Author

Not really sure what the need for this is, calling 'destructor' multiple times on one object doesn't sound like a great idea.

I guess it's not a real destructor and might be like an fclose for a FILE, so it's not an UB from the host side (although calling fclose twice is also an UB and some mark has to be placed behind), it raises question why doesn't the userdata just have a separate function to free the resource and correctly mark it as empty.

Sorry for being unclear, I'll try to explain:

There are many types of userdata which benefit from explicit destruction. Userdata representing files and GPU resources are typical examples, but basically any expensive userdata that allocates memory externally or has side-effects falls into this category.

Performing manual destruction is perfectly fine as long as the following rules are not violated:

  1. The destructor should mark the userdata as freed so that calling the destructor twice does not cause double free. For example:
if(userdata->fp)
{
	fclose(userdata->fp);
	userdata->fp = NULL;
}
  1. Any binding function that gets the userdata as an argument needs to raise an error if the userdata has been freed to prevent crashing (another option is silent treatment).

It is true that we could have a separate function for destroying each type of userdata, but in our case we have ~70 userdata types with destructors, so it's more convenient to just have a single function that frees any type of userdata than having 70 different functions that essentially do the same thing (also easier for the Lua scripter).

@vegorov-rbx vegorov-rbx merged commit 7345891 into luau-lang:master Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants