Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
May 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5abaac0
feat(wasm-c-api): allow not copying the wasm binary into the module
eloparco May 3, 2024
c18c0d7
feat: add api to check if wasm binary buffer can be freed
eloparco May 10, 2024
af265e0
feat: add example of early free for wasm binary buffer
eloparco May 13, 2024
7e94c60
fix: put additional checks in wasm_runtime_is_underlying_binary_freeable
eloparco May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions core/iwasm/aot/aot_loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3835,6 +3835,7 @@ create_module(char *name, char *error_buf, uint32 error_buf_size)
module->module_type = Wasm_Module_AoT;

module->name = name;
module->is_binary_freeable = false;

#if WASM_ENABLE_MULTI_MODULE != 0
module->import_module_list = &module->import_module_list_head;
Expand Down Expand Up @@ -4065,8 +4066,8 @@ create_sections(AOTModule *module, const uint8 *buf, uint32 size,
}

static bool
load(const uint8 *buf, uint32 size, AOTModule *module, char *error_buf,
uint32 error_buf_size)
load(const uint8 *buf, uint32 size, AOTModule *module,
bool wasm_binary_freeable, char *error_buf, uint32 error_buf_size)
{
const uint8 *buf_end = buf + size;
const uint8 *p = buf, *p_end = buf_end;
Expand All @@ -4090,8 +4091,8 @@ load(const uint8 *buf, uint32 size, AOTModule *module, char *error_buf,
error_buf_size))
return false;

