bytecodealliance / wasm-micro-runtime

WebAssembly Micro Runtime (WAMR)
Apache License 2.0
4.66k stars 576 forks source link

Add aot_emit_aot_file.h to expose functions related to emitting AOT files #3520

Closed XeniaLu closed 3 weeks ago

XeniaLu commented 3 weeks ago
wenyongh commented 3 weeks ago

@XeniaLu The aot compiler related APIs have been exported in core/iwasm/include/aot_export.h, could you make changes in it instead? BTW, why need to expose the internal structure AOTObjectData and add new API aot_emit_aot_file_buf_ex?

https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/include/aot_export.h

XeniaLu commented 3 weeks ago

@XeniaLu The aot compiler related APIs have been exported in core/iwasm/include/aot_export.h, could you make changes in it instead? BTW, why need to expose the internal structure AOTObjectData and add new API aot_emit_aot_file_buf_ex?

https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/include/aot_export.h

We are developing a PostgreSQL extension to compile WASM using AOT compiler. Due to PostgreSQL's requirement of allocating an additional 4 bytes, we prefer to allocate memory ourselves and then directly call aot_emit_aot_file_buf_ex. This enables us to achieve zero-copy integration with the AOT compiler.

I understand the benefits of not exposing the internal structure AOTObjectData. And we intentionally avoided adding these APIs to aot_export.h because we don't want them to be publicly exposed. Instead, we include them in aot_emit_aot_file.h file that is not to be commonly imported by normal users. Do you have any suggestions or recommendations for achieving this without exposing it?

wenyongh commented 3 weeks ago

@XeniaLu The aot compiler related APIs have been exported in core/iwasm/include/aot_export.h, could you make changes in it instead? BTW, why need to expose the internal structure AOTObjectData and add new API aot_emit_aot_file_buf_ex? https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/include/aot_export.h

We are developing a PostgreSQL extension to compile WASM using AOT compiler. Due to PostgreSQL's requirement of allocating an additional 4 bytes, we prefer to allocate memory ourselves and then directly call aot_emit_aot_file_buf_ex. This enables us to achieve zero-copy integration with the AOT compiler.

I understand the benefits of not exposing the internal structure AOTObjectData. And we intentionally avoided adding these APIs to aot_export.h because we don't want them to be publicly exposed. Instead, we include them in aot_emit_aot_file.h file that is not to be commonly imported by normal users. Do you have any suggestions or recommendations for achieving this without exposing it?

Do you mean to allocate memory for aot_file_buf with extra 4 bytes and then call aot_emit_aot_file_buf_ex? It seems that we don't need to expose structures in aot_emit_aot_file.h, but just add typedef struct AOTObjectData AOTObjectData in aot_emit_aot_file.h?

wenyongh commented 3 weeks ago

IIUC, we can make changes in core/iwasm/include/aot_export.h, somewhat like adding the following codes:

struct AOTObjectData;
typedef struct AOTObjectData *aot_obj_data_t;

aot_obj_data_t
aot_obj_data_create(aot_comp_context_t comp_ctx);

void
aot_obj_data_destroy(aot_obj_data_t obj_data);

uint32_t
aot_get_aot_file_size(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                  aot_obj_data_t obj_data);

uint8 *
aot_emit_aot_file_buf(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                      uint32_t *p_aot_file_size);

bool
aot_emit_aot_file_buf_ex(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                         aot_obj_data_t obj_data, uint8_t *aot_file_buf,
                         uint32_t aot_file_size);
XeniaLu commented 3 weeks ago

@XeniaLu The aot compiler related APIs have been exported in core/iwasm/include/aot_export.h, could you make changes in it instead? BTW, why need to expose the internal structure AOTObjectData and add new API aot_emit_aot_file_buf_ex? https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/include/aot_export.h

We are developing a PostgreSQL extension to compile WASM using AOT compiler. Due to PostgreSQL's requirement of allocating an additional 4 bytes, we prefer to allocate memory ourselves and then directly call aot_emit_aot_file_buf_ex. This enables us to achieve zero-copy integration with the AOT compiler. I understand the benefits of not exposing the internal structure AOTObjectData. And we intentionally avoided adding these APIs to aot_export.h because we don't want them to be publicly exposed. Instead, we include them in aot_emit_aot_file.h file that is not to be commonly imported by normal users. Do you have any suggestions or recommendations for achieving this without exposing it?

Do you mean to allocate memory for aot_file_buf with extra 4 bytes and then call aot_emit_aot_file_buf_ex? It seems that we don't need to expose structures in aot_emit_aot_file.h, but just add typedef struct AOTObjectData AOTObjectData in aot_emit_aot_file.h?

