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

Plugin zlib #2395

Closed
wants to merge 8 commits into from
Closed

Plugin zlib #2395

wants to merge 8 commits into from

Conversation

NeelDigonto
Copy link
Contributor

@NeelDigonto NeelDigonto commented Apr 4, 2023

feat: a zlib plugin

I have already completed a minimal but complete zlib implementation with WasmEdge C SDK here: https://github.com/notfathomless/wasmedge-zlib/tree/main/src.

I also posted a detailed plan on how I approached this issue and what we might need to consider here: #2244 (comment).

I have started to transition my code into a proper WasmEdge Plugin.

So, this PR is being created to track it's progress.

PROGRESS:

  • Basic Plugin Setup.
  • deflateInit_
  • deflateInit
  • deflate
  • deflateEnd
  • inflateInit_
  • inflateInit
  • inflate
  • inflateEnd

Note that all these functions currently stand implemented in my WasmEdge C SDK playground.
I will continue re-implementing the functions here after finishing my GSOC Proposal.

Thank you all.
Have a nice day.

…nd', albeit without syncing msg to wasm z_stream, which will require further discussion.
Copy link
Contributor

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


The reviews are about a software patch that adds a new plugin called WasmEdge Zlib. The patch modifies existing files, adds new directories and files, and includes two new jobs to the build matrix for building the plugin on different architectures. The patch enables the zlib library to a WasmEdge module, implementing several new classes each representing a different zlib function. Potential problems include typos or syntax errors in the new code and missing implementation of some functions that need to be added for proper functionality. Further, the code requires review to ensure it meets project coding standards and best practices. Some necessary #include statements for new classes are not included, and further discussion is needed before syncing the msg field to the z_stream.

Details

Commit 1

The patch adds a new plugin called WasmEdge Zlib to the existing plugins list. The changes include adding new directories and files, and modifying existing files to include the new plugin. The patch also adds two new jobs to the build matrix for building the plugin on different architectures.

Potential problems include typos or syntax errors in the new code which may cause the build process to fail. Additionally, there is a missing implementation of WasmEdgeZlib module function which needs to be added for proper functionality. The code also needs to be reviewed to ensure it meets the project's coding standards and best practices.

Commit 2

This patch adds support for the zlib library to a WasmEdge module. Key changes include:

  • New struct z_stream, which is used to manage compression/decompression streams
  • WasmEdgeZlibEnvironment now uses a std::unordered_map to store z_stream objects
  • Several new classes are defined, each representing a different zlib function, inheriting from a parent WasmEdgeZlib class
  • These new classes implement the body function, which defines the behavior of each corresponding zlib function
  • WasmEdgeZlibModule now registers each of these new classes as host functions

A potential problem with this patch is that it does not include the necessary #include statements for some of the new classes, which could lead to compilation errors. Additionally, further discussion is required before syncing the msg field to the z_stream.

@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #2395 (62366ed) into master (4c37361) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2395   +/-   ##
=======================================
  Coverage   82.23%   82.23%           
=======================================
  Files         133      133           
  Lines       19270    19270           
  Branches     4096     4096           
=======================================
  Hits        15847    15847           
  Misses       2416     2416           
  Partials     1007     1007           

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

@WasmEdge WasmEdge deleted a comment from alabulei1 Apr 5, 2023
@juntao
Copy link
Member

juntao commented Apr 5, 2023

flows summarize

Copy link
Contributor

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


Overall Summary:

The primary focus of the submitted patches is the development and initialization of a new WasmEdge Zlib plugin. The key changes include adding a new plugin directory, incorporating the Zlib dependency in the CMakeLists.txt file, and defining new structs and classes for the zlib plugin. Furthermore, the patches involve removing the Util struct, adding the z_stream struct, and creating classes for various zlib functions. These updates improve the efficiency and functionality of the plugin.

However, there are some potential issues and errors present in these patches. Firstly, the absence of tests for the newly added plugin is a concern, as unit tests or integration tests should be included to ensure its proper functioning. Secondly, there is an incorrect class name in the zlibbase.h file which needs to be corrected. Additional problems include the lack of error validations for input parameters and unimplemented functionality related to syncing message information from wasm ZStream.

To address these issues and ensure the robustness of the plugin, it is recommended to review the current changes, discuss possible improvements, and finalize the desired behavior to avoid any complications or unforeseen consequences.

Details

Commit 1

This patch includes the initialization of a new WasmEdge Zlib Plugin. The key changes can be summarized as follows:

  1. Added new plugin directory plugins/wasmedge_zlib with necessary files such as CMakeLists.txt, zlibbase.h, zlibenv.cpp, and zlibenv.h.

  2. Included the Zlib dependency in the CMakeLists.txt file.

  3. Added the new plugin option WASMEDGE_PLUGIN_ZLIB to configure the build.

  4. Updated the .github/workflows/build-extensions.yml file for plugin building and added a new job for building the WasmEdge Zlib plugin.

  5. Defined new structs and classes for the zlib plugin.

Potential issues:

  1. The patch doesn't include any tests for the newly added plugin. It should have unit tests or integration tests to ensure it works as expected.

  2. The patch seems to contain an incorrect class name, WasmEdgeHttpsReq, in the zlibbase.h file. It should be WasmEdgeZlib.

Commit 2

Summary of key changes:

  1. Removed the Util struct and added a z_stream struct in zlibenv.h.
  2. Replaced std::unordered_map<uint32_t, z_stream *> with std::unordered_map<uint32_t, std::unique_ptr<z_stream>> in WasmEdgeZlibEnvironment class.
  3. Added classes for different zlib functions (WasmEdgeZlibDeflateInit_, WasmEdgeZlibInflateInit_, WasmEdgeZlibDeflateInit, WasmEdgeZlibInflateInit, WasmEdgeZlibDeflate, WasmEdgeZlibInflate, WasmEdgeZlibDeflateEnd, and WasmEdgeZlibInflateEnd) in zlibfunc.h.
  4. Implemented methods corresponding to these classes in zlibfunc.cpp.
  5. Registered all these functions in WasmEdgeZlibModule class in zlibmodule.cpp.

Potential problems:

  1. There are no error validations for input parameters, which could lead to improper program behavior if not provided.
  2. In WasmEdgeZlibDeflateEnd and WasmEdgeZlibInflateEnd classes, the comments mention that they will later sync message information from wasm ZStream. This functionality is not yet implemented and could lead to incorrect behavior if not added.

As the solution requires further discussion for syncing the message to wasm z_stream, it's recommended to review the current changes, possible improvements, and finalize the desired behavior to avoid any issues.

@NeelDigonto NeelDigonto marked this pull request as draft April 5, 2023 08:01
@q82419
Copy link
Collaborator

q82419 commented Apr 11, 2023

Thanks for your contribution.
We'll discuss for this plug-in then.

@NeelDigonto
Copy link
Contributor Author

NeelDigonto commented Apr 14, 2023

@q82419 Thank you for checking out the PR, I would to love add more of the zlib library.
I am very open to discussion, and I will definitely need to talk about certain stuff's like if WasmEdge could allocate memory on the WASM Linear memory, and how the malloc implementation of WasmEdge works(I will need to take a look).
Will need info on this to get few things like the z_stream's z_const char *msg; struct field to sync properly.

Anyways we will discuss, tell me how I can get back to you or any other developers or maintainers, I am already on the Discord and Slack channel as fathomless.

I have already contacted you on Discord on 3rd April, so I might be on the contact list.

And also very sorry for the delay in responding, I used to check this PR regularly hoping somebody will respond but I completely missed your message. I used to press End on my keyboard and scroll to the bottom and your messages got hidden in these bunch of automated test case section.

Hoping to have a good discussion and start contributing more.
Have a great day!

@q82419
Copy link
Collaborator

q82419 commented Apr 28, 2023

@q82419 Thank you for checking out the PR, I would to love add more of the zlib library. I am very open to discussion, and I will definitely need to talk about certain stuff's like if WasmEdge could allocate memory on the WASM Linear memory, and how the malloc implementation of WasmEdge works(I will need to take a look). Will need info on this to get few things like the z_stream's z_const char *msg; struct field to sync properly.

Anyways we will discuss, tell me how I can get back to you or any other developers or maintainers, I am already on the Discord and Slack channel as fathomless.

I have already contacted you on Discord on 3rd April, so I might be on the contact list.

And also very sorry for the delay in responding, I used to check this PR regularly hoping somebody will respond but I completely missed your message. I used to press End on my keyboard and scroll to the bottom and your messages got hidden in these bunch of automated test case section.

Hoping to have a good discussion and start contributing more. Have a great day!

Sorry that we were working on the 0.12.0 releases and didn't have enough time to concentrate on this.
For the allocate on the WASM linear memory, we'll plan it in the nearly future.
Thanks.

@NeelDigonto
Copy link
Contributor Author

@q82419 Thank you for checking out the PR, I would to love add more of the zlib library. I am very open to discussion, and I will definitely need to talk about certain stuff's like if WasmEdge could allocate memory on the WASM Linear memory, and how the malloc implementation of WasmEdge works(I will need to take a look). Will need info on this to get few things like the z_stream's z_const char *msg; struct field to sync properly.
Anyways we will discuss, tell me how I can get back to you or any other developers or maintainers, I am already on the Discord and Slack channel as fathomless.
I have already contacted you on Discord on 3rd April, so I might be on the contact list.
And also very sorry for the delay in responding, I used to check this PR regularly hoping somebody will respond but I completely missed your message. I used to press End on my keyboard and scroll to the bottom and your messages got hidden in these bunch of automated test case section.
Hoping to have a good discussion and start contributing more. Have a great day!

Sorry that we were working on the 0.12.0 releases and didn't have enough time to concentrate on this. For the allocate on the WASM linear memory, we'll plan it in the nearly future. Thanks.

Sure, we can start discussing implementation details when you get some time!

Thanks, have a nice day.

@hydai hydai added the c-Plugin An issue related to WasmEdge Plugin label May 17, 2023
@hydai
Copy link
Member

hydai commented Jun 14, 2023

Hi @notfathomless
It looks like you already have a new PR for zlib plugin. Is it fine to close this?

@NeelDigonto
Copy link
Contributor Author

#2562 supersedes this PR.

@NeelDigonto

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-Plugin An issue related to WasmEdge Plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants