Skip to content

Enable aux stack allocations on application heap#1799

Merged
wenyongh merged 3 commits intobytecodealliance:dev/wasi_threadsfrom
loganek:loganek/heap-aux-stack
Dec 19, 2022
Merged

Enable aux stack allocations on application heap#1799
wenyongh merged 3 commits intobytecodealliance:dev/wasi_threadsfrom
loganek:loganek/heap-aux-stack

Conversation

@loganek
Copy link
Copy Markdown
Contributor

@loganek loganek commented Dec 8, 2022

This is necessary for WASI threads as the aux stack should be managed by the application. See #1790 for details.

@loganek loganek mentioned this pull request Dec 8, 2022
19 tasks
@loganek loganek force-pushed the loganek/heap-aux-stack branch from 4004fee to b2cb12b Compare December 8, 2022 17:10

/* routine exit, destroy instance */
wasm_runtime_deinstantiate_internal(module_inst, true);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how is this related to the rest of the PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If WASM_ENABLE_HEAP_AUX_STACK_ALLOCATION is enabled, module instance can't be deinstantiate untill the memory allocated for the aux stack is released.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • with this change, where will it be deinstantiated instead?
  • how about the other wasm_runtime_deinstantiate_internal call in this function? (L507)

Copy link
Copy Markdown
Contributor Author

@loganek loganek Dec 14, 2022

Choose a reason for hiding this comment

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

with this change, where will it be deinstantiated instead?

It's deinstantiated in the thread_manager_start_routine()

how about the other wasm_runtime_deinstantiate_internal call in this function? (L507)

Yes, this one, and one in the lib_wasi_threads_wrapper.c have to be removed. Already updated the code, thanks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do you mean already updated locally but not pushed yet?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes sorry, didn't push my change. Should be updated now.

Comment thread core/iwasm/libraries/thread-mgr/thread_manager.c
The refactoring is necessary for further steps where we use module
instance of the exec env environment to perform on-heap aux stack
allocations.
This enables us to have fully application-managed aux stack. The change
is needed for wasi threads where aux stack is managed by Libc.
@loganek loganek force-pushed the loganek/heap-aux-stack branch from b2cb12b to 762b501 Compare December 15, 2022 13:46
Comment thread core/iwasm/interpreter/wasm_runtime.c Outdated
Comment thread core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads.cmake Outdated
Comment thread core/iwasm/libraries/thread-mgr/thread_manager.c Outdated
Comment thread core/iwasm/libraries/thread-mgr/thread_manager.c Outdated
So it can be used in thread manager even if wasi threads aren't enabled.
@loganek loganek force-pushed the loganek/heap-aux-stack branch from 762b501 to c344759 Compare December 18, 2022 23:48
@wenyongh wenyongh merged commit 0f637cf into bytecodealliance:dev/wasi_threads Dec 19, 2022
@yamt yamt mentioned this pull request Dec 21, 2022
vickiegpt pushed a commit to vickiegpt/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
This is necessary for WASI threads as the aux stack should be managed by the application.
See bytecodealliance#1790 for details.
@loganek loganek deleted the loganek/heap-aux-stack branch June 10, 2024 12:48
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