Skip to content

Conversation

XeniaLu
Copy link
Contributor

@XeniaLu XeniaLu commented Jun 12, 2024

  • add declarations related to emitting AOT files into a separate header file named aot_emit_aot_file.h
  • extracting aot_emit_aot_file_buf_ex and expose through the aot_emit_aot_file.h

@wenyongh
Copy link
Contributor

@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
Copy link
Contributor Author

XeniaLu commented Jun 12, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor Author

XeniaLu commented Jun 14, 2024

@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
Copy link
Contributor Author

XeniaLu commented Jun 14, 2024

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.

@wenyongh
Copy link
Contributor

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
Copy link
Contributor Author

XeniaLu commented Jun 14, 2024

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!

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@TianlongLiang
Copy link
Collaborator

LGTM

@wenyongh wenyongh merged commit ad5d31b into bytecodealliance:main Jun 14, 2024
@XeniaLu XeniaLu deleted the add-emit-aot-file-header branch June 14, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants