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

Rework Zig SDK #264

Closed
wants to merge 23 commits into from
Closed

Rework Zig SDK #264

wants to merge 23 commits into from

Conversation

sea-grass
Copy link
Contributor

@sea-grass sea-grass commented Feb 9, 2024

Summary

This PR reworks the Zig SDK to provide more control to the user w.r.t. dynamic allocation (fixes #218) and also provides facilities to write the worker config directly from the Zig build.zig, so Zig developers can run e.g. zig build; wws zig-out root and be up and running with wws in a matter of seconds.


TODO

  • Rewrite worker Zig SDK to wws Zig SDK
  • Add zig example using the wws SDK
  • Rewrite existing Zig examples using the wws SDK
  • Add example using zig-router
  • Add example demonstrating dynamic allocation (of headers, KV, response body)
  • Add example with folder mounts
  • Update Zig docs
  • Add benchmarks between current SDK (kits/zig/worker) and new SDK (in my initial testing, the new method is way faster)
  • Figure out the suggested way to get Zig developers to add wws as a dependency (see outstanding questions below)

Details

A summary of changes, so far:

  • Zig 0.11 -> Zig. 0.12 (this is important, as it includes updates to the Zig package manager)
  • All zig examples will be in examples/zig-examples to ease maintenance of Zig example workers and also demonstrate composition of multiple wws workers in a single server
  • Renamed the Zig SDK module from worker to wws
  • The wws module exposes some build-time functions to build a Zig worker and associated config
  • The wws module exposes functions to read the request object from stdin (or any reader) and write the request object to stdout (or any writer)

I believe these changes align with the goals of a worker:

  • Easier to develop: Since the worker's main function is the request handler (no need to defer to a handleFn, unless it makes sense to split code that way for your use case), the code stays small and focused.
  • Easier to test: The wws request parser lets you alternatively provide a string as request input (no need to depend on stdin) and the wws response writer lets you provide your own writer (does not depend on stdout), so it's easy for developers to test individual parts of their worker. Additionally, the request parser requires the developer to provide their own allocator, so it's easy to ensure there are no memory leaks.
  • Easier to deploy: The wws module provides helpers for compiling wasm and writing the associated worker config, so there are less steps to getting a wws server up and running.

Overall, with the new API surface, developing wws workers with Zig looks like this:

  1. Add wws as a dependency
  2. In your build.zig, use const worker = try wws.addWorker(...) to build your Zig program as a WASM module
  3. In your build.zig, use worker.addToWriteFiles(b, write_files) to copy the worker and its associated config to a WWS server directory
  4. In your root source file (i.e. src/main.zig), use wws.parseStream(allocator, parse_config) to decode the request
  5. Construct a wws.Response
  6. Use wws.writeResponse(response, writer) to write the response

Minimal Zig worker now looks like this (click to expand)

// build.zig.zon
.{
  // ...
  .dependencies = .{
    .wws = .{
      .path = "../path/to/kits/zig/wws",
    },
  },
}
// build.zig
const std = @import("std");
const wws = @import("wws");

pub fn build(b: *std.Build) !void {
  const wws_dep = b.dependency("wws", .{});

  const wf = b.addWriteFiles();

  const worker = try wws.addWorker(.{
    .name = "example",
    .root_source_file = .{ .path = "src/main.zig" },
    .wws = wws_dep,
  });

  worker.addToWriteFiles(b, wf);

  const install = b.addInstallDirectory(.{
    .source_dir = wf.getDirectory(),
    .install_dir = .prefix,
    .install_subdir = "root",
  });

  b.getInstallStep().dependOn(&install.step);
// main.zig
const std = @import("std");
const wws = @import("wws");

pub fn main() !void {
  var gpa = std.heap.GeneralPurposeAllocator(.{}){};
  // can switch on the result to check for memory leaks
  defer _ = gpa.deinit();
  const allocator = gpa.allocator();

  // By default, will read from stdin
  const parse_result = try wws.parseStream(allocator, .{});
  const request = parse_result.value;

  const response = wws.Response{
    // Simply echo the request url
    .data = request.url,
  };

  const stdout = std.io.getStdOut();
  // Serializes the response as a json object and writes to the provided writer
  try wws.writeResponse(response, stdout.writer());
}

To build/run the server:

$ zig build
$ tree zig-out/root
zig-out/root/
├── example.toml
└── example.wasm
$ wws zig-out/root
⚙️  Preparing the project from: zig-out/root
⚙️  Loading routes from: zig-out/root
⏳ Loading workers from 1 routes...
✅ Workers loaded in 29.292541ms.
    - http://127.0.0.1:8080/example
      => zig-out/root/example.wasm
🚀 Start serving requests at http://127.0.0.1:8080

Outstanding questions:

  • Should the Zig kit be moved to a separate repo? Zig's package manager lets you specify dependencies in the local filesystem or a remote tar archive, but I'm not sure if it lets you specify a "relative root" for the Zig package in that tarball. An alternative would be to move the build.zig to the root of this repository, but that may be confusing.

@sea-grass
Copy link
Contributor Author

@voigt I've still got a bit of work to do on this PR, but just tagging you in case you have some early feedback/to get the discussion going.

@sea-grass sea-grass marked this pull request as ready for review February 17, 2024 21:54
@sea-grass
Copy link
Contributor Author

I've done all of the code changes I was planning to do, so I've marked this ready for review. There are still some outstanding documentation/housekeeping tasks, but I figured it'd be good to start those after code review.

Let me know of any suggestions/questions/considerations. Thanks!

@voigt
Copy link
Contributor

voigt commented Feb 18, 2024

This already looks great - thank you for the excuse to look a bit deeper into Zig 0.12 🤓
zig-router also looks amazing.

I'm starting to look into your PR!

@voigt
Copy link
Contributor

voigt commented Feb 18, 2024

Tried to build examples, but have no luck with the recent zig master:

$ zig-examples git:(zig-updates) ✗ pwd
/Users/c.voigt/go/src/github.com/voigt/wasm-workers-server/examples/zig-examples
$ zig-examples git:(zig-updates) ✗ zig version      
0.12.0-dev.2809+f3bd17772
$ zig-examples git:(zig-updates) ✗ zig build        
Fetch Packages [5/4] Index pack... /Users/c.voigt/.cache/zig/p/1220ce458470e5982a1d1b15a1571b17079c7f635639753d42f5c22d8903914784df/build.zig.zon:13:20: error: unable to unpack git files: BadZlibHeader
            .url = "git+https://github.com/ibokuri/protest.git#bae398b701e763ef18480aa3cfcca103716996de",
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

In the meantime, I will try the version you documented.

@sea-grass
Copy link
Contributor Author

@voigt thanks for testing it out! I can confirm I see this too after updating to the latest Zig. Looks like the dependency for one of the examples, zig-router, isn't compatible with that Zig version yet. I just commented out all the relevant dependency/example code for that, so you should be able to test out the rest of the examples with the latest Zig 0.12.0-dev.2809+f3bd17772 again.

@sea-grass
Copy link
Contributor Author

Created a PR against getty (getty-zig/getty#154) to fix the issue you saw when running zig build with the latest Zig. Once that gets merged in and zig-router is updated to point to the new version, we should be able to uncomment the zig-router example.

@ereslibre ereslibre added the 🚀 enhancement New feature or request label Feb 22, 2024
@ereslibre
Copy link
Contributor

Thanks a lot for your contribution @sea-grass, and @voigt for taking the time to review it! Although I am not knowledgeable on Zig, I'll have a general pass tomorrow and confirm that the examples are working :)

@Angelmmiguel
Copy link
Contributor

This is amazing! Thank you @sea-grass for the contribution and @voigt for reviewing and testing the different changes. We will go over the code and provide feedback soon. For now, I just activated the CI for this MR.

Thank you!

This commit introduces `kits/zig/wws`, with the intention of replacing `kits/zig/worker`.
The design of the new SDK improves upon the previous one by exposing utilities for parsing wws requests and serializing wws responses, to give the user more control over memory while still keeping the SDK easy to use.
The name was changed to make it more ergonomic within the Zig ecosystem (the developer will pull in a `wws` dependency to interface with wws, rather than a `worker` dependency).
The dependency `zig-router` doesn't yet support the latest Zig. Once that library is updated, we can uncomment this example.

`std.fs.Dir.openFileWasi` has been removed, so we can just use `std.fs.Dir.openFile`.
@ereslibre
Copy link
Contributor

ereslibre commented Feb 23, 2024

I see that the latest Zig version is 0.11 yet. If we document it, I think it's fine, but it would be even better if we release all changes when 0.12 is officially out, not sure about their timeline. However, I would say it's definitely not a blocker to merge this work.

I have downloaded https://ziglang.org/builds/zig-linux-x86_64-0.12.0-dev.2833+8802ec583.tar.xz and used this one for my tests.

So far it's looking terrific. I only found a couple of issues:

  • Although the /workerkv seems to increment the counter, /no-alloc-kv and /mixed-alloc-kv seem to always return 1.

Regarding your task Add benchmarks between current SDK (kits/zig/worker) and new SDK (in my initial testing, the new method is way faster): if you want to provide it, it would be amazing, but it's not really a requirement given how experimental wasm-workers-server is at this moment. However, as I said, if you want to provide it, that would be stellar.

Thanks for your work both, I think we are very close to merge this work :)

@sea-grass
Copy link
Contributor Author

I see that the latest Zig version is 0.11 yet. If we document it, I think it's fine, but it would be even better if we release all changes when 0.12 is officially out, not sure about their timeline. However, I would say it's definitely not a blocker to merge this work.

I'm not sure when 0.12 will be released either, but as Zig is pre-1.0 it seems to me that it's fairly common for Zig developers to keep up with the latest Zig master, for the most part. Zig 0.12 introduces the built-in package manager, after all. I know of one Zig project, Mach, which doesn't keep up with the very latest but instead has a recommended supported Zig version (which currently points to Zig 0.12.0-dev.2063+804cee3b9, so not that far behind).

Although the /workerkv seems to increment the counter, /no-alloc-kv and /mixed-alloc-kv seem to always return 1.

Good catch! I missed adding the features for those examples. They should work correctly now.

Regarding your task Add benchmarks between current SDK (kits/zig/worker) and new SDK (in my initial testing, the new method is way faster): if you want to provide it, it would be amazing, but it's not really a requirement given how experimental wasm-workers-server is at this moment. However, as I said, if you want to provide it, that would be stellar.

I have some preliminary results but am hoping to share something more concrete soon. I think I was a bit hasty in saying that "the new method is way faster." The old method made no heap allocations and the current method does make heap allocations, and from further testing it's clear that no heap allocations is always faster.

That influenced me to add the no-alloc-kv and mixed-alloc-kv to be able to test the "no heap allocations" scenario with the new wws Zig SDK. The performance of these examples is comparable to the pre-existing worker-kv example + the no heap allocations worker.zig. The nice part is that no-alloc-kv, mixed-alloc-kv, and worker-kv examples are pretty much identical, except for their choice in allocators. (For context, in Zig, instead of using malloc/free to allocate memory you make use of something that implements the Allocator interface.)

no-alloc-kv uses a FixedBufferAllocator, which is pretty self-explanatory; all memory that it allocates belongs to a fixed buffer (which, in the example, is on the stack). The performance of this example is nearly the same as the pre-existing worker-kv + worker.zig since it doesn't make any heap allocations.

mixed-alloc-kv uses a StackFallbackAllocator, which essentially does the same thing as FixedBufferAllocator but, in this example, falls back to allocating on the heap if required. The performance of this example is nearly the same as the pre-existing worker-kv + worker.zig in the regular case, but it's able to handle arbitrarily large request/response objects.

Once I have some spare time I'll run some more measurements with the latest release build of WWS and post a simple table comparing the performance between each of these examples.

@voigt
Copy link
Contributor

voigt commented Feb 24, 2024

I +1 on the belief that Zig developers are usually on the latest master. For 0.12, this is an even more significant benefit, as the package manager improves working with the language. I'd be okay with going with 0.12 as long as we document the version.

I really like the improvements you made. They definitely make wws' Zig support more polished! 🎉

@sea-grass
Copy link
Contributor Author

I really like the improvements you made. They definitely make wws' Zig support more polished! 🎉

Thanks, @voigt! I couldn't have made this progress on the Zig integration without your initial implementation, so thanks a lot for all the ground work and getting it started!

@sea-grass
Copy link
Contributor Author

As promised, here are some simple benchmarks comparing the Zig KV examples.

Each benchmark was run using a release build of the WWS main branch (at 4b05de2), on a MacBook Pro with an M2 chip. Zig version 0.12.0-dev.2811+3cafb9655 was used to compile the examples (by running zig build in the zig-examples folder). I used hey to send requests and summarize results, using the following command:

$ hey -z 10s http://127.0.0.1:8080/path-to-example

Here are the results, sorted by Requests/sec:

Zig example Zig SDK used Can make heap allocations Requests/sec Fastest (secs) Slowest (secs) Average (secs)
no-alloc-kv wws.zig no 2255.2 0.0119 0.0434 0.0221
mixed-alloc-kv wws.zig yes 2221.8 0.0118 0.0424 0.0225
worker-kv (old) worker.zig no 2069.4 0.0122 0.0367 0.0241
workerkv wws.zig yes 1748.6 0.0112 0.045 0.0285

The no-alloc-kv + wws.zig and worker-kv (old) + worker.zig are the most similar - they both only make use of a buffer on the stack to build up the request/response objects - but you can see there's a marginal improvement in response time with the new SDK.

The mixed-alloc-kv example can make heap allocations, but I think the buffer size used was large enough that it didn't have to make any for this benchmark, so it makes sense that its performance is comparable to that of no-alloc-kv.

@sea-grass
Copy link
Contributor Author

Side note: If you're interested in running the benchmarks yourself, check out the benchmark branch, which also compiles and hosts the previous worker-kv example: https://github.com/sea-grass/wasm-workers-server/tree/benchmark

@sea-grass
Copy link
Contributor Author

Just finished updating the docs! Let me know if you want me to update/clarify anything else.

Also, for the last item on my TODO ("Figure out the suggested way to get Zig developers to add wws as a dependency") I figured adding the wasm-workers-server repo as a git submodule would be the least-friction way of adding the WWS Zig SDK to the Zig project:

zig init
git init
mkdir lib
git submodule add https://github.com/vmware-labs/wasm-workers-server.git lib/wws
# Then, follow instructions in the docs to add the wws dependency to build.zig and add a worker

The Zig package manager does support referencing dependencies directly from e.g. a GitHub repo, but AFAIK it requires the Zig project (i.e. the build.zig and build.zig.zon files) to be at the root of the repository, which may be confusing within the WWS repo, since all of the SDK files currently exist at the kit/ subpath. Let me know if you have any thoughts/suggestions on this.

Pending any further change requests, I think this is just about done! I also tested the examples on the latest Zig version as of today (0.12.0-dev.3381+7057bffc1) and it looks good.

@Angelmmiguel
Copy link
Contributor

This was an amazing job @sea-grass ! Thank you very much for all the progress and work here. I plan to give it a final review over these days and merge it!

Thank you also for your patience here :)

@Angelmmiguel
Copy link
Contributor

Hello @sea-grass @voigt ,

Sorry for the long time to reply and thank you for your contribution. After some long discussions, this project is currently in the process of being archived. However, the development of this project will continue as an independent community fork present at https://github.com/webassemblylabs/wasm-workers-server.

We would be grateful if you could recreate your contribution under that fork and we will work on applying it.

Thank you ❤️

@sea-grass
Copy link
Contributor Author

Thanks for letting me know @Angelmmiguel. I'll re-open this PR against that repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow the Zig SDK to return arbitrarily big responses
4 participants