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

Shared memory should only be shared inside cluster #1962

Closed
xujuntwt95329 opened this issue Feb 15, 2023 · 1 comment · Fixed by #2415
Closed

Shared memory should only be shared inside cluster #1962

xujuntwt95329 opened this issue Feb 15, 2023 · 1 comment · Fixed by #2415

Comments

@xujuntwt95329
Copy link
Collaborator

In current implementation, the shared memory is shared globally, instances from different clusters shares the same memory, which is not expected behavior.

We should only share the memory inside cluster, and when new clusters are created, there should be new memory instances.

The problem is: the cluster is created during creating exec_env rather than module instance, so during instantiating there may be no cluster information.

@xujuntwt95329
Copy link
Collaborator Author

Possible solution:

Data structures:

  • In WASMSharedMemNode, add one field: main_inst_wp, weak pointer to main instance
  • In WASMCluster, add one field: shared_memory_wp, weak pointer to the shared MemoryInstance

Logic:

  • When creating shared memory instance, store the pointer of main instance in main_inst_wp field
  • When creating new cluster, if the module's memory is shared, search the WASMSharedMemNode according to the module and main_instance, and set the shared MemoryInstance to shared_memory_wp field
  • Change is_sub_inst parameter of wasm_runtime_instantiate_internal to WASMCluster *cluster
    • If cluster is NULL, always create new memory instance rather than search the shared memory list.
    • If cluster is not NULL and module's memory is shared, use the cluster->shared_memory_wp directly

yamt added a commit to yamt/wasm-micro-runtime that referenced this issue Aug 2, 2023
- inherit shared memory from the parent instance, instead of
  trying to look it up by the underlying module. the old method
  works correctly only when every clusters uses different modules.

- reference count WASMMemoryInstance/AOTMemoryInstance directly

- retire WASMSharedMemNode

- for atomic opcode implementations in the interpreters, use
  a global lock for now.

- update the internal API users.
  (wasi-threads, lib-pthread, wasm_runtime_spawn_thread)

Fixes bytecodealliance#1962
wenyongh pushed a commit that referenced this issue Aug 4, 2023
- Inherit shared memory from the parent instance, instead of
  trying to look it up by the underlying module. The old method
  works correctly only when every cluster uses different module.
- Use reference count in WASMMemoryInstance/AOTMemoryInstance
  to mark whether the memory is shared or not
- Retire WASMSharedMemNode
- For atomic opcode implementations in the interpreters, use
  a global lock for now
- Update the internal API users
  (wasi-threads, lib-pthread, wasm_runtime_spawn_thread)

Fixes #1962
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this issue May 27, 2024
- Inherit shared memory from the parent instance, instead of
  trying to look it up by the underlying module. The old method
  works correctly only when every cluster uses different module.
- Use reference count in WASMMemoryInstance/AOTMemoryInstance
  to mark whether the memory is shared or not
- Retire WASMSharedMemNode
- For atomic opcode implementations in the interpreters, use
  a global lock for now
- Update the internal API users
  (wasi-threads, lib-pthread, wasm_runtime_spawn_thread)

Fixes bytecodealliance#1962
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 a pull request may close this issue.

1 participant