[Proposal] Implement WASI-threads

[LFsWang]

Todo

It is required to implement an SharedMemory in MemoryInstance, and

1. resolve the race condition issue.
2. resolve the memory base pointer issue during grow page.

[juntao]

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.

---

This Pull Request titled "[Proposal] Implement WASI-threads" is an attempt by the contributor to significantly improve the thread handling in the WebAssembly System Interface (WASI). The changes across various files and addition of new methods relate primarily to the spawning, managing, and handling threads, as well as introducing a shared memory model in place of separate memory space per thread.

The most significant potential issues span across several key areas:

1. Thread Safety: The proposal to replace the MemoryInstance with the SharedMemory class, among other changes, introduces potential thread-safety risks. The concurrent access and synchronization will require meticulous management and extensive testing.

2. Error handling and Logging: Both the proposed changes and the new methods lack comprehensive error handling and descriptive logging. This would make debugging quite challenging. Specifically, the handling of a module name conflict and thread creation failure are noted areas of concern.

3. Memory Management: There is repeated usage of raw pointers in the changes, rather than the more robust smart pointers. This could potentially result in memory management issues, including memory leaks.

4. Compatibility: The significant changes to core functionalities like module instantiation/registration and memory handling may introduce compatibility issues and cause disruptions in the existing dependent codebase.

In terms of the critical findings, the updates include:

1. The introduction of a WasiThreadsEnvironment class for managing the thread environment and a ThreadManager class to manage thread ID allocation.

2. The creation of the SharedMemoryInstance class for implementing a shareable version of the previous MemoryInstance class, and an attempt at thread-safe procedures using shared mutexes.

3. The inclusion of methods ThreadStartArg and WasiThreadSpawn for spawning and managing new threads, and an updated wasiThreadSpawn implementation.

4. Annotation of TODO comments, notably regarding the management of module name conflicts, suggesting ongoing areas of work.

Despite these substantial changes, the proposed patches lack in several best practices:

1. Code Quality: The coding practices and naming conventions for variables and functions could be improved. The deep nesting of code and usage of the deprecated 'new' operator hinders readability and long-term maintainability.

2. Testing: The sizable functional additions do not correspond with updates to the testing framework, risking neglected edge cases and potential impacts on system stability.

3. Documentation: The PR does not accompany any notable updates to documentation, which considering the scale of the changes introduced, would be necessary for the benefit of future developers and users.

In conclusion, the changes proposed are comprehensive but they evidently lack in crucial areas like error handling, thread safety, memory management, and test coverage. These areas must be addressed to ensure the reliability, efficiency, and maintainability of the updated software.

Details

Commit d340e63b6cbeec1330c116f70159b6697b935917

This patch, submitted by Sylveon from secondstate.io, primarily aims to handle the import of shared memory for WASI threads. This change affects two files mainly:

1. include/runtime/storemgr.h (Store Manager header)
2. lib/executor/instantiate/import.cpp (Library Executor for instantiation)

Key Changes:

1. Attempting to add a function addMemoryInModule but it's commented out in the StoreManager class located in storemgr.h. This function, as per the function name, seems intended to add memory instances in a specified module. Use of mutex suggests that the function is designed to be thread-safe.

2. In the import.cpp file, code was added within the Executor::instantiate function. This code is designed to handle the cases where shared memory referenced in the WASI Threads Proposal is being imported. A block of code finds the first shared memory and creates it if it does not exist, particularly in an environment module env.

Potential Issues:

1. The added function addMemoryInModule in storemgr.h is entirely commented out and contains a TODO comment about handling module name conflicts.

2. Reference made to an ErrCode::Value::ModuleNameConflict without any code handling it, this could lead to a module name conflict that is not properly mitigated.

3. Descriptive error logging is lacking in the implemented check for shared memory in 'import.cpp'. The current logging with "isShared" and ExtName does not provide much context.

