Fix for func_id rollover

[swimmadude66]

Since func_id is read from magic as a 16-bit signed integer, the callFunction method was receiving it in two's complement. This meant that any func id over 32,767 wrapped to a negative number, and thus could not be found in the context's fnMap.

This PR handles this by re-typing magic to a uint32_t, allowing over 4B func ids to be generated before overflow. There is a TS-imposed limit before this, though, as the fnMap itself only supports 2^24 keys. I've added a test that all values up to that number work, and to expect a rangeError when the map is overfull. (This test takes a LONG time, so feel free to remove. just wanted a test case that failed with the current version, and passed with the uint32_t change)

This change addresses our issues, but in general, I'd like to request some way to clean up the fnMap. Even once a function is dispose or set to undefined, the fnMap retains it, and the fnIndex increases forever. I'm not sure of the best fix for this, but it means eventually even this fix will cause us to get invalid function calls.

[swimmadude66]

@justjake please let me know if there's any more info you need around this, or if there's any other work needed. I know we have an odd usecase that can even register > 32K functions on a context, but its a real blocker for us.

[justjake]

Do you need 2^24 functions registered at once, or can you make do with 2^16 functions if we can re-use function IDs after they’re dereferenced?

i would like to avoid modifying the QuickJS sources as much as possible. If the real fix here is re-using function IDs, I would prefer to do that than merge the current approach.

As is, it seems like a lot of the issue could be solved by starting the function ID at the minimum int16_t, or by casting from signed to unsigned in our C interface code without modifying the interpreter.

[swimmadude66]

Do you need 2^24 functions registered at once, or can you make do with 2^16 functions if we can re-use function IDs after they’re dereferenced?

i would like to avoid modifying the QuickJS sources as much as possible. If the real fix here is re-using function IDs, I would prefer to do that than merge the current approach.

As is, it seems like a lot of the issue could be solved by starting the function ID at the minimum int16_t, or by casting from signed to unsigned in our C interface code without modifying the interpreter.

If function Ids could be re-used, I think that addresses most of our needs. 2^16 would be an improvement over the current situation, but more is definitely better. I agree that the minimum solution is treating magic as an unsigned int (16, 32, w/e) because the current issue is that newFunction registers as unsigned, but callFunction calls in as a signed int, so the ids do not match

I've removed the modifications to quickJS, and instead only alter the type in c/interface.c. This still seems to work, and has a much smaller footprint

[justjake]

It’s possible to define a custom class from C code, and define a “finalizer” that’s called when an instance of the class is freed by the GC. I suggest investigating if you can define a custom class that behaves identically to a Function. Then in the finalizar, you can do some callback to “free” the function ID.

[swimmadude66]

It’s possible to define a custom class from C code, and define a “finalizer” that’s called when an instance of the class is freed by the GC. I suggest investigating if you can define a custom class that behaves identically to a Function. Then in the finalizar, you can do some callback to “free” the function ID.

We actually are using FinalizationRegistry to do something similar, but it seems as though GC is not reliably firing. Even still, it appears functionIDs are purely incremented, so we'd still need some way to assign the next unused function ID. I think increasing the bit space for functions from approx 2^15 to 2^32 combined with some optimizations on our side will have us sorted for quite a bit

[swimmadude66]

Commenting to add that I was apparently incorrect before. Upon removing the changes from quickJS, we are still getting the conversion to negative numbers. I'm not sure exactly where that casting needs to be done, but its apparently missing here

[justjake]

It may be most simple to start with an initial ID of the minimum int16. Then no casting necessary?

[swimmadude66]

It may be most simple to start with an initial ID of the minimum int16. Then no casting necessary?

That feels somewhat cloogier than simply using unsigned integers, no?

[justjake]

With regard to re-allocating function IDs, how do you know when a function is no longer needed by the guest? I was suggesting using finalizers inside the guest. Are you using FinalizationRegistry on the host, or inside the guest? In the case of the guest, there may be an API to explicitly run a GC cycle. In any case, you could push the no-longer-in-use function ID onto a “free-list”, and have the “get new function ID” operation on the host prefer to pop from the free-list instead of bumping the counter.

[swimmadude66]

With regard to re-allocating function IDs, how do you know when a function is no longer needed by the guest? I was suggesting using finalizers inside the guest. Are you using FinalizationRegistry on the host, or inside the guest? In the case of the guest, there may be an API to explicitly run a GC cycle. In any case, you could push the no-longer-in-use function ID onto a “free-list”, and have the “get new function ID” operation prefer to pop from the free-list instead of bumping the counter.

we are using FinalizationRegistry in the JS-land, and registering proxies to quickjs handles, using the same logic as the Scope.

I will look into the reused function ids as a fix for the future, but I think that is substantially more complex (determining when is a function ready to be freed) than handling this negative rollover at the moment. I'm looking now to compare the effort of casting vs starting functionID at minimum signed int16

[justjake]

It may be most simple to start with an initial ID of the minimum int16. Then no casting necessary?

That feels somewhat cloogier than simply using unsigned integers, no?

Diverging from upstream is a bigger cludge i think.

[swimmadude66]

It may be most simple to start with an initial ID of the minimum int16. Then no casting necessary?

That feels somewhat cloogier than simply using unsigned integers, no?

Diverging from upstream is a bigger cludge i think.

Fair enough. I'll make the min starting point change here, and look into a PR upstream to re-type magic to an unsigned int

[swimmadude66]

I've simplified the changeset and opened an issue with quickjs. Please let me know if you'd like me to make any other changes before this can land. Definitely looking forward to using the full address space for functions

[justjake]

Looks good to me!

[justjake]

By the way, to work around the issue entirely, you could use a mapping table in host user space backed by a UUID. Instead of binding 2^N functions via newFunction, just make one called dispatch, and then call dispatch.bind(uuid) inside the VM. On the outside of the VM, put your host function handler into the Map<UUID, Function>.

[justjake]

@swimmadude66 it looks like the tests failed. Can you get them to pass?

[swimmadude66]

@swimmadude66 it looks like the tests failed. Can you get them to pass?

I've pushed a "fix" which skips the massive max funcID test in debug mode, as it was timing out (and takes ~5min locally). I can re-add it with an absurd timeout if you'd like, but I think its probably ok to skip this one in debug

[justjake]

Sounds good

[swimmadude66]

@justjake sorry, I missed a place for my fix. re-pushed, but the tests don't seem to re-run automatically