You are right, we only need to add a typedef in aot_emit_aot.file.h.

XeniaLu commented 3 weeks ago

IIUC, we can make changes in core/iwasm/include/aot_export.h, somewhat like adding the following codes:

struct AOTObjectData;
typedef struct AOTObjectData *aot_obj_data_t;

aot_obj_data_t
aot_obj_data_create(aot_comp_context_t comp_ctx);

void
aot_obj_data_destroy(aot_obj_data_t obj_data);

uint32_t
aot_get_aot_file_size(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                  aot_obj_data_t obj_data);

uint8 *
aot_emit_aot_file_buf(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                      uint32_t *p_aot_file_size);

bool
aot_emit_aot_file_buf_ex(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                         aot_obj_data_t obj_data, uint8_t *aot_file_buf,
                         uint32_t aot_file_size);

Following your suggestion, I have submitted a separate commit to add the above codes to aot_export.h in https://github.com/bytecodealliance/wasm-micro-runtime/pull/3520/commits/ca2e491426b9542acd7621f3f81ccda6bb4940b3. However, this somewhat increases the exposure of functions. Please review and let me know if this is acceptable.

wenyongh commented 3 weeks ago

IIUC, we can make changes in core/iwasm/include/aot_export.h, somewhat like adding the following codes:

struct AOTObjectData;
typedef struct AOTObjectData *aot_obj_data_t;

aot_obj_data_t
aot_obj_data_create(aot_comp_context_t comp_ctx);

void
aot_obj_data_destroy(aot_obj_data_t obj_data);

uint32_t
aot_get_aot_file_size(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                  aot_obj_data_t obj_data);

uint8 *
aot_emit_aot_file_buf(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                      uint32_t *p_aot_file_size);

bool
aot_emit_aot_file_buf_ex(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                         aot_obj_data_t obj_data, uint8_t *aot_file_buf,
                         uint32_t aot_file_size);

Following your suggestion, I have submitted a separate commit to add the above codes to aot_export.h in ca2e491. However, this somewhat increases the exposure of functions. Please review and let me know if this is acceptable.

@XeniaLu Exposing some APIs is good to me and I added several minor comments. If you don't want to expose AOTObjectData pointer and the related APIs, may we can just provide a memory allocator callback for aot_emit_file_buf, like:

typedef void *(*aot_file_buf_alloc_func)(uint32_t size);

uint8 *
aot_emit_aot_file_buf(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                          aot_file_buf_alloc_func alloc_func,
                          uint32_t *p_aot_file_size);

And also refactor the current code of aot_emit_aot_file_buf, e.g. when alloc_func is NULL, we use wasm_runtime_malloc to allocate the aot file buffer.

XeniaLu commented 3 weeks ago

IIUC, we can make changes in core/iwasm/include/aot_export.h, somewhat like adding the following codes:

struct AOTObjectData;
typedef struct AOTObjectData *aot_obj_data_t;

aot_obj_data_t
aot_obj_data_create(aot_comp_context_t comp_ctx);

void
aot_obj_data_destroy(aot_obj_data_t obj_data);

uint32_t
aot_get_aot_file_size(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                  aot_obj_data_t obj_data);

uint8 *
aot_emit_aot_file_buf(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                      uint32_t *p_aot_file_size);

bool
aot_emit_aot_file_buf_ex(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                         aot_obj_data_t obj_data, uint8_t *aot_file_buf,
                         uint32_t aot_file_size);

Following your suggestion, I have submitted a separate commit to add the above codes to aot_export.h in ca2e491. However, this somewhat increases the exposure of functions. Please review and let me know if this is acceptable.

@XeniaLu Exposing some APIs is good to me and I added several minor comments. If you don't want to expose AOTObjectData pointer and the related APIs, may we can just provide a memory allocator callback for aot_emit_file_buf, like:

typedef void *(*aot_file_buf_alloc_func)(uint32_t size);

uint8 *
aot_emit_aot_file_buf(aot_comp_context_t comp_ctx, aot_comp_data_t comp_data,
                          aot_file_buf_alloc_func alloc_func,
                          uint32_t *p_aot_file_size);

And also refactor the current code of aot_emit_aot_file_buf, e.g. when alloc_func is NULL, we use wasm_runtime_malloc to allocate the aot file buffer.

Got it, I kept the current approach with exposing the APIs.

Thanks for your review!

TianlongLiang commented 3 weeks ago

LGTM