4. The new memory instance (within import.cpp) in the environment module uses the deprecated 'new' operator instead of smart pointers, this may lead to potential memory leaks.

5. No validation check or error handling is done if StoreMgr.registerModule operation fails. If the registration of the new module fails for some reason, it is currently not handled.

6. Code quality could be improved by following best practices for naming variables and functions, and structuring the code for readability. Factors like deep nesting should be avoided for readability and maintenance purposes.

Commit cffc39d6b58f9182af52ca56ac2dd91bc1833e1c

This patch primarily implements functionality for WebAssembly System Interface (WASI) threads, enabling multi-threading within WASI. It is a significant addition to the software, impacting multiple files, involved in creating, handling, and supporting threads. It comprises of 454 new lines of code and includes the creation of several new files, mostly related to thread environment configuration and management.

Key Changes:

• Introduced WasiThreadsEnvironment class for managing thread environment.
• The ThreadStartArg and wasiThreadSpawn methods are implemented, which helps in creating and managing new threads.
• WasiThreadSpawn in the body method checks if an entry point for WASI exists. If it does, it spawns a new thread.
• Added SharedMemoryInstance class that is a shared version of the MemoryInstance class and utilizes shared mutexes for thread safety.
• A ThreadManager class is added to manage thread ID allocation.
• The new file threadenv.cpp implements thread spawning.
• In module.h, a Clone method is added to duplicate ModuleInstances.
• Added a new mock implementation for testing the WASI threads functionality.

Potential Issues:

• This change allows tight integration with OS-level threads which could potentially lead to performance degradation if not properly managed as threads can introduce overhead including context switching, synchronization, etc.
• There is no explicit error handling or logging if thread creation fails, which could make debugging difficult.
• It includes direct calls to exit() which will terminate the whole program on certain thread-related errors.
• The patch doesn't mention any changes in the testing framework to accommodate this major functional addition, which might undermine the stability of the system.
• Thread-safety isn't well addressed throughout the changes, particularly with data structures that may be accessed simultaneously by multiple threads.
• The Clone method in Runtime::Instance::Module could be problematic if ModuleInstances are not designed to handle deep-copy semantics appropriately.

Commit 5639b65a9cdd5e811d35695422272241c2dc6f24

In this Pull Request, the contributor has tried to improve thread handling in the WebAssembly System Interface (WASI) by creating more robust mechanisms for spawning and handling threads. Major changes introduced with this PR include:

1. Addition of methods related to WASM module instantiation and module registration in the Executor class.

2. Changes to the handling of memory instances, replacing the MemoryInstance implementation with a SharedMemory class. This suggests a move from separate memory space per thread to a shared memory model.

3. The ModuleInstance class's clone method is replaced with the CloneMemoryInEnv.

4. The VM class has new methods like unsafeGetRegisterHostsCallback.

5. WasiThreadsEnvironment::wasiThreadSpawn implementation modified with a new parameter of std::unique_ptr<Runtime::Instance::ModuleInstance> EnvMod.

6. SharedMemoryInstance replaced with SharedMemory.

Potential issues or concerns:

1. Thread Safety: The introduction of the shared memory model and changes made to mutexes may lead to potential thread-safety risks. Synchronization and concurrent access need to be carefully managed and extensively tested.

2. Memory Management: The usage of raw pointers (new and delete) instead of smart pointers in several places can cause memory management issues and leaks.

3. Compatibility and Dependability: As the contributor merges multiple things and modifies some core functionalities (like the module instantiation/registration and memory handling), these changes may break the existing dependent codebase.

4. Unit Testing: The addition of complex functionalities (like threading) requires substantial unit testing to ensure that all the scenarios (including edge cases) are correctly handled.

5. Documentation: The changes introduced in this pull request are quite substantial and would require detailed documentation updates for the developers and users of the library.

[hydai]

Hi, Please refer to the latest master branch. We don't need to add these link options now, instead, there are some changes in the following cmakelists.txt file.