Wasm build #93

dnlup · 2021-03-08T10:25:56Z

👋🏻 There.

This PR is based on the work started by @devsnek in https://github.com/devsnek/llhttp/tree/wasm.

A little background

We needed a wasm build of llhttp to implement a custom parser in nodejs/undici#575.

The objective of these changes is to expose the required pieces in api.(h|c) to implement a parser using a web assembly build of llhttp and add the build logic to build the binary. We also needed to run the build in a docker container (nodejs/undici#22 (comment)).

Given the tight coupling of the changes with the pieces used to build the c parser, we thought it was worth a shot opening a PR to discuss integrating these changes upstream.

dnlup · 2021-03-08T10:27:10Z

cc @mcollina @ronag

ronag

I don't think the package-lock.json should be here

Add wasm build using a docker container. Expose some api needed only in wasm.

devsnek · 2021-03-08T13:43:53Z

I think llhttp should abstract away the wasm instantiation, callback tracking, reading strings etc. maybe something like:

const llhttp = require('llhttp/wasm');

const parser = new llhttp.Parser(llhttp.RESPONSE, {
  onX() {},
  onY() {},
})

parser.execute(bytes); // might throw instead of returning an errno

You could also export llhttp.wasmModule so people can create their own instances.

ronag · 2021-03-08T14:41:02Z

Creating our instance is what we've done over at nodejs/undici#575. Where we on purpose only have implemented RESPONSE. Would it be sufficient to just do llhttp.wasmModule for now?

devsnek · 2021-03-08T14:53:26Z

hmm I was thinking the logic would still be implemented by the user, like they'd put an llhttp.Parser inside their HTTPParser (like how node uses llhttp_t inside its HTTPParser). the interface on the llhttp side would just be responsible for converting errors and strings and keeping the callbacks consistent. I'm not maintaining this though so you do you.

Also a sidenote, there's a useful crossover between versions of node that expose the internal http parser, and versions of node that support FinalizationRegistry, so I think you could hardcode gc on this side.

dnlup · 2021-03-08T16:15:25Z

hmm I was thinking the logic would still be implemented by the user, like they'd put an llhttp.Parser inside their HTTPParser (like how node uses llhttp_t inside its HTTPParser). the interface on the llhttp side would just be responsible for converting errors and strings and keeping the callbacks consistent. I'm not maintaining this though so you do you.

Also a sidenote, there's a useful crossover between versions of node that expose the internal http parser, and versions of node that support FinalizationRegistry, so I think you could hardcode gc on this side.

I like this idea. Given that we are still in an early stage, as @ronag said even just a wasm module is enough for now if it's not a problem of course. There's a lot of room for improvement. I would be ok with merging this in a custom branch if it's better.

mcollina · 2021-03-08T16:18:11Z

It really depends what @indutny prefers :).

dnlup · 2021-03-08T16:32:19Z

It really depends what @indutny prefers :).

Yes, of course

indutny · 2021-03-09T03:11:01Z

Yay. WASM!

I think we should strive for a saner API like @devsnek has posted, but having raw WASM support as an unstable feature is acceptable too. Not locking into JS API means that you'd have more room to experiment with it an undici, right?

dnlup · 2021-03-09T08:35:42Z

Not locking into JS API means that you'd have more room to experiment with it an undici, right?

Yes, exactly.

I think the JS API will organically define itself while we experiment (I would be happy to try to implement it). Defining a JS API here would also mean that we would have to install llhttp as a dependency in undici, and we would like to avoid any dependencies (formally declared in package.json) atm.

ronag · 2021-03-09T08:45:14Z

Just for reference:

It would be nice if the wasm build lived in undici. That way we could apply undici specific optimizations in the wasm api and reduce the number of wasm->js calls. I think we could move more of the HTTPParser logic into native/wasm land.
e.g. wasm_on_header_field and wasm_on_header_value might be possible to combine into wasm_on_headers.

i.e. if we put wasm in llhttp we might want to do some semver major (?) changes in the future related to optimizations of the API. If that makes sense?

indutny · 2021-03-11T02:37:16Z

So is this ready for review?

mcollina · 2021-03-11T07:47:53Z

Yes it is.

ronag · 2021-03-17T08:05:36Z

@indutny any idea when you might have time to review this?

indutny · 2021-03-18T05:30:54Z

Hey, Sorry for the delay. I'll try to look into it this weekend. Thanks for submitting it again!

…

On Wed, Mar 17, 2021 at 1:05 AM Robert Nagy ***@***.***> wrote: @indutny <https://github.com/indutny> any idea when you might have time to review this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#93 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB2HQ5KH36RMR4NSZJN5TLTEBPGDANCNFSM4YZEHGGQ> .

indutny

This looks very good, but I've left few nitpicks/requests that have to be resolved before merging. Thank you for submitting it, and sorry for a long delay on my end.

build_wasm.js

examples/wasm.js

src/native/api.c

dnlup · 2021-03-29T06:35:30Z

This looks very good, but I've left few nitpicks/requests that have to be resolved before merging. Thank you for submitting it, and sorry for a long delay on my end.

No worries, thank you for the review. I'll work on it!

Co-authored-by: Fedor Indutny <[email protected]>

dnlup · 2021-04-02T07:46:08Z

To reiterate:

these changes add some scripts to build wasm. Nothing is exposed in the package itself. If one wanted to use the build, they would have to copy the build/wasm folder in their project and write their own parser.

