wren
wren copied to clipboard
wren-lang
Pass userdata to foreign-class allocate() and finalize() functions
This builds on #971. The bindForeignClass function can return userData. The userData will then be provided to the allocator edit and finalizer, as requested at https://github.com/wren-lang/wren/pull/981#issuecomment-823426641.
Fixes #959.
@Orcolom here you go :)
Benchmark
Baseline = eca90e29
At 5bdc2170:
api_call - wren .......... 0.08s 0.0032 97.32% relative to baseline
api_foreign_method - wren .......... 0.55s 0.0088 97.50% relative to baseline
binary_trees - wren .......... 0.32s 0.0058 104.79% relative to baseline
binary_trees_gc - wren .......... 1.51s 0.0398 101.51% relative to baseline
delta_blue - wren .......... 0.28s 0.0025 97.36% relative to baseline
fib - wren .......... 0.34s 0.0083 90.45% relative to baseline
fibers - wren .......... 0.07s 0.0012 101.35% relative to baseline
for - wren .......... 0.14s 0.0019 101.58% relative to baseline
method_call - wren .......... 0.18s 0.0068 91.70% relative to baseline
map_numeric - wren .......... 1.51s 0.0058 100.63% relative to baseline
map_string - wren .......... 0.16s 0.0006 98.85% relative to baseline
string_equals - wren .......... 0.31s 0.0122 104.90% relative to baseline
I ran it a few times and the numbers fluctuate --- as far as I can tell, it's close to a wash. Other data welcome!
doesn't this now run into the problem where a static finilizer doesn't have a easy way of knowing what instanced function to pass the call onto?
True. As a side note, we might want to factor out setting a value at a given index with a filling value, since this behavior is shared with the class construction at least.
@Orcolom I didn't need that for my use case, but am happy to add it to support yours!
@Orcolom updated in 67488e8. All tests pass.
Benchmarks still fluctuate; here's the most recent---
api_call - wren .......... 0.08s 0.0031 98.40% relative to baseline
api_foreign_method - wren .......... 0.55s 0.0138 97.49% relative to baseline
binary_trees - wren .......... 0.35s 0.0042 96.16% relative to baseline
binary_trees_gc - wren .......... 1.56s 0.0185 98.88% relative to baseline
delta_blue - wren .......... 0.28s 0.0043 98.13% relative to baseline
fib - wren .......... 0.32s 0.0260 96.21% relative to baseline
fibers - wren .......... 0.07s 0.0016 102.90% relative to baseline
for - wren .......... 0.14s 0.0034 101.81% relative to baseline
method_call - wren .......... 0.18s 0.0051 94.83% relative to baseline
map_numeric - wren .......... 1.51s 0.0090 100.22% relative to baseline
map_string - wren .......... 0.15s 0.0021 100.73% relative to baseline
string_equals - wren .......... 0.33s 0.0101 98.69% relative to baseline
Hmmm, following your logic, shouldn't allocate and finalize have a separated userData ?
@mhermier it's easy to do, but is there a use case for it? E.g., if the userData is type information, the allocator and finalizer should get the same userData.
For the same reason you need a userData per method. If it was only about
the type, you could stick only one inside of class, and call it a day.
Basically, your request is about having discrimination per method, allocate
and finalize are only 2 specific case, and should not deviate.
in case of foreign class the differentiator is needed for classes. allocator and finalizer already get called via separate functions (with different signatures) so its not needed, but I do get the argument for consistency sake
The use case follows what was done with the methods have a single entry point for constructor and destructor. But for efficiency, some constructors could point to the same function (zeroing/common base class init) but destruction needs different actions.
@mhermier that makes sense to me --- it's the reverse of what I do for Vala (constructor knows the type but destructor doesn't). Plus if we make this change now, we don't have to break the API again later :) . I'll update this PR.
To add a little more. There is an inven simpler argument, the user data points to the real function in your language paradigm/abstraction. The callback act as a generic trampoline to your real function. This behavior has to be available for all forms of methods constructor and destructor.
Updated in 2cf79e0. All tests pass. Timing with respect to current main (eca90e29):
api_call - wren .......... 0.09s 0.0026 97.59% relative to baseline
api_foreign_method - wren .......... 0.55s 0.0052 100.08% relative to baseline
binary_trees - wren .......... 0.33s 0.0077 104.87% relative to baseline
binary_trees_gc - wren .......... 1.49s 0.0126 105.67% relative to baseline
delta_blue - wren .......... 0.27s 0.0018 97.69% relative to baseline
fib - wren .......... 0.33s 0.0046 96.39% relative to baseline
fibers - wren .......... 0.08s 0.0026 101.14% relative to baseline
for - wren .......... 0.14s 0.0059 98.95% relative to baseline
method_call - wren .......... 0.17s 0.0048 97.06% relative to baseline
map_numeric - wren .......... 1.54s 0.0079 103.02% relative to baseline
map_string - wren .......... 0.16s 0.0015 100.51% relative to baseline
string_equals - wren .......... 0.33s 0.0035 102.40% relative to baseline
While reviewing the generated docs, I noticed a typo --- foreign function should have been foreign instance in "storing C data". I fixed that in 227b88a.
I looked at the second patch, and the diff to the test is confusing/hard to read. Put test logic in its own class. So we can differentiate between and error accessing foreign data, from accessing user data.
@mhermier Thanks for reviewing! I restored the old foreign_class tests and added a new foreign_class_user_data test.
Thinking of it add yourself in a separated patch, not that it matters much, but at least your name will not get lost in case of revert.
Also please you remove Instance, it make the patch hard to follow. We don't need the test to be something realistic, just something that demonstrate the solution.
So all in all make the diff print smaller, avoid renaming stuff.
@mhermier Thanks!
- Added myself to AUTHORS in a separate patch in #971, as you suggested
- Removed
Instance; now using a plaindoubleas the foreign data
Also, I regenerated projects/ in new commits so that the new test files would build on all platforms.
For me as this re result is good enough for consumption, and I can finally understand what is happens in the test (you went beyond my expectations and that final results is better than what I imagined). I don't know what is a good policy for the regeneration of the makefiles, otherwise seems good for more battle testing/merge.
@ChayimFriedman2 I see you are listed as a reviewer, but GitHub won't show me your comments :( . Did you have any thoughts on the latest?
https://github.com/wren-lang/wren/pull/981#discussion_r624549593. You just force-pushed since then 😃
@ChayimFriedman2 thanks for the clarification! I thought GH was telling me you had started a comment thread.
- Rebased onto latest main and #971
- No functional changes
- All tests pass @Orcolom
test don't run because signatures don't align. (how come they do succeed for you?)
@Orcolom good catch! My compiler doesn't even flag that with -Wall :( . Fixed now, and rebased on latest main.
@Orcolom I was #including the wrong header in test/api/foreign_class_user_data.c! That is now fixed also.
Just started to look at Wren. It's shame this was never merged, was there a reason?
