Give foreign methods a userData field

[cxw42]

This lets a single WrenForeignMethodFn implement multiple foreign methods or otherwise change its behaviour based on a void *userData. My use case is the Vala bindings, in which I dispatch from a single WrenForeignMethodFn to any number of implementations based on userData. However, I believe others will find many uses for this!

Thank you for considering this PR!

High-level changes:

• Make WrenForeignMethodFn take (WrenVM* vm, void *userData) instead of just vm
• Make WrenBindForeignMethodFn return both a WrenForeignMethodFn and a userData value
• Corresponding changes throughout

Notes:

• All tests pass
• Fixes #942. Makes it much easier to implement #959 (only about 10 more lines of code! --- example)
• I am happy to make this a compile-time option if you would like.
• I can also split up the changes into commit(s) differently.

Timing analysis

This plot shows the results from the baseline txt file. The Y axis is score, so higher numbers are better. I ran benchmark.py --generate-baseline 10 times for each commit, and saved the baseline.txt files.

The tests are, in order left to right:

1. api_call
2. api_foreign_method
3. binary_trees
4. binary_trees_gc
5. delta_blue
6. fib
7. fibers
8. for
9. map_numeric
10. map_string
11. method_call
12. string_equals

I do not know why the change in api_call --- I looked at the code, and it is barely making any foreign method calls that I can tell. Any ideas?

[ChayimFriedman2]

In my opinion, this change is too breaking. It will break any application that embeds Wren.

[ruby0x1]

Side note: That's true for any breaking change @ChayimFriedman2 this one is no different or worse.

[CrazyInfin8]

This change is too breaking. It will break any application that embeds Wren.

While it is breaking, I can see the advantage to api's that cannot create C functions.

WrenGo currently simulates this behavior by uses an arbitrary number of C functions that access an array of Go functions. This has a major limitation that I cannot bind more functions then allocated during compile.

With this change, instead of allocating an arbitrary number of C functions during compile, I can instead store go functions in a map and call them from Go

[ChayimFriedman2]

No, I meant: we may accept breaking change that makes the situation "more correct" if it affects only a small amount of embedders, but this one affects anyone, no way you don't have foreign methods.

Edit: I agree it is useful, I'm just saying we need to find a less breaking way.

[CrazyInfin8]

Edit: I agree it is useful, I'm just saying we need to find a less breaking way.

If we want a less breaking way, I'm not sure how hard it would be but would it be possible to call a function to get the name of the currently running method with it's class type and module name? that way we don't really change the signature of that function but embedders can still tell which function was meant to run from the VM.

[mhermier]

Result are better than I anticipated. The deviation in the api_vall benchmark was expected, since it rapidly fire foreign functions to benchmark state of that specific path in the VM (Due to the extra C stack manipulation and memory growth of the class method array). As this, it is good enough. Maybe something like DEF_PRIMITIVE should be publicly provided to limit the impact of future breaking changes like this one.

[ChayimFriedman2]

I hate relying on macros for API compatibility, since:

• They are not available for bindings to other languages.
• They make it impossible to support dynamically loaded Wren version.

[cxw42]

@mhermier I think you must be right about the size of the class method array being a factor. I retested with this change on top of the PR:

--- a/src/vm/wren_vm.c
+++ b/src/vm/wren_vm.c
@@ -682,1 +682,1 @@
-  method->as.foreign(vm, method->userData);
+  method->as.foreign(vm, NULL);

The results were substantially the same as with the fetch of method->userData. In the picture below, yellow is a re-run of this PR, and green is with the diff above. Five runs of each.

[cxw42]

@mhermier @ChayimFriedman2 If we changed the API, we could make the internals a compile-time condition, like the optional modules are now. The API would still provide userdata, but it wouldn't be stored in the interpreter unless the compile-time condition were true.

[mhermier]

@cxw42 while its sounds safer, I would discourage to do so. We already have 2 dimensions to the code that are usually not well tested (debug and alternative value representation)... Let's not add another one.

[cxw42]

@mhermier I moved the foreign-method userData values into a separate Buffer and got better results:

(tests are, left to right: api_call api_foreign_method binary_trees binary_trees_gc delta_blue fib fibers for map_numeric map_string method_call string_equals --- same order as in the top post. All tests done on x64.)

All: would you please glance at https://github.com/cxw42/wren/commit/2ea1e89cc082dd54c19fa471487c4ecc74cbdb5a and see if the general approach seems reasonable? If so, I will squash that back into this PR. Thanks!

Edit in brief, the high-level changes introduced by that commit are:

• Method no longer has userData
• New type ForeignMethodUserData holds the userdata
• sObjClass now has a foreignMethodUserDatas buffer to hold the userdata (if any)
• wrenBindMethod now takes userData as a separate parameter

[mhermier]

Nice improvement.

I only fing the usage of wrenForeignMethodUserDataBufferFill a bit suspicious in case the order of the method declaration mess the fill... Otherwise it seems to be good for me.

[cxw42]

@mhermier interesting point about order --- I'll try a few different orders when I write tests.

[cxw42]

@mhermier I just squashed and updated --- seems like it works OK! See test/api/foreign_method_user_data.wren for the orders I tried.

All, I have not yet updated the docs. If this code change is approved, I will change this PR to include the updates docs, or submit a second PR.

Thanks!

[cxw42]

@ruby0x1 would you be willing to review, at your convenience? The GitHub interface isn't letting me request a review for some reason. Thank you in advance!

[Orcolom]

want to throw in that I have learned c# unity il2cpp will need this change

[Orcolom]

I feel like #959 those 10 lines should be in done in the same PR. or at least be considered to be part of this PR

[cxw42]

@Orcolom I'm happy to do so if the project maintainers don't mind!

[cxw42]

I updated the documentation. Any other thoughts or comments?

[joshgoebel]

It will break any application that embeds Wren.

It would also in many cases only be a 1 or 2 line change, no? Requiring someone to update their WrenBindForeignMethodFn API signature? And if they were linking against the library dynamically perhaps it would even just continue to work since the extra param would just be NULL? (I'm unsure of this though).

My 0.02: I feel worrying about breaking changes [so early] while we're still 0.x is a bit premature. Sure, we shouldn't break things for no reason if we can avoid it, but if we have a reason to break things I don't think the bar should be super high to breaking them if that results in an improvement (even a small one).

If someone wants stability (pre 1.0) they can pin to a specific version. When upgrading someone should expect they may need to update some of their code.

[cxw42]

in many cases only be a 1 or 2 line change, no?

Yes, as far as I know. We can't count on the extra param --- for reliability, I would recommend relinking.

0.4.0 made a similar change:

API

BREAKING: Add userData to wrenReallocateFn

(from https://github.com/wren-lang/wren/blob/main/CHANGELOG.md)

[ChayimFriedman2]

0.4.0 made a similar change:

API BREAKING: Add userData to wrenReallocateFn

You're both right and wrong.

Right, because it's a breaking change.

But wrong, because usually you don't bother to change wrenReallocateFn. The default is good enough, unless you're building an OS and need your own allocator.

OTOH, each and every embedder have foreign methods. Changing their signature will break everyone.

[cxw42]

@ChayimFriedman2 fair point! What alternative would you suggest edit for supporting Go, Vala, and C# il2cpp bindings?

[ChayimFriedman2]

I don't know. Maybe it is the right way. But it should be on the table.

[cxw42]

• Rebased on latest main
• Added a comment that foreignMethodUserDatas is separate from struct Method for performance reasons.

No functional changes.

[RobLoach]

Could you take advantage of vm->config.userData here?

[mhermier]

No. Current model rely on the fact that the user has to generate one entry point per native method. Adding that parameter, permits to have only one entry point, and discriminate the native method at runtime . It can be usefull on full featured language binding where you can create an object that automagically transform wren parameters to language binding type, without passing via the the manual decoding of parameters.