I see a lot of potential with wasm. The next step would be exposing a set of API at the package level that allows the user to install llhttp as a dependency and do something like:

import { createParser, constants } from 'llhttp';

const parser = createParser(constants.TYPE.RESPONSE, {
  ...// other options
})

As you already suggested.

This reduces the context size from a few MB to a few KB. Minor cleanup in `examples/wasm.ts`.

indutny

Awesome work. Thank you so much for taking time to address my review comments!

indutny · 2021-04-04T05:02:26Z

bin/build_wasm.ts

+    mkdirSync(join(WASM_SRC, 'build'));
+    process.exit(0);
+  } catch (error) {
+    if (error.code !== 'EEXIST') {


Nice touch!

indutny · 2021-04-04T05:44:22Z

Released in v5.0.0

wasm

69d4405

dnlup mentioned this pull request Mar 8, 2021

Use llhttp wasm build nodejs/undici#575

Closed

7 tasks

ronag reviewed Mar 8, 2021

View reviewed changes

feat: add wasm build

4021558

Add wasm build using a docker container. Expose some api needed only in wasm.

dnlup force-pushed the undici_wasm branch from 8ab5f85 to 4021558 Compare March 8, 2021 10:42

ronag approved these changes Mar 8, 2021

View reviewed changes

mcollina requested a review from indutny March 8, 2021 15:07

indutny requested changes Mar 27, 2021

View reviewed changes

indutny mentioned this pull request Mar 27, 2021

api: introduce llhttp_set_lenient_keep_alive #94

Closed

dnlup and others added 5 commits March 29, 2021 17:26

doc: fix examples typo

a16190e

Co-authored-by: Fedor Indutny <[email protected]>

doc: refactor examples

d215177

Co-authored-by: Fedor Indutny <[email protected]>

src: remove useless changes

319fdca

build: refactor wasm build script to bin/*.ts

5b549b1

examples: re-write in .ts

ed0d43a

dnlup force-pushed the undici_wasm branch from d9a6a06 to ed0d43a Compare March 29, 2021 16:40

dnlup added 2 commits March 29, 2021 18:47

examples: minor refactor

d59b33b

build(wasm): export constants for ts and js users

b0773dc

build(wasm): stricter dockerignore

a18d702

This reduces the context size from a few MB to a few KB. Minor cleanup in `examples/wasm.ts`.

dnlup force-pushed the undici_wasm branch from 810527e to a18d702 Compare April 2, 2021 13:36

indutny approved these changes Apr 4, 2021

View reviewed changes

bin/build_wasm.ts

mkdirSync(join(WASM_SRC, 'build'));

process.exit(0);

} catch (error) {

if (error.code !== 'EEXIST') {

Copy link

Member

indutny Apr 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice touch!

indutny merged commit a620012 into nodejs:master Apr 4, 2021

dnlup mentioned this pull request Apr 7, 2021

Update llhttp nodejs/undici#691

Merged

This was referenced Feb 4, 2022

What is the difference between wasi-sdk and emscripten? WebAssembly/wasi-sdk#222

Closed

feat: add build for porting fetch to Node.js nodejs/undici#1183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wasm build #93

Wasm build #93

dnlup commented Mar 8, 2021 •

edited

Loading

dnlup commented Mar 8, 2021

ronag left a comment

devsnek commented Mar 8, 2021 •

edited

Loading

ronag commented Mar 8, 2021 •

edited

Loading

devsnek commented Mar 8, 2021 •

edited

Loading

dnlup commented Mar 8, 2021

mcollina commented Mar 8, 2021

dnlup commented Mar 8, 2021

indutny commented Mar 9, 2021

dnlup commented Mar 9, 2021

ronag commented Mar 9, 2021 •

edited

Loading

indutny commented Mar 11, 2021

mcollina commented Mar 11, 2021

ronag commented Mar 17, 2021

indutny commented Mar 18, 2021 via email

indutny left a comment

dnlup commented Mar 29, 2021

dnlup commented Apr 2, 2021

indutny left a comment

indutny Apr 4, 2021

indutny commented Apr 4, 2021

Wasm build #93

Wasm build #93

Conversation

dnlup commented Mar 8, 2021 • edited Loading

A little background

dnlup commented Mar 8, 2021

ronag left a comment

Choose a reason for hiding this comment

devsnek commented Mar 8, 2021 • edited Loading

ronag commented Mar 8, 2021 • edited Loading

devsnek commented Mar 8, 2021 • edited Loading

dnlup commented Mar 8, 2021

mcollina commented Mar 8, 2021

dnlup commented Mar 8, 2021

indutny commented Mar 9, 2021

dnlup commented Mar 9, 2021

ronag commented Mar 9, 2021 • edited Loading

indutny commented Mar 11, 2021

mcollina commented Mar 11, 2021

ronag commented Mar 17, 2021

indutny commented Mar 18, 2021 via email

indutny left a comment

Choose a reason for hiding this comment

dnlup commented Mar 29, 2021

dnlup commented Apr 2, 2021

indutny left a comment

Choose a reason for hiding this comment

indutny Apr 4, 2021

Choose a reason for hiding this comment

indutny commented Apr 4, 2021

dnlup commented Mar 8, 2021 •

edited

Loading

devsnek commented Mar 8, 2021 •

edited

Loading

ronag commented Mar 8, 2021 •

edited

Loading

devsnek commented Mar 8, 2021 •

edited

Loading

ronag commented Mar 9, 2021 •

edited

Loading