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.

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

API and data structure refactor #3218

Merged

Conversation

TianlongLiang
Copy link
Contributor

@TianlongLiang TianlongLiang commented Mar 12, 2024

  • remove parameter 'signature' from wasm_runtime_lookup_function
  • move common data members c_api_func_imports and cur_exec_env from WASMModuleInstanceExtraCommon to WASMModuleInstance
  • In WASMModuleInstance enlarge reserved[3] to reserved[5] in case add more data member in the future

@TianlongLiang
Copy link
Contributor Author

Hi, @yamt, I did some further refactoring according to the first bulletin mentioned in this issue #2530. Could you please take a look at this PR at your convenience? By the way, I tested the performance on the 32-bit platform after refactoring. On benchmarks, including CoreMark, Dhrystone, and JetStream, there was basically no performance impact for JetStream, and the performance was roughly down by 1% for CoreMark and 2% for Dhrystone. I think generally speaking, we don't have to worry about the performance impact on the 32-bit platform

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 TianlongLiang changed the base branch from dev/refactor_for_memory64 to main March 12, 2024 04:42
…ve common data member from WASMModuleInstanceExtraCommon to WASMModuleInstance
@wenyongh wenyongh merged commit c3e33a9 into bytecodealliance:main Mar 13, 2024
406 checks passed
@wenyongh
Copy link
Contributor

@yamt, I merged this PR (and also PR #3209) since we are to implement the memory64 feature and hope to refactor the related APIs and data structures before that, and also to make the related update for wamr-rust-sdk, Python language binding and Go language binding. @TianlongLiang have done a lot of test on the two PRs and also the code has been carefully reviewed. Please let us know if there are issues found. Many thanks.

@yamt
Copy link
Collaborator

yamt commented Mar 13, 2024

this patch looks reasonable at a glance. but i have no time to make careful review this month.
please go ahead w/o waiting for me.

i guess ideally we can stop aot relying on known sizeof(WASMModuleInstance) so that all these
"extra" stuff can be merged into the "base" structure. how do you think?

@wenyongh
Copy link
Contributor

Yes, it may be another way that AOT doesn't rely on sizeof(WASMModuleInstance), for example, wamrc puts the field offsets of WASMModuleInstance inside the AOT file and lets aot code read these fields according to the offsets. The issue is that it impacts the performance since an extra load instruction is added when accessing these fields, my preference is that performance is more important and we can reserve some spaces in WASMModuleInstance for future use, in fact there is a field uint32 reserved[5] in it, and we just found that field used_to_be_wasi_ctx is unused and can be merged to the reserved filed, so now the reserved space is 28 bytes: uint32 reserved[7]. Normally it should be enough, do you think we should enlarge it?

BTW, we found the global shared memory lock issue you mentioned in #3209, and plan to add a field in WASMMemoryInstance, korp_mutex *memory_lock. Is it good to you?

@TianlongLiang TianlongLiang deleted the dev/refactor_for_memory64 branch March 27, 2024 09:17
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
…ASMModuleInstance (bytecodealliance#3218)

Remove the unused parameter `signature` from `wasm_runtime_lookup_function`.

Refactor the layout of WASMModuleInstance structure:
- move common data members `c_api_func_imports` and `cur_exec_env` from
  `WASMModuleInstanceExtraCommon` to `WASMModuleInstance`
- In `WASMModuleInstance`, enlarge `reserved[3]` to `reserved[5]` in case that
  we need to add more fields in the future

ps.
bytecodealliance#2530
bytecodealliance#3202
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.

None yet

3 participants