[feat] Support new c-apis #13

apepkuss · 2023-06-25T06:29:21Z

In this PR, the new c-apis below are supported:

WasmEdge_ModuleInstanceCreateWithData
WasmEdge_ModuleInstanceGetHostData

juntao · 2023-06-25T06:29:23Z

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

The Pull Request includes many changes in various parts of the codebase, including updates to existing functionality and new functionality. The most significant changes were made to the host_data function, requiring it to have the traits Send, Sync, and Clone. While these changes seem beneficial, they may cause compatibility issues with existing code.

The patch descriptions could be improved to provide more context, and it may be necessary to review the changes in more detail to verify that all changes work together well. The patch demonstrates routine maintenance tasks, but it is hard to tell if more complex underlying restructuring is happening in the codebase due to the lack of context. The Pull Request includes tests, but they don't seem to cover some of the specific changes made.

Overall, there are no significant issues with the Pull Request, but it is recommended to review the changes in more detail and request more information from the author where necessary.

Details

Commit 95c92e381f39c52c75e168c309a9778055836bdc

Key changes

Implement two functions create_with_data and host_data to support new c-apis.

Potential problems

Code style changes are not mentioned.
No information regarding the specific changes being made.
No context on the two new functions is provided.
It is uncertain what kind of host data is being used.
It is unclear how the host data is being stored or how it will be used in other functions.

Commit 3f74cd2d0c50815432a86bc2758ca92bad914a2c

This patch adds the Finalizer type to the module module and exports it to the top level of the wasmedge-sys crate, allowing it to be used in other parts of the code.

The change could potentially cause problems if the Finalizer type is not implemented correctly or if it affects the existing functionality of the module in some way. However, this situation seems unlikely since this is a simple addition of a new type.

Commit d5ed906c7ca71d0e96a9b634ced8e2ff45784c22

This patch adds a new function host_data to the Rust SDK's Instance struct. The function is intended to return host data held by the module instance. There are no potential problems identified within this patch.

Commit 71dddba4152f99cd11d4b419cae0a4f3b6dab34c

The patch adds a new method build_with_data in ImportObjectBuilder struct that allows creating a new ImportObject instance with host data and a finalizer function. The finalizer function is used to drop the host data when the module instance is dropped. The patch also adds tests for the new method.

The changes look good and seem to serve the purpose. However, it would have been helpful to have more information on how the function should be used and the purpose behind it. Additionally, the patch could have included some documentation for the new function and any potential issues users may need to be aware of while using it.

Commit f3f2d73ef5c3d984047efcbd5663e0f5387f8fb8

The patch exports a new type Finalizer from the Rust SDK's lib.rs. This means that Finalizer can now be used outside of the Rust SDK and could be used by other parts of the codebase. There don't appear to be any potential problems with this change. It is a small and well-defined addition that is unlikely to cause any issues.

Commit fa3dd0f7d7b031e67779f8a96648321efb5bccd4

The patch is a code reformatting, which makes no functional changes to the software. It only modifies the import.rs file by rearranging the order of imports and formatting the code according to Rust formatting conventions. There are no potential problems with this change.

Commit 20c857d3ce40714e2aa85df44fc1e26ef0d2b338

Key Changes:

Updated the create_with_data function in ImportModule to include additional trait bounds for a generic type T
Added Clone derivative to Circle struct

Potential Problems:

There seem to be no potential problems with the changes made in this specific patch. However, it would be ideal to review the other patches from the larger pull request to ensure that they work well together.

Commit 662d58a95c86df8f4e23904114712a78f51c6187

Key changes:

Added trait bounds Send + Sync + Clone + 'static to the generic parameter of the build_with_data method in ImportObjectBuilder.
Defined a struct Circle and implemented Clone trait for it.
Wrapped the creation of circle variable in a block.

Potential problems:

None are apparent. The changes look safe and necessary.

Commit 76d082e5f54eb01e7712a0bdc7887e339e73fc09

Key changes:

Added the Send and Sync implementations for the NeverType enum in Rust.

Potential problems:

There are no potential problems identified in this patch.

Commit 9e5311e63b877a153ce0fc51d0fffd9c6b2e02d9

Key changes:

The host_data method in the Instance struct is updated to take a mutable reference to self.
The instance variable is changed from immutable to mutable in the test cases.

Potential problems:

None found.

Commit 8428b54b55aff11e13646d889601f3fdac1accd4

Key Changes:

host_data method in Instance struct is changed to take a mutable reference to self.
vm.named_module is changed to vm.named_module_mut.

Potential Problems:

There is no explanation for why the changes are being made.
The commit message and patch title are very general and do not provide a lot of information.
It's not clear what the impact on performance or functionality will be.

Commit aec4622f2f8109b84f5a1c9dd3e2efe2e5c71824

The key change in this pull request is an update to the host_data function in the Instance module of the wasmedge-sys crate. The function now requires that the generic type T implement the Send, Sync, and Clone traits, which ensures that the data held by the module instance can be safely accessed and modified across multiple threads.

There don't appear to be any potential problems with this change as it is a straightforward and sensible improvement to the host_data function to ensure thread safety.

Commit 32b3c85f2ff4cb658066b69f7f3469411019ddf1

This patch updates the Rust SDK by changing the host_data method in Instance to include trait bounds Send, Sync, and Clone. The purpose is likely to ensure that the host_data held by the module instance can be safely shared between threads and cloned.

The most important finding is that the developer has added Send, Sync, and Clone trait bounds to host_data, which could improve the safety and reliability of the Rust code. Since host_data is likely to be accessed often and from different threads, this change could reduce the potential for data races and make the code more robust against concurrency issues.

A potential problem with this change is that it might not be compatible with existing code that expects host_data to have a different trait bound. For instance, some uses of host_data might require Debug or Eq traits instead. Therefore, adding these trait bounds may cause existing code to break, and it is essential to verify that this change does not break any existing functionality.

Another potential problem is that the addition of the Clone trait bound may affect the performance of the code. Cloning complex objects can be expensive in terms of both time and memory, so it is important to ensure that the Clone bound is genuinely necessary before adding it.

Commit da8751e72d27d89caa179584067f1bdfc5467c5e

The key changes in this patch are the refactoring of ImportModule to take a type parameter T that implements Send + Sync + Clone. The create method now returns an ImportModule of type NeverType, which is an empty type with no values. The create_with_data method takes a Box<T> parameter and sets an optional host_data field in the ImportModule struct. Additionally, all instances of ImportModule are now being instantiated with the new <NeverType> or with the type parameter <T>. The patch also includes related changes in the test modules.

There doesn't seem to be any major issues with this patch. The changes appear to be consistent with the updated API requirements, but the patch itself lacks context regarding the motivation for the updates. It may be worthwhile to check if any other code references this newly added host_data parameter.

Commit 904cc6085582d39c42e765a764271d8ed275d7b2

Key Changes:

Refactor ImportObject and ImportObjectBuilder structs
Use generics in ImportObjectBuilder for improved type safety
Remove unnecessary Send and Sync traits from VmDock struct

Potential problems:

None of the key changes appear to be problematic in and of themselves. However, it may be worth examining the new generics implementation in ImportObjectBuilder for potential bugs.

Commit f05c65cc0a27de43941b641ddee062190b674953

The patch adds an implementation of Default for the ImportObjectBuilder struct in the src/import.rs file. This implementation adds #[derive(Debug), Default)] to the struct definition. The Default trait allows for the creation of a default instance of an object.

A potential problem with this change is that adding #[derive(Default)] may cause unexpected behavior if any fields of the struct are added or removed in the future. Additionally, if there are any use cases where creating a default ImportObjectBuilder is not appropriate, then this implementation may cause issues.

Commit ead8118e2387b42908cf5cf04f70c3d3a341cdf7

The patch updates example and test code for the wasmedge-sys crate. There are no major issues found. However, it's worth noting that the changes were not explained in the pull request description, so it's difficult to identify why these specific changes were made. Additionally, the patch updates the use of the ImportModule from ImportModule::create("host")? to ImportModule::<NeverType>::create("host")?. It's unclear why this change was made and if it has any impact on the functionality of the code. It would be best to request more information from the author.

Commit 3c38e00b6fa712b80ba7e04ba23c6fa05178dbdb

The key change is adding the NeverType to VmBuilder in compiler.rs. This allows the code to build with the latest Rust version. The change seems to be a routine maintenance task that upgrades the code to work with the newest Rust version.

Potential problems:

It is hard to know if this is really just a routine upgrade or if there is more complex underlying restructuring happening in the codebase as well.
The tests added seem to cover basic file handling and building, but they don't seem to test any specific c-apis that were added.

apepkuss · 2023-06-25T06:52:45Z

@L-jasmine Could you please help review this PR? Thanks a lot!

L-jasmine · 2023-06-25T10:09:56Z

src/import.rs

+    pub fn build_with_data<T>(
+        self,
+        name: impl AsRef<str>,
+        host_data: &mut T,
+        finalizer: Option<Finalizer>,
+    ) -> WasmEdgeResult<ImportObject> {


This function creates an ImportObject that does not have a dependency on &mut host_data in terms of its lifetime. This could potentially lead to memory safety issues.

Accepted. Thanks a lot!

L-jasmine · 2023-06-25T10:26:26Z

crates/wasmedge-sys/src/instance/module.rs

+    pub fn host_data<T>(&self) -> Option<&mut T> {
+        let ctx = unsafe { ffi::WasmEdge_ModuleInstanceGetHostData(self.inner.0) };
+
+        match ctx.is_null() {
+            true => None,
+            false => {
+                let ctx = unsafe { &mut *(ctx as *mut T) };
+                Some(ctx)
+            }
+        }
+    }
+


The Instance struct, in which host_data exists, is Send and Sync. However, host_data is obtained through an immutable borrow, which creates a mutable borrow. This means that when an Instance is cloned and sent to different threads, it's possible for multiple threads to simultaneously have &mut host_data, leading to potential data race issues.

Accepted. Thanks a lot!

…rt new c-apis Signed-off-by: Xin Liu <[email protected]>

Signed-off-by: Xin Liu <[email protected]>

apepkuss · 2023-06-27T06:46:58Z

@L-jasmine Thanks for the review!

L-jasmine reviewed Jun 25, 2023

View reviewed changes

apepkuss added 6 commits June 25, 2023 20:52

feat(rust-sys): implement create_with_data and host_data to suppo…

95c92e3

…rt new c-apis Signed-off-by: Xin Liu <[email protected]>

chore(rust-sys): export Finalizer type

3f74cd2

Signed-off-by: Xin Liu <[email protected]>

feat(rust-sdk): implement host_data in Instance

d5ed906

Signed-off-by: Xin Liu <[email protected]>

feat(rust-sdk): implement build_with_data in ImportObjectBuilder

71dddba

Signed-off-by: Xin Liu <[email protected]>

chore(rust-sdk): export Finalizer type

f3f2d73

Signed-off-by: Xin Liu <[email protected]>

chore(rust-sdk): format code

fa3dd0f

Signed-off-by: Xin Liu <[email protected]>

apepkuss force-pushed the feat/new-c-api branch from f4e9099 to fa3dd0f Compare June 25, 2023 12:53

apepkuss added 12 commits June 26, 2023 03:01

chore(rust-sys): update create_with_data in ImportModule

20c857d

Signed-off-by: Xin Liu <[email protected]>

chore(rust-sdk): update build_with_data in ImportObjectBuilder

662d58a

Signed-off-by: Xin Liu <[email protected]>

feat(rust-types): implement Send and Sync for NeverType

76d082e

Signed-off-by: Xin Liu <[email protected]>

chore(rust-sys): update host_data in Instance

9e5311e

Signed-off-by: Xin Liu <[email protected]>

chore(rust-sdk): update host_data in Instance

8428b54

Signed-off-by: Xin Liu <[email protected]>

chore(rust-sys): update host_data in Instance

aec4622

Signed-off-by: Xin Liu <[email protected]>

chore(rust-sdk): update host_data in Instance

32b3c85

Signed-off-by: Xin Liu <[email protected]>

refactor(rust-sys): refactor ImportModule

da8751e

Signed-off-by: Xin Liu <[email protected]>

refactor(rust-sdk): refactor ImportObjectBuilder and ImportObject

904cc60

Signed-off-by: Xin Liu <[email protected]>

chore(rust-sdk): impl Default for ImportObjectBuilder

f05c65c

Signed-off-by: Xin Liu <[email protected]>

chore(rust-sys): update example and test code

ead8118

Signed-off-by: Xin Liu <[email protected]>

chore(rust-sdk): update test code

3c38e00

Signed-off-by: Xin Liu <[email protected]>

L-jasmine approved these changes Jun 27, 2023

View reviewed changes

apepkuss merged commit bf2b840 into WasmEdge:main Jun 27, 2023

apepkuss deleted the feat/new-c-api branch June 27, 2023 06:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] Support new c-apis #13

[feat] Support new c-apis #13

apepkuss commented Jun 25, 2023

juntao commented Jun 25, 2023 •

edited

Loading

apepkuss commented Jun 25, 2023

L-jasmine Jun 25, 2023

apepkuss Jun 26, 2023

L-jasmine Jun 25, 2023

apepkuss Jun 26, 2023

apepkuss commented Jun 27, 2023

[feat] Support new c-apis #13

[feat] Support new c-apis #13

Conversation

apepkuss commented Jun 25, 2023

juntao commented Jun 25, 2023 • edited Loading

Details

apepkuss commented Jun 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apepkuss commented Jun 27, 2023

juntao commented Jun 25, 2023 •

edited

Loading