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);

[wenyongh]

@eloparco just my suggestion: maybe we can add two APIs, one is for WAMR api and is added in wasm_export.h, the other is for wasm-c-api and is added inc wasm_c_api.h

Added those (separate commit), not sure how useful they are. Why would the developer use wasm_module_is_underlying_binary_cloned? They already know if they passed clone_wasm_binary=true as LoadArgs.
Same for wasm_runtime_is_underlying_binary_freeable, it won't lift any implementation knowledge from the developer in my opinion. To do something like that, an API like

bool
wasm_module_free_wasm_binary_buffer(wasm_byte_vec_t *binary,
                                    wasm_module_t *module)
{
    if (!((wasm_module_ex_t *)module)->is_binary_cloned
        && !wasm_runtime_is_underlying_binary_freeable(module))
        return false;

    wasm_byte_vec_delete(binary);
    return true;
}

would be more useful. And something equivalent in the WAMR API.
But I'm fine with the suggestion since it won't make a huge difference for me.

when i suggested a query api, i meant to make it check all necessary conditions including is_binary_cloned, module type, etc. so it would be just:

if (wasm_runtime_is_underlying_binary_freeable(...)) {
    wasm_byte_vec_delete(binary);
    have_binary = false;
}

The wasm-c-api wasm_module_ex_t's is_binary_cloned and wasm/aot wasm_module_t's is_binary_freeable are two options and the behaviors are different, and they are stored in different structures, I think we cannot just provide one xxx_is_underly_binary_freeable like API for both of them, and also the input binary for wasm_module_new/wasm_module_new_ex/wasm_runtime_load/wasm_runtime_load_ex is provided by developer, it should not be freed by runtime or we had better not use a runtime API to free it. And the cloned binary of wasm-c-api (if the input binary is cloned) will be freed by runtime, developer should not free it also.

So, per my understanding, developer can use the xxx_is_underly_binary_freeable like API to check whether he can free the input binary and free it if yes, for wasm-c-api, his code is somewhat like:

    if (wasm_module_is_underlying_binary_cloned(module)) {  //or if (wasm_module_is_underlying_binary_freeable(module))
        wasm_byte_vec_delete(binary);
        wasm_runtime_free(binary);
    }

for wasm/aot loader, his code is somewhat like:

    if (wasm_runtime_is_underlying_binary_freeable(module)) {
        wasm_runtime_free(buf);
    }

@eloparco @yamt not sure whether that is ok for you? Or how about providing wasm_module_is_underlying_binary_freeable instead of wasm_module_is_underlying_binary_cloned for wasm-c-api, and wasm_runtime_is_underlying_binary_freeable for wasm/aot loader? Or is there other suggestion from you? Thanks.

[eloparco]

Or how about providing wasm_module_is_underlying_binary_freeable instead of wasm_module_is_underlying_binary_cloned for wasm-c-api, and wasm_runtime_is_underlying_binary_freeable for wasm/aot loader?

Do we need two separate APIs? One (i.e. wasm_runtime_is_underlying_binary_freeable, that checks wasm_binary_freeable) should be enough since we set args->wasm_binary_freeable = !args->clone_wasm_binary;

And wasm_runtime_is_underlying_binary_freeable would look like what I already added to this PR.

including is_binary_cloned, module type, etc.

It's not module type but rather execution mode. I wouldn't add a check on that but rather write in the documentation that, with fast interpreter and AOT, the module can be freed after loading; while, in classic interpreter mode, after instantiation.

[yamt]

I wouldn't add a check on that

why not?

[yamt]

while, in classic interpreter mode, after instantiation.

i believe you can't free the buffer for classic interpreter.

[wenyongh]

Or how about providing wasm_module_is_underlying_binary_freeable instead of wasm_module_is_underlying_binary_cloned for wasm-c-api, and wasm_runtime_is_underlying_binary_freeable for wasm/aot loader?

Do we need two separate APIs? One (i.e. wasm_runtime_is_underlying_binary_freeable, that checks wasm_binary_freeable) should be enough since we set args->wasm_binary_freeable = !args->clone_wasm_binary;

And wasm_runtime_is_underlying_binary_freeable would look like what I already added to this PR.

Yeah, I mean one API for wasm-c-api and one for wamr runtime api, and I found you had done that in the PR, it LGTM.

[wenyongh]

while, in classic interpreter mode, after instantiation.

i believe you can't free the buffer for classic interpreter.

Agree, how about we merge this PR first and then submit another PR to clone the wasm file's bytecode for classic interpreter mode to avoid referring to the underlying buffer?

[yamt]

while, in classic interpreter mode, after instantiation.

i believe you can't free the buffer for classic interpreter.

Agree, how about we merge this PR first and then submit another PR to clone the wasm file's bytecode for classic interpreter mode to avoid referring to the underlying buffer?

i'm not sure if it's a good idea.
i suspect usually code and data segments occupy the major part of wasm module.
if so, for classic interpreter, it might be simpler to do the opposite to what this PR does.
that is, keep references to the underlying buffer for data segments as well.

i feel it's simpler to make wasm_runtime_is_underlying_binary_freeable return false for now, until someone evaluates pros/cons.

[wenyongh]

while, in classic interpreter mode, after instantiation.

i believe you can't free the buffer for classic interpreter.

Agree, how about we merge this PR first and then submit another PR to clone the wasm file's bytecode for classic interpreter mode to avoid referring to the underlying buffer?

i'm not sure if it's a good idea. i suspect usually code and data segments occupy the major part of wasm module. if so, for classic interpreter, it might be simpler to do the opposite to what this PR does. that is, keep references to the underlying buffer for data segments as well.

i feel it's simpler to make wasm_runtime_is_underlying_binary_freeable return false for now, until someone evaluates pros/cons.

OK, cloning bytecode for classic interpreter may save the memory of data segments, but as you said, we can just let wasm_runtime_is_underlying_binary_freeable return false now.

[eloparco]

@yamt @wenyongh I applied the changes after the discussion, let me know if I missed anything

[wenyongh]

LGTM except two minor comments.

[wenyongh]

Seems we had better also set args.is_binary_freeable = false in wasm_runtime_load, see L1385, though LoadArgs args = { 0 }; already set it to false?

[eloparco]

Actually, while I was able to free the buffer after loading in the simple example in this PR, I see problems when doing it with a more complex example. During module instantiation, this part is causing problems

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

Line 2457 in 8f098a5

bh_memcpy_s(memory_data + base_offset,

Everything is fine if I free the buffer after module instantiation instead.

[EDIT]: Actually I can see the problem already with the sample in this example, it outputs: calling into WASM function: mul7, mul7 return %�0x15:i32 instead of

calling into WASM function: mul7,    mul7 return 21 
0x15:i32

when turning fast interpreter on.
So I guess we can't free the buffer after module loading but only after module instantiation. I think that's still fine, at least for mine and most uses cases, especially with long-running wasm apps.

Let's agree on that and I'll modify the PR accordingly. I think it makes sense to have this PR cover the case of free after instantiation and if we want to investigate if it's possible to free after loading we can do it separately.

[wenyongh]

Actually, while I was able to free the buffer after loading in the simple example in this PR, I see problems when doing it with a more complex example. During module instantiation, this part is causing problems

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

Line 2457 in 8f098a5

bh_memcpy_s(memory_data + base_offset,

Everything is fine if I free the buffer after module instantiation instead.
[EDIT]: Actually I can see the problem already with the sample in this example, it outputs: calling into WASM function: mul7, mul7 return %�0x15:i32 instead of

calling into WASM function: mul7,    mul7 return 21 
0x15:i32

when turning fast interpreter on. So I guess we can't free the buffer after module loading but only after module instantiation. I think that's still fine, at least for mine and most uses cases, especially with long-running wasm apps.

Let's agree on that and I'll modify the PR accordingly. I think it makes sense to have this PR cover the case of free after instantiation and if we want to investigate if it's possible to free after loading we can do it separately.

Hi, I checked the source code again, there are really several places in wasm/aot module which refer to the input buffer even when we set args.wasm_binary_freeable = true:

1. the data segments' data, or WASMDataSeg->data, see
   

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

   Line 4722 in 8f098a5

   dataseg->data = (uint8 *)p;

It may be used in wasm instantiation, that's why your case works if we free buffer after instantiation but doesn't work before it. But the issue is that the passive data segment may be also used in opcode memory.init:

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

Line 5595 in 8f098a5

data = module->module->data_segments[segment]->data;

And note that the issue only exists in interpreter mode since the aot loader will allocate new memory for the data segment:

wasm-micro-runtime/core/iwasm/aot/aot_loader.c

Lines 1002 to 1017 in 8f098a5

	read_uint32(buf, buf_end, byte_count);
	size = offsetof(AOTMemInitData, bytes) + (uint64)byte_count;
	if (!(data_list[i] = loader_malloc(size, error_buf, error_buf_size))) {
	return false;
	}
	#if WASM_ENABLE_BULK_MEMORY != 0
	/* is_passive and memory_index is only used in bulk memory mode */
	data_list[i]->is_passive = (bool)is_passive;
	data_list[i]->memory_index = memory_index;
	#endif
	data_list[i]->offset.init_expr_type = init_value.init_expr_type;
	data_list[i]->offset.u = init_value.u;
	data_list[i]->byte_count = byte_count;
	read_byte_array(buf, buf_end, data_list[i]->bytes,
	data_list[i]->byte_count);

2. module->string_literal_ptrs, used when both stringref and GC feature are enabled, and in both interpreter and aot:

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

Line 4946 in 8f098a5

module->string_literal_ptrs[i] = p;

wasm-micro-runtime/core/iwasm/aot/aot_loader.c

Line 868 in 8f098a5

module->string_literal_ptrs[i] = p;

3. module->custom_section_list, in both interpreter and aot:

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

Lines 5121 to 5124 in 8f098a5

	section->name_addr = (char *)p;
	section->name_len = name_len;
	section->content_addr = (uint8 *)(p + name_len);
	section->content_len = (uint32)(p_end - p - name_len);

wasm-micro-runtime/core/iwasm/aot/aot_loader.c

Lines 931 to 934 in 8f098a5

	section->name_addr = (char *)section_name;
	section->name_len = (uint32)strlen(section_name);
	section->content_addr = (uint8 *)p;
	section->content_len = (uint32)(p_end - p);

It is mainly used by wamrc and the API wasm_runtime_get_custom_section exported in wasm_export.h

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.

for 2, maybe we can check whether module->string_literal_ptrs is 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.

@yamt, @eloparco how do you think? Thanks.

[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.