Allow not copying the wasm binary into the module when using WASM C API

[eloparco]

The wasm_module_new function copies the WASM binary passed as an argument into the module.
This PR allows passing an additional flag to wasm_module_new_ex, to avoid that copy. In that way, the binary can be manually released after module loading (instead of having to wait for the store to be destroyed).

[lum1n0us]

not very sure about it. the binary content(in wasm_module_t) will be used during execution. It's on purpose to release the binary when destroying store.

[wenyongh]

not very sure about it. the binary content(in wasm_module_t) will be used during execution. It's on purpose to release the binary when destroying store.

I think we can have a try, it really increases the memory usage if wasm-c-api clones another copy, and if it doesn't do that, the content of input buffer from developer may be modified and the buffer will be referred by runtime after loading, the difference is that developer can not use the input buffer to new a module again.

[eloparco]

not very sure about it. the binary content(in wasm_module_t) will be used during execution. It's on purpose to release the binary when destroying store.

Is that the case? When opening this PR, I had it tested only in interpreter mode and I thought it wouldn't work with AOT (that's why I created it as a draft and didn't bother fixing the CI errors).
But now I tried it in AOT mode and it seems to work as well. Is there anything I'm missing?

[lum1n0us]

be manually released after module instantiation

if this means wasm_byte_vec_delete(&binary) after wasm_instance_new(), I suggest repeat the AOT example with memset(binary.data, 0, ...) instead of wasm_byte_vec_delete.

IIUC, the big concern is function bytecodes. Classic-interp always needs the binary content. fast-interp doesn't because of recompilation. xxx_jit needs the content only during bytecodes to IRs translation. aot is there ⬆️. XIP always depends on the binary content. Then, next problem will be const strings. Luck for us, some function, like wasm_const_str_list_insert(), aot_const_str_set_insert(), have a flag to control copying. All we need is to go through.

[yamt]

Luck for us, some function, like wasm_const_str_list_insert(), aot_const_str_set_insert(), have a flag to control copying. All we need is to go through.

iirc, it isn't by luck.
it's for app-manager, which performs stream-loading, which is basically same as what this PR seems trying.
i don't know how complete it is these days though. (eg. does it cover fast-jit?)

[eloparco]

if this means wasm_byte_vec_delete(&binary) after wasm_instance_new(), I suggest repeat the AOT example with memset(binary.data, 0, ...) instead of wasm_byte_vec_delete.

Done that, it works fine. I wasn't precise there, you can't release right after module instantiation, you have to wait until wasm execution (e.g. wasm_func_call) because other APIs may use the binary buffer.

IIUC, the big concern is function bytecodes. Classic-interp always needs the binary content. fast-interp doesn't because of recompilation. xxx_jit needs the content only during bytecodes to IRs translation. aot is there ⬆️. XIP always depends on the binary content. Then, next problem will be const strings. Luck for us, some function, like wasm_const_str_list_insert(), aot_const_str_set_insert(), have a flag to control copying. All we need is to go through.

Just tried with classic interpreter (I had only tried fast interpreter and AOT successfully) and as you say this PR doesn't work there.

Then, next problem will be const strings. Luck for us, some function, like wasm_const_str_list_insert(), aot_const_str_set_insert(), have a flag to control copying. All we need is to go through.

What about those functions? Why do they need special treatment? During testing I didn't run into any problems with those.

So changes in this PR only work for fast interpreter and AOT (without XIP). Possibly JIT too, but I haven't tested it yet.
I think it makes sense to move forward with this PR and have this feature (to save memory) only for those modes that are compatible, what are your thoughts?
I initially raised #3377, but, when I was using those new APIs with the Wasm C API, I had to replace wasm_module_new with wasm_runtime_read_to_sections, that doesn't use the store and did't seem a good fit for the Wasm C API.

[eloparco]

which is basically same as what this PR seems trying.

This PR is not about stream-loading. It tries to save memory when using WAMR by allowing to release the wasm binary buffer before starting the wasm execution.

[yamt]

which is basically same as what this PR seems trying.

This PR is not about stream-loading. It tries to save memory when using WAMR by allowing to release the wasm binary buffer before starting the wasm execution.

the "make a copy instead of keeping references" aspect is basically same.

[yamt]

iirc, one of implications of is_load_from_file_buf=false is to make a copy. i guess you can somehow use it.

[eloparco]

iirc, one of implications of is_load_from_file_buf=false is to make a copy. i guess you can somehow use it.

I see that a copy is done anyway regardless of that flag

wasm-micro-runtime/core/iwasm/interpreter/wasm_runtime.c

Line 4625 in ca61184

bh_memcpy_s(node->str, len + 1, str, len);

That flag seems to be used to shift the string

wasm-micro-runtime/core/iwasm/interpreter/wasm_runtime.c

Line 4596 in ca61184

else if (is_load_from_file_buf) {

But again, if we avoid freeing the module buffer until the wasm execution that shouldn't be needed.

[yamt]

iirc, one of implications of is_load_from_file_buf=false is to make a copy. i guess you can somehow use it.

I see that a copy is done anyway regardless of that flag

wasm-micro-runtime/core/iwasm/interpreter/wasm_runtime.c

Line 4625 in ca61184

bh_memcpy_s(node->str, len + 1, str, len);

That flag seems to be used to shift the string

wasm-micro-runtime/core/iwasm/interpreter/wasm_runtime.c

Line 4596 in ca61184

else if (is_load_from_file_buf) {

in the latter case, the user-given buffer is used.

But again, if we avoid freeing the module buffer until the wasm execution that shouldn't be needed.

i don't understand what you mean. can you explain a bit?

[eloparco]

i don't understand what you mean. can you explain a bit?

I was wrong, I wasn't considering cases like this one #3389 (comment)

[wenyongh]

I discussed with @lum1n0us, per our understanding, currently it is only available for fast interpreter mode and AOT mode. For classic interpreter and all JIT modes, the bytecode is used by the interpreter or by the JIT compiler after loading, we may try to clone the bytecode to fix it, but we can do it in another PR.

So could you mention this here to avoid misunderstanding?

And could you also add another item for wasm/AOT loader, since now we can set wasm_binary_freeable=true in LoadArgs and free the wasm binary buffer after module loading. And similar, it's only available for fast interpreter mode and AOT mode now.

[eloparco]

I discussed with @lum1n0us, per our understanding, currently it is only available for fast interpreter mode and AOT mode

What is supposed to break with classic interpreter? Because I gave it a quick try and seemed to run fine.
Anyway, I updated the the doc.

[wenyongh]

The bytecodes in code section is used in classic interpreter after loading:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/interpreter/wasm_loader.c#L3700-L3710

https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/interpreter/wasm_loader.c#L5722-L5725

[eloparco]

#3389 (comment) So, yes, I actually free the wasm binary buffer after module instantiation, not loading. In that way, it works with classic interpreter too.

In my case it doesn't make much of a difference if the binary is freed after loading or after instantiation. The main goal is to free it before wasm execution, because the execution will start using additional memory and we don't want to overhead of the binary buffer.

[wenyongh]

LGTM

[lum1n0us]

What is supposed to break with classic interpreter? Because I gave it a quick try and seemed to run fine.

Please correct me if I am wrong. Your examples will free/release/pollute binary content after module loading (before instantiation). And it works well in classic-interp, fast-interp, jit and aot.

[yamt]

my impression is that this requires users too much implementation knowledge.

how about providing an api to query if it's safe to free the buffer?

eg.

bool has_reference_to_underlying_byte_vec(const wasm_module_t *module);

[eloparco]

Hi, I checked the source code again, there are really several places in wasm/aot module which refer to the input buffer

Thanks for spotting that

I am not sure how to handle them, for 1, there may be two options:
(1) allocate new memory for each data segment, (2) wasm_runtime_is_underlying_binary_freeable checks whether each data segment is dropped with bh_bitmap_get_bit and returns false if one of the data segments is not dropped.

I'll try to update the code to use (1) and see how it goes. How does (2) fix the problem with memory.init? Memory.init would be called during wasm execution and we won't be able to free the wasm binary buffer before that.

for 2, maybe we can check whether module->string_literal_ptrs is NULL

And what do we do if it's NULL?

for 3, maybe we can ignore it, and mention in the document that after the underlying binary buffer is freed, developer cannot call wasm_runtime_get_custom_section again.

Yes, makes sense to me

[wenyongh]

Hi, I checked the source code again, there are really several places in wasm/aot module which refer to the input buffer

Thanks for spotting that

I am not sure how to handle them, for 1, there may be two options:
(1) allocate new memory for each data segment, (2) wasm_runtime_is_underlying_binary_freeable checks whether each data segment is dropped with bh_bitmap_get_bit and returns false if one of the data segments is not dropped.

I'll try to update the code to use (1) and see how it goes. How does (2) fix the problem with memory.init? Memory.init would be called during wasm execution and we won't be able to free the wasm binary buffer before that.

No better way to fix (2), what we can do is to assume that memory.init is executed during instantiation and all the passive data segments are consumed and set to dropped state, so wasm_runtime_is_underlying_binary_freeable may return true if all of them are dropped.

Agree to use (1) to fully resolve the issue, and we may also free the dropped data segments after they are dropped - not in this PR, we can submit another PR to refine it.

for 2, maybe we can check whether module->string_literal_ptrs is NULL

And what do we do if it's NULL?

wasm_runtime_is_underlying_binary_freeable return false when module->string_literal_ptrs is not NULL. It only returns true when both 1 is good and module->string_literal_ptrs is NULL.

[eloparco]

No better way to fix (2), what we can do is to assume that memory.init is executed during instantiation and all the passive data segments are consumed and set to dropped state, so wasm_runtime_is_underlying_binary_freeable may return true if all of them are dropped.

Right, done that now, but I needed to change the API to accept the instance instead of the module as an argument.

[wenyongh]

This makes that the binary can be freed only after instantiation, is it better to clone data segment in wasm module and clone string_literal_ptrs in both wasm and aot modules when wasm_binary_freeable is true? So that we can free the input buffer after loading. If it is a little complex, we can help do it after this PR is merged.

[eloparco]

Yeah, we would need to clone the data_dropped bitmap into the module to avoid using the instance. But, as you say, we can do that in a separate PR

[wenyongh]

Should be to clone the data segments in wasm file but not to clone the data_dropped bitmap. Anyway, let's do it in another PR.

[wenyongh]

LGTM

[wenyongh]

Should be to clone the data segments in wasm file but not to clone the data_dropped bitmap. Anyway, let's do it in another PR.

[yamt]

i still think the load_arg->wasm_binary_freeable api is not a good idea.
with this api, if a user wants to save memory, he first needs to determine aot/interpreter type/etc by examining wamr build options and the module to be loaded to decide wasm_binary_freeable=true/false.
IMO, it should be something like "a user just expresses the intent to save memory. the runtime does it automatically when possible" instead.
how do you think?

btw, can you stop marking comments "resolved" so eagerly?

[wenyongh]

@yamt, do you mean that developer doesn't need to use wasm_runtime_is_underlying_binary_freeable/wasm_runtime_is_underlying_binary_freeable API to check again? For example, if he wants to save memory, he can just set option in LoadArgs to express the intent, and runtime does all the things, and then he can just free the input binary after loading. I strongly agree that since it simplifies the operations and makes less confusing, and we may clone data segments for wasm module, and even more, clone bytecode if needed, e.g. for classic interpreter and lazy jit. How do you think?

[eloparco]

btw, can you stop marking comments "resolved" so eagerly?

I'm not deleting them, you can still check them if needed. If I already addressed the comment, I prefer to resolved it so that I can better track what still needs to be discussed or addressed.

@yamt, do you mean that developer doesn't need to use

I guess it's more about avoiding the extra arguments in load_arg and try to save memory automatically if possible. And then the user calls wasm_runtime_is_underlying_binary_freeable to see it the runtime was able to.

[yamt]

with the current api, a user needs something complex to decide wasm_binary_freeable.
eg.

if (get_package_type(buffer) == aot) {
  loadarg.wasm_binary_freeable = true;
} else {
#if WASM_ENABLE_FAST_INTERP != 0
  loadarg.wasm_binary_freeable = true;
#else
  loadarg.wasm_binary_freeable = false;
#endif
}

i meant it can be
eg.

loadarg.want_early_free = true; /* just a hint to the runtime */

[eloparco]

loadarg.want_early_free = true; /* just a hint to the runtime */

Would it make sense to assume it to always be true? So that we don't need loadarg at all and the runtime always try to avoid the wasm binary buffer copy.

[wenyongh]

with the current api, a user needs something complex to decide wasm_binary_freeable. eg.

if (get_package_type(buffer) == aot) {
  loadarg.wasm_binary_freeable = true;
} else {
#if WASM_ENABLE_FAST_INTERP != 0
  loadarg.wasm_binary_freeable = true;
#else
  loadarg.wasm_binary_freeable = false;
#endif
}

i meant it can be eg.

loadarg.want_early_free = true; /* just a hint to the runtime */

Is that to allow the developer to free the wasm binary after wasm/aot loading? And do you mean to use the same option for both wasm-c-api and wamr api? And what should the behavior be for wasm/aot loader and wamr-c-api loader?

[wenyongh]

loadarg.want_early_free = true; /* just a hint to the runtime */

Would it make sense to assume it to always be true? So that we don't need loadarg at all and the runtime always try to avoid the wasm binary buffer copy.

I think sometimes developer cannot free the input buffer, seems we had better add an option.

[yamt]

loadarg.want_early_free = true; /* just a hint to the runtime */

Would it make sense to assume it to always be true?

it would have a negative effect to increase memory usage unless a user actually tries to free the buffer early with wasm_runtime_is_underlying_binary_freeable.

[yamt]

with the current api, a user needs something complex to decide wasm_binary_freeable. eg.

if (get_package_type(buffer) == aot) {
  loadarg.wasm_binary_freeable = true;
} else {
#if WASM_ENABLE_FAST_INTERP != 0
  loadarg.wasm_binary_freeable = true;
#else
  loadarg.wasm_binary_freeable = false;
#endif
}

i meant it can be eg.

loadarg.want_early_free = true; /* just a hint to the runtime */

Is that to allow the developer to free the wasm binary after wasm/aot loading?

it tells the runtime to perform something which possibly allows early free (typically making extra copies)
if it makes sense. otherwise, the runtime can ignore it and make
wasm_runtime_is_underlying_binary_freeable return false.

And do you mean to use the same option for both wasm-c-api and wamr api?

maybe. i haven't thought much about wasm-c-api yet.

And what should the behavior be for wasm/aot loader and wamr-c-api loader?

i'm not sure if i understand this sentence.
are you talking about the default behavior?

[wenyongh]

Is that to allow the developer to free the wasm binary after wasm/aot loading?

it tells the runtime to perform something which possibly allows early free (typically making extra copies) if it makes sense. otherwise, the runtime can ignore it and make wasm_runtime_is_underlying_binary_freeable return false.

Yes, how about loadarg.allow_early_free instead of loadarg.want_early_free? And per my underderstanding, it allows to free the binary after loading, but not after instantiation, and we will change argument wasm_module_inst_t module_inst of wasm_runtime_is_underlying_binary_freeable to wasm_module_t module, just like we discussed before in this PR.

And do you mean to use the same option for both wasm-c-api and wamr api?

maybe. i haven't thought much about wasm-c-api yet.

By default wasm-c-api will clone the binary and already allows to early free, so using the option doesn't make sense for it. Maybe we can also keep loadarg.clone_wasm_binary for wasm-c-api, and:

• If loadarg.clone_wasm_binary is true, loadarg.allow_early_free doesn't take effect
• If loadarg.clone_wasm_binary is false, wasm-c-api doesn't clone the binary, and if loadarg.allow_early_free is true, the wasm/aot loader clones data if needed

And what should the behavior be for wasm/aot loader and wamr-c-api loader?

i'm not sure if i understand this sentence. are you talking about the default behavior?

I mean the wamr API wasm_runtime_load, wasm_runtime_load_ex and wasm-c-api wasm_module_new, wasm_module_new_ex. Suggest to keep the behavior of wasm_runtime_load and wasm_module_new unchanged, and change the behavior of wasm_runtime_load_ex and wasm_module_new_ex according to the option.

[eloparco]

This makes that the binary can be freed only after instantiation, is it better to clone data segment in wasm module and clone string_literal_ptrs in both wasm and aot modules when wasm_binary_freeable is true? So that we can free the input buffer after loading. If it is a little complex, we can help do it after this PR is merged.

Been trying to use some c sample with threads (https://github.com/bytecodealliance/wasm-micro-runtime/tree/591a20b91741c24cdb9c031d3c5eec1d80bd72dc/core/iwasm/libraries/lib-wasi-threads/test) compiled with wasi-sdk. I see that in such case passive data segments are used and not all are dropped after memory.init, leading to wasm_runtime_is_underlying_binary_freeable returning false because of

wasm-micro-runtime/core/iwasm/common/wasm_runtime_common.c

Line 7283 in 591a20b

if (!bh_bitmap_get_bit(

.
Both memory.init and memory.drop are called during instantiation in the wasm start function.

Cloning the data segments into the data module would be the only way to free the wasm binary buffer in that case.

[yamt]

This makes that the binary can be freed only after instantiation, is it better to clone data segment in wasm module and clone string_literal_ptrs in both wasm and aot modules when wasm_binary_freeable is true? So that we can free the input buffer after loading. If it is a little complex, we can help do it after this PR is merged.

Been trying to use some c sample with threads (https://github.com/bytecodealliance/wasm-micro-runtime/tree/591a20b91741c24cdb9c031d3c5eec1d80bd72dc/core/iwasm/libraries/lib-wasi-threads/test) compiled with wasi-sdk. I see that in such case passive data segments are used and not all are dropped after memory.init, leading to wasm_runtime_is_underlying_binary_freeable returning false because of

wasm-micro-runtime/core/iwasm/common/wasm_runtime_common.c

Line 7283 in 591a20b

if (!bh_bitmap_get_bit(

.

the test only makes sense after instantiation. i'm not sure what you are trying to test.

Both memory.init and memory.drop are called during instantiation in the wasm start function.

it's expected for the data segment for shared memory initialization.

Cloning the data segments into the data module would be the only way to free the wasm binary buffer in that case.

what's "the data module"?

[yamt]

Is that to allow the developer to free the wasm binary after wasm/aot loading?

it tells the runtime to perform something which possibly allows early free (typically making extra copies) if it makes sense. otherwise, the runtime can ignore it and make wasm_runtime_is_underlying_binary_freeable return false.

Yes, how about loadarg.allow_early_free instead of loadarg.want_early_free?

either names are fine for me.

my point is to make it a hint so that the runtime can make the decision by itself.

And per my underderstanding, it allows to free the binary after loading, but not after instantiation, and we will change argument wasm_module_inst_t module_inst of wasm_runtime_is_underlying_binary_freeable to wasm_module_t module, just like we discussed before in this PR.

i guess it has pros and cons.
right now i don't have a clear opinion about which would be better.

And do you mean to use the same option for both wasm-c-api and wamr api?

maybe. i haven't thought much about wasm-c-api yet.

By default wasm-c-api will clone the binary and already allows to early free, so using the option doesn't make sense for it. Maybe we can also keep loadarg.clone_wasm_binary for wasm-c-api, and:

* If `loadarg.clone_wasm_binary` is true, `loadarg.allow_early_free` doesn't take effect

* If `loadarg.clone_wasm_binary` is false, wasm-c-api doesn't clone the binary, and if `loadarg.allow_early_free` is true, the wasm/aot loader clones data if needed

And what should the behavior be for wasm/aot loader and wamr-c-api loader?

i'm not sure if i understand this sentence. are you talking about the default behavior?

I mean the wamr API wasm_runtime_load, wasm_runtime_load_ex and wasm-c-api wasm_module_new, wasm_module_new_ex. Suggest to keep the behavior of wasm_runtime_load and wasm_module_new unchanged, and change the behavior of wasm_runtime_load_ex and wasm_module_new_ex according to the option.

i agree the user-visible semantics of old/standard api should not be changed.

[yamt]

are you assuming this module instance is the only one which is based on the underlying buffer?
it seems wrong because our instantiation api allows to instantiate multiple instances from a single wasm_module_t.

[eloparco]

Yeah, I think that was a simplification, but as you say it doesn't work if there are multiple instances created from the same module. We should be able to remove that check if we clone the data segments into the module, if I understand correctly.

[eloparco]

the test only makes sense after instantiation. i'm not sure what you are trying to test.

Yes, I was calling wasm_runtime_is_underlying_binary_freeable after module instantiation to make sure it was safe to free the wasm binary buffer. But memory.drop was called on all but one data segment during module instantiation, making wasm_runtime_is_underlying_binary_freeable return false.

it's expected for the data segment for shared memory initialization.

Yes, I was reading the specs and that's expected when using passive data segments (as it happens when compiling using WASI threads).

what's "the data module"?

Sorry, I just meant the module. Since passive data segments can be referred to after module instantiation, I think we need to clone them into the module if we want to free the wasm binary buffer.

[wenyongh]

@yamt since I want to merge branch main into dev/checkpoint_and_restore for @victoryang00 to have a rebase for PR #3289, I merged this PR. I think it basically implemented the ideas we discussed above, (maybe except change the option name loadarg.wasm_binary_freeable to loadard.allow_early_free?). If you prefer to use allow_early_free or have more ideas on this feature, we can go on discussion and submit another PR to refactor the code.

[yamt]

@yamt since I want to merge branch main into dev/checkpoint_and_restore for @victoryang00 to have a rebase for PR #3289, I merged this PR. I think it basically implemented the ideas we discussed above, (maybe except change the option name loadarg.wasm_binary_freeable to loadard.allow_early_free?). If you prefer to use allow_early_free or have more ideas on this feature, we can go on discussion and submit another PR to refactor the code.

i still think it should be runtime's decision, not user's.
otherwise, the api is too complex for a user to use.