ret = load_from_sections(module, section_list, true, error_buf,
error_buf_size);
ret = load_from_sections(module, section_list, !wasm_binary_freeable,
error_buf, error_buf_size);
if (!ret) {
/* If load_from_sections() fails, then aot text is destroyed
in destroy_sections() */
Expand Down Expand Up @@ -4135,7 +4136,8 @@ aot_load_from_aot_file(const uint8 *buf, uint32 size, const LoadArgs *args,
return NULL;

os_thread_jit_write_protect_np(false); /* Make memory writable */
if (!load(buf, size, module, error_buf, error_buf_size)) {
if (!load(buf, size, module, args->wasm_binary_freeable, error_buf,
error_buf_size)) {
aot_unload(module);
return NULL;
}
Expand Down
3 changes: 3 additions & 0 deletions core/iwasm/aot/aot_runtime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,9 @@ typedef struct AOTModule {

/* user defined name */
char *name;

/* Whether the underlying wasm binary buffer can be freed */
bool is_binary_freeable;
} AOTModule;

#define AOTMemoryInstance WASMMemoryInstance
Expand Down
46 changes: 34 additions & 12 deletions core/iwasm/common/wasm_c_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
typedef struct wasm_module_ex_t {
struct WASMModuleCommon *module_comm_rt;
wasm_byte_vec_t *binary;
/* If true, binary in wasm_module_ex_t contains a copy of the WASM binary */
bool is_binary_cloned;
korp_mutex lock;
uint32 ref_count;
#if WASM_ENABLE_WASM_CACHE != 0
Expand Down Expand Up @@ -2234,8 +2236,7 @@ try_reuse_loaded_module(wasm_store_t *store, char *binary_hash)
#endif /* WASM_ENABLE_WASM_CACHE != 0 */

wasm_module_t *
wasm_module_new_ex(wasm_store_t *store, const wasm_byte_vec_t *binary,
const LoadArgs *args)
wasm_module_new_ex(wasm_store_t *store, wasm_byte_vec_t *binary, LoadArgs *args)
{
char error_buf[128] = { 0 };
wasm_module_ex_t *module_ex = NULL;
Expand Down Expand Up @@ -2283,14 +2284,21 @@ wasm_module_new_ex(wasm_store_t *store, const wasm_byte_vec_t *binary,
if (!module_ex)
goto quit;

module_ex->binary = malloc_internal(sizeof(wasm_byte_vec_t));
if (!module_ex->binary)
goto free_module;
module_ex->is_binary_cloned = args->clone_wasm_binary;
if (args->clone_wasm_binary) {
module_ex->binary = malloc_internal(sizeof(wasm_byte_vec_t));
if (!module_ex->binary)
goto free_module;

wasm_byte_vec_copy(module_ex->binary, binary);
if (!module_ex->binary->data)
goto free_binary;
wasm_byte_vec_copy(module_ex->binary, binary);
if (!module_ex->binary->data)
goto free_binary;
}
else {
module_ex->binary = binary;
}

args->wasm_binary_freeable = !args->clone_wasm_binary;
eloparco marked this conversation as resolved.
Show resolved Hide resolved
module_ex->module_comm_rt = wasm_runtime_load_ex(
eloparco marked this conversation as resolved.
Show resolved Hide resolved
(uint8 *)module_ex->binary->data, (uint32)module_ex->binary->size, args,
error_buf, (uint32)sizeof(error_buf));
Expand Down Expand Up @@ -2328,9 +2336,11 @@ wasm_module_new_ex(wasm_store_t *store, const wasm_byte_vec_t *binary,
unload:
wasm_runtime_unload(module_ex->module_comm_rt);
free_vec:
wasm_byte_vec_delete(module_ex->binary);
if (args->clone_wasm_binary)
wasm_byte_vec_delete(module_ex->binary);
free_binary:
wasm_runtime_free(module_ex->binary);
if (args->clone_wasm_binary)
wasm_runtime_free(module_ex->binary);
free_module:
wasm_runtime_free(module_ex);
quit:
Expand All @@ -2343,7 +2353,8 @@ wasm_module_new(wasm_store_t *store, const wasm_byte_vec_t *binary)
{
LoadArgs args = { 0 };
args.name = "";
return wasm_module_new_ex(store, binary, &args);
args.clone_wasm_binary = true;
return wasm_module_new_ex(store, (wasm_byte_vec_t *)binary, &args);
}

bool
Expand Down Expand Up @@ -2402,7 +2413,8 @@ wasm_module_delete_internal(wasm_module_t *module)
return;
}

DEINIT_VEC(module_ex->binary, wasm_byte_vec_delete);
if (module_ex->is_binary_cloned)
DEINIT_VEC(module_ex->binary, wasm_byte_vec_delete);

if (module_ex->module_comm_rt) {
wasm_runtime_unload(module_ex->module_comm_rt);
Expand Down Expand Up @@ -2986,6 +2998,16 @@ wasm_module_get_name(wasm_module_t *module)
return wasm_runtime_get_module_name(module_ex->module_comm_rt);
}

bool
wasm_module_is_underlying_binary_freeable(
const wasm_module_t *module, const struct wasm_instance_t *instance)
{
if (((wasm_module_ex_t *)module)->is_binary_cloned)
return true;

return wasm_runtime_is_underlying_binary_freeable(instance->inst_comm_rt);
}

static wasm_func_t *
wasm_func_new_basic(wasm_store_t *store, const wasm_functype_t *type,
wasm_func_callback_t func_callback)
Expand Down
57 changes: 57 additions & 0 deletions core/iwasm/common/wasm_runtime_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1350,12 +1350,18 @@ wasm_runtime_load_ex(uint8 *buf, uint32 size, const LoadArgs *args,
true,
#endif
args, error_buf, error_buf_size);
if (module_common)
((WASMModule *)module_common)->is_binary_freeable =
args->wasm_binary_freeable;
#endif
}
else if (get_package_type(buf, size) == Wasm_Module_AoT) {
#if WASM_ENABLE_AOT != 0
module_common = (WASMModuleCommon *)aot_load_from_aot_file(
buf, size, args, error_buf, error_buf_size);
if (module_common)
((AOTModule *)module_common)->is_binary_freeable =
args->wasm_binary_freeable;
#endif
}
else {
Expand Down Expand Up @@ -1383,6 +1389,7 @@ wasm_runtime_load(uint8 *buf, uint32 size, char *error_buf,
{
LoadArgs args = { 0 };
args.name = "";
args.wasm_binary_freeable = false;
return wasm_runtime_load_ex(buf, size, &args, error_buf, error_buf_size);
}

Expand All @@ -1400,6 +1407,7 @@ wasm_runtime_load_from_sections(WASMSection *section_list, bool is_aot,
LOG_DEBUG("WASM module load failed from sections");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return NULL;
}
((WASMModule *)module_common)->is_binary_freeable = true;
return register_module_with_null_name(module_common, error_buf,
error_buf_size);
#endif
Expand All @@ -1412,6 +1420,7 @@ wasm_runtime_load_from_sections(WASMSection *section_list, bool is_aot,
LOG_DEBUG("WASM module load failed from sections");
return NULL;
}
((AOTModule *)module_common)->is_binary_freeable = true;
return register_module_with_null_name(module_common, error_buf,
error_buf_size);
#endif
Expand Down Expand Up @@ -7170,3 +7179,51 @@ wasm_runtime_detect_native_stack_overflow_size(WASMExecEnv *exec_env,
}
return true;
}

WASM_RUNTIME_API_EXTERN bool
wasm_runtime_is_underlying_binary_freeable(const wasm_module_inst_t module_inst)
{
uint32 i;

#if WASM_ENABLE_INTERP != 0
eloparco marked this conversation as resolved.
Show resolved Hide resolved
if (module_inst->module_type == Wasm_Module_Bytecode) {
#if (WASM_ENABLE_JIT != 0 || WASM_ENABLE_FAST_JIT != 0) \
&& (WASM_ENABLE_LAZY_JIT != 0)
return false;
#elif WASM_ENABLE_FAST_INTERP == 0
return false;
#else
/* Fast interpreter mode */
WASMModule *module = ((WASMModuleInstance *)module_inst)->module;
if (!module->is_binary_freeable)
return false;
#if WASM_ENABLE_GC != 0 && WASM_ENABLE_STRINGREF != 0
if (module->string_literal_ptrs)
return false;
#endif
#if WASM_ENABLE_BULK_MEMORY != 0
for (i = 0; i < module->data_seg_count; i++)
if (!bh_bitmap_get_bit(
((WASMModuleInstance *)module_inst)->e->common.data_dropped,
i))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return false;
#endif
wenyongh marked this conversation as resolved.
Show resolved Hide resolved
#endif
}
#endif /* WASM_ENABLE_INTERP != 0 */
#if WASM_ENABLE_AOT != 0
if (module_inst->module_type == Wasm_Module_AoT) {
AOTModule *module =
(AOTModule *)((AOTModuleInstance *)module_inst)->module;
if (!module->is_binary_freeable)
return false;
#if WASM_ENABLE_GC != 0 && WASM_ENABLE_STRINGREF != 0
if (module->string_literal_ptrs)
return false;
#endif
}
#endif /* WASM_ENABLE_AOT != 0 */

(void)i;
return true;
}
wenyongh marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions core/iwasm/common/wasm_runtime_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,6 +1196,10 @@ WASM_RUNTIME_API_EXTERN bool
wasm_runtime_detect_native_stack_overflow_size(WASMExecEnv *exec_env,
uint32 requested_size);

WASM_RUNTIME_API_EXTERN bool
wasm_runtime_is_underlying_binary_freeable(
const wasm_module_inst_t module_inst);

#if WASM_ENABLE_LINUX_PERF != 0
bool
wasm_runtime_get_linux_perf(void);
Expand Down
10 changes: 9 additions & 1 deletion core/iwasm/include/wasm_c_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,12 @@ typedef struct WASMModuleCommon *wasm_module_t;
#define LOAD_ARGS_OPTION_DEFINED
typedef struct LoadArgs {
char *name;
/* True by default, used by wasm-c-api only.
If false, the wasm input buffer (wasm_byte_vec_t) is referenced by the
module instead of being cloned. Hence, it can be freed after module loading. */
bool clone_wasm_binary;
/* This option is only used by the AOT/wasm loader (see wasm_export.h) */
bool wasm_binary_freeable;
eloparco marked this conversation as resolved.
Show resolved Hide resolved
/* TODO: more fields? */
} LoadArgs;
#endif /* LOAD_ARGS_OPTION_DEFINED */
Expand All @@ -536,7 +542,7 @@ WASM_API_EXTERN own wasm_module_t* wasm_module_new(

// please refer to wasm_runtime_load_ex(...) in core/iwasm/include/wasm_export.h
WASM_API_EXTERN own wasm_module_t* wasm_module_new_ex(
wasm_store_t*, const wasm_byte_vec_t* binary, const LoadArgs *args);
wasm_store_t*, wasm_byte_vec_t* binary, LoadArgs *args);

WASM_API_EXTERN void wasm_module_delete(own wasm_module_t*);

Expand All @@ -556,6 +562,8 @@ WASM_API_EXTERN void wasm_shared_module_delete(own wasm_shared_module_t*);
WASM_API_EXTERN bool wasm_module_set_name(wasm_module_t*, const char* name);
WASM_API_EXTERN const char *wasm_module_get_name(wasm_module_t*);

WASM_API_EXTERN bool wasm_module_is_underlying_binary_freeable(const wasm_module_t *module, const struct wasm_instance_t* instance);


// Function Instances

Expand Down
17 changes: 17 additions & 0 deletions core/iwasm/include/wasm_export.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,13 @@ typedef struct RuntimeInitArgs {
#define LOAD_ARGS_OPTION_DEFINED
typedef struct LoadArgs {
char *name;
/* This option is only used by the Wasm C API (see wasm_c_api.h) */
bool clone_wasm_binary;
/* False by default, used by AOT/wasm loader only.
eloparco marked this conversation as resolved.
Show resolved Hide resolved
If true, the AOT/wasm loader creates a copy of some module fields (e.g.
const strings), making it possible to free the wasm binary buffer after
loading. */
bool wasm_binary_freeable;
/* TODO: more fields? */
} LoadArgs;
#endif /* LOAD_ARGS_OPTION_DEFINED */
Expand Down Expand Up @@ -1860,6 +1867,16 @@ WASM_RUNTIME_API_EXTERN bool
wasm_runtime_detect_native_stack_overflow_size(wasm_exec_env_t exec_env,
uint32_t required_size);

/**
* Query whether the wasm binary buffer used to create the module can be freed
*
* @param module_inst the target module instance
* @return true if the wasm binary buffer can be freed
*/
WASM_RUNTIME_API_EXTERN bool
wasm_runtime_is_underlying_binary_freeable(
const wasm_module_inst_t module_inst);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#ifdef __cplusplus
}
#endif
Expand Down
3 changes: 3 additions & 0 deletions core/iwasm/interpreter/wasm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,6 +1071,9 @@ struct WASMModule {

/* user defined name */
char *name;

/* Whether the underlying wasm binary buffer can be freed */
bool is_binary_freeable;
};

typedef struct BlockType {
Expand Down
12 changes: 7 additions & 5 deletions core/iwasm/interpreter/wasm_loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6149,6 +6149,7 @@ create_module(char *name, char *error_buf, uint32 error_buf_size)
module->start_function = (uint32)-1;

module->name = name;
module->is_binary_freeable = false;

#if WASM_ENABLE_FAST_INTERP == 0
module->br_table_cache_list = &module->br_table_cache_list_head;
Expand Down Expand Up @@ -6376,8 +6377,8 @@ static union {
#define is_little_endian() (__ue.b == 1)

static bool
load(const uint8 *buf, uint32 size, WASMModule *module, char *error_buf,
uint32 error_buf_size)
load(const uint8 *buf, uint32 size, WASMModule *module,
bool wasm_binary_freeable, char *error_buf, uint32 error_buf_size)
{
const uint8 *buf_end = buf + size;
const uint8 *p = buf, *p_end = buf_end;
Expand Down Expand Up @@ -6405,8 +6406,8 @@ load(const uint8 *buf, uint32 size, WASMModule *module, char *error_buf,
}

if (!create_sections(buf, size, &section_list, error_buf, error_buf_size)
|| !load_from_sections(module, section_list, true, error_buf,
error_buf_size)) {
|| !load_from_sections(module, section_list, !wasm_binary_freeable,
error_buf, error_buf_size)) {
destroy_sections(section_list);
return false;
}
Expand Down Expand Up @@ -6582,7 +6583,8 @@ wasm_loader_load(uint8 *buf, uint32 size,
module->load_size = size;
#endif

if (!load(buf, size, module, error_buf, error_buf_size)) {
if (!load(buf, size, module, args->wasm_binary_freeable, error_buf,
error_buf_size)) {
goto fail;
}

Expand Down
Loading
Loading