Allow setting pooling allocation in the C API #10484
Conversation
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks! Would you be up for refactoring this to use an object-style API along the lines of wasm_config_t where a new wasmtime_pooling_config_t object was added and the properties here are functions to configure each field? That way it's possible to configure just a single option while leaving all the others at their defaults, while currently it would require configuring all options at the same time.
Additionally as a minor thing, mind putting #ifdef guards around these definitions for the pooling allocator cfg being turned on?
Thanks - I switched to an object-style API and added the |
|
Yeah adding a free method for it I think is the way to go. Otherwise "cloning out" the configuration makes sense to me as well. Mind also adding |
|
Pushed some changes, please take another look. |
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks! Just some minor things here and there, but otherwise looks good
| WASMTIME_POOLING_ALLOCATION_CONFIG_PROP(total_gc_heaps, uint32_t) | ||
|
|
||
| WASM_API_EXTERN void wasmtime_pooling_allocation_strategy_set( | ||
| wasm_config_t *, wasmtime_pooling_allocation_config_t *); |
There was a problem hiding this comment.
For this I think the second argument can be const wasmtime_pooling_allocation_config_t* to reflect how it doesn't mutate it.
| WASM_API_EXTERN void wasmtime_pooling_allocation_strategy_set( | ||
| wasm_config_t *, wasmtime_pooling_allocation_config_t *); | ||
|
|
||
| #endif |
There was a problem hiding this comment.
Mind adding a // WASMTIME_FEATURE_POOLING_ALLOCATOR here to indicate which #ifdef it's closing?
| typedef struct wasmtime_pooling_allocation_config_t | ||
| wasmtime_pooling_allocation_config_t; | ||
|
|
||
| WASM_API_EXTERN wasmtime_pooling_allocation_config_t * | ||
| wasmtime_pooling_allocation_config_new(); | ||
|
|
||
| WASM_API_EXTERN void wasmtime_pooling_allocation_config_delete( | ||
| wasmtime_pooling_allocation_config_t *); |
There was a problem hiding this comment.
Can you add some documentation to these functions/types? Just some basic docs is fine, but for example indicating that *_new() must be deallocated with *_delete() and perhaps linking to the Rust documentation for wasmtime_pooling_allocation_config_t.
Also for wasmtime_pooling_allocation_strategy_set down below can you document that it doesn't take ownership of the pooling config and the pooling config still needs to be manually deleted?
No breaking C-api changes. All tests pass. Note that this release adds the pooling allocation functionality to the C api. bytecodealliance/wasmtime#10484. this functionality is not used by the wasmtime-dotnet bindings so it is not breaking, but it might be worthwhile to expose this functionality.
No breaking C-api changes. All tests pass. Note that this release adds the pooling allocation functionality to the C api. bytecodealliance/wasmtime#10484. this functionality is not used by the wasmtime-dotnet bindings so it is not breaking, but it might be worthwhile to expose this functionality.
We are troubleshooting performance / scaling issues in our project that uses Wasmtime and Pooling Allocation was recommended. Currently, we are not able to enable Pooling Allocation through the C API.
Note: mpk options are not exposed.