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

[Common] Move async class to common and support in executor. #2428

Merged
merged 4 commits into from
Jun 16, 2023

Conversation

q82419
Copy link
Collaborator

@q82419 q82419 commented Apr 23, 2023

This is for WasmEdge 0.13.0.

@codecov
Copy link

codecov bot commented Apr 23, 2023

Codecov Report

Merging #2428 (d789f6e) into master (9978a74) will decrease coverage by 0.02%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master    #2428      +/-   ##
==========================================
- Coverage   81.10%   81.08%   -0.02%     
==========================================
  Files         142      142              
  Lines       21341    21351      +10     
  Branches     4260     4264       +4     
==========================================
+ Hits        17309    17313       +4     
- Misses       2874     2877       +3     
- Partials     1158     1161       +3     
Impacted Files Coverage Δ
include/executor/executor.h 90.90% <ø> (ø)
include/vm/vm.h 80.24% <ø> (ø)
include/common/async.h 96.55% <80.00%> (ø)
lib/api/wasmedge.cpp 95.19% <100.00%> (+0.02%) ⬆️
lib/executor/executor.cpp 93.61% <100.00%> (+1.11%) ⬆️
lib/vm/vm.cpp 85.53% <100.00%> (-0.25%) ⬇️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

include/common/async.h Outdated Show resolved Hide resolved
@WasmEdge WasmEdge deleted a comment from alabulei1 Apr 27, 2023
@juntao
Copy link
Member

juntao commented Apr 27, 2023

flows review

Copy link
Member

juntao commented Apr 27, 2023

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


This Pull Request focuses on moving the Async class to a common location and adding support for it in the executor. The following are the significant changes and potential problems found in the PR:

Key Changes:

  1. The Async class is moved to the WasmEdge namespace and renamed to common/async.h.
  2. The Async class is now more flexible by accepting any class instance and a pointer to a member function as template arguments.
  3. The Async class features a StopFunc member variable to cancel the thread when necessary.
  4. The Executor class has been extended with an asyncInvoke member function, using the updated Async class.
  5. A new asynchronous invoke API WasmEdge_ExecutorAsyncInvoke has been exposed for developers to call WebAssembly functions asynchronously.
  6. Generalized AsyncWasm variable is added in several test files, replacing hardcoded bytecodes with the same set of bytecodes.

Potential Problems:

  1. The handling of null pointers and error cases for function instances might be insufficient in the new implementation.
  2. Proper initialization of the StopFunc member variable might be lacking when creating an Async object with the default constructor.
  3. Limited flexibility due to the absence of a way to replace the StopFunc with another function in the Async class.
  4. Changes in function signatures, such as Executor::invoke, which might lead to unexpected behavior.
  5. Insufficient error handling and potential concurrency and thread safety issues in the implementation of WasmEdge_ExecutorAsyncInvoke.
  6. The lack of unit tests to ensure the correctness of the new asynchronous API.
  7. Direct changes in test files, leading to potential issues with other tests that rely on previous hardcoded values.

It is essential to address these potential problems and thoroughly test the new code and functionality to ensure its correctness as well as compatibility with existing code.

Details

Commit 22e649ea69c5116213b6839c0f5a67a1ecfc6baf

Summary of key changes:

  1. The Async class has been moved from the VM namespace to the WasmEdge namespace and renamed to common/async.h.
  2. The Async class now takes an instance of any class, not just VM, and a pointer to a member function as template arguments.
  3. The Async class now has a StopFunc member variable of type std::function<void()>, which is used to cancel the thread when necessary.
  4. The Executor class now has an asyncInvoke member function that uses the new Async class.

Potential problems:

  1. In the invoke function definition in executor.cpp, there is no handling for the case when FuncInst is a null pointer.
  2. The StopFunc member variable of Async class might not be properly initialized when creating an Async object with the default constructor.
  3. The Async class does not provide a way to replace the StopFunc with another function, which might limit its flexibility.
  4. Function signature change in Executor::invoke now expects a pointer to FunctionInstance, which might lead to unexpected behavior.

Commit 4e2599d5aee42063ef3839ec1814124d483742ab

This patch introduces the following key changes:

  1. A new asynchronous invoke API for the executor, WasmEdge_ExecutorAsyncInvoke, has been added to the include/api/wasmedge/wasmedge.h file. This API allows developers to invoke a WebAssembly (WASM) function asynchronously after instantiating a WASM module.

  2. The implementation of WasmEdge_ExecutorAsyncInvoke has been added to the lib/api/wasmedge.cpp file. This implementation checks if the given ExecutorContext and FunctionInstanceContext are not null and then calls the asyncInvoke method with the generated parameter pair.

Potential problems:

  1. Error handling might be insufficient. The implementation checks if the context pointers are not null, but there might be more cases where the function should return nullptr or generate an error.

  2. There may be potential issues related to concurrency and thread safety due to the introduction of an asynchronous invoke API. Additional safeguards and testing might be necessary to ensure the safe execution of WASM functions in parallel.

  3. No additional unit tests are included in this patch to verify the functionality of the new asynchronous API. It is essential to create and integrate tests that cover the new functionality to ensure its correctness.

Commit b7dccbdf3829187c8c6b73dfa455c089d0d75e5b

This Pull Request contains changes to add support for async class in a common location and allows it to be utilized in the executor. It includes changes in 5 files, with 201 additions and 65 deletions.

Key changes:

  1. A std::array<WasmEdge::Byte, 46> AsyncWasm{...} variable is added in several test files containing the same set of bytecodes.
  2. In AOTcoreTest.cpp, APIStepsCoreTest.cpp, APIVMCoreTest.cpp, and ExecutorTest.cpp, the hardcoded bytecodes are replaced with the AsyncWasm variable.
  3. A new test is added in APIStepsCoreTest.cpp to test async invoke in the executor after modifications.

Potential problems:

  1. Directly changing the test files might cause issues in case there are other tests relying on the previous hardcoded values.

@q82419
Copy link
Collaborator Author

q82419 commented May 4, 2023

@apepkuss Please check that the API is what you want. Thanks.

@apepkuss
Copy link
Collaborator

apepkuss commented May 5, 2023

@q82419 Thanks for the message. I'll check the new APIs. Thanks!

@q82419 q82419 force-pushed the yiying/async branch 2 times, most recently from 8c26e9f to 4cbfdca Compare May 10, 2023 22:11
@hydai hydai added enhancement New feature or request c-CAPI An issue related to the WasmEdge C API labels May 17, 2023
@github-actions github-actions bot added the c-Test An issue/PR to enhance the test suite label May 30, 2023
@q82419 q82419 marked this pull request as ready for review June 12, 2023 12:42
lib/executor/executor.cpp Show resolved Hide resolved
@q82419 q82419 requested a review from ibmibmibm June 16, 2023 17:17
@q82419 q82419 merged commit 27fd72f into master Jun 16, 2023
@q82419 q82419 deleted the yiying/async branch June 16, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-CAPI An issue related to the WasmEdge C API c-Test An issue/PR to enhance the test suite enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

5 participants