PR#3: Add loader for nodejs builtin (rhbz#2259320) - rpms/nodejs-undici

rpms / nodejs-undici

#3 Add loader for nodejs builtin (rhbz#2259320)

Closed 2 years ago by jstanek. Opened 2 years ago by mochaa.

rpms/ mochaa/nodejs-undici fix-builtin-loading into rawhide

Read llhttp from .wasm files instead of base64-encoded .js

Zephyr Lykos • 2 years ago

708f0b8

Add loader for nodejs builtin (rhbz#2259320)

Zephyr Lykos • 2 years ago

92eca37

0001-perf-read-wasm-files-directly.patch

file added

+82

		`@@ -0,0 +1,82 @@`
		`+ From 9bdd36bb4ffdd020b852bb9ab5c8641036a014bd Mon Sep 17 00:00:00 2001`
		`+ From: Zephyr Lykos <[email protected]>`
		`+ Date: Fri, 26 Jan 2024 15:09:48 +0800`
		`+ Subject: [PATCH] perf: read .wasm files directly`
		`+`
		`+ ---`
		`+ build/wasm.js \| 14 +-------------`
		`+ lib/client.js \| 7 +++----`
		`+ 2 files changed, 4 insertions(+), 17 deletions(-)`
		`+`
		`+ diff --git a/build/wasm.js b/build/wasm.js`
		`+ index 2b63f3c7..8fe0e34d 100644`
		`+ --- a/build/wasm.js`
		`+ +++ b/build/wasm.js`
		`+ @@ -1,7 +1,7 @@`
		`+ 'use strict'`
		`+`
		`+ const { execSync } = require('child_process')`
		`+ -const { writeFileSync, readFileSync } = require('fs')`
		`+ +const { writeFileSync } = require('fs')`
		`+ const { join, resolve } = require('path')`
		`+`
		`+ const ROOT = resolve(__dirname, '../')`
		+ @@ -67,21 +67,9 @@ execSync(`${WASM_CC} ${WASM_CFLAGS} ${WASM_LDFLAGS} \
		`+ -o ${join(WASM_OUT, 'llhttp.wasm')} \`
		+ ${WASM_LDLIBS}`, { stdio: 'inherit' })
		`+`
		`+ -const base64Wasm = readFileSync(join(WASM_OUT, 'llhttp.wasm')).toString('base64')`
		`+ -writeFileSync(`
		`+ - join(WASM_OUT, 'llhttp-wasm.js'),`
		+ - `module.exports = '${base64Wasm}'\n`
		`+ -)`
		`+ -`
		`+ // Build wasm simd binary`
		+ execSync(`${WASM_CC} ${WASM_CFLAGS} -msimd128 ${WASM_LDFLAGS} \
		`+ ${join(WASM_SRC, 'src')}/*.c \`
		`+ -I${join(WASM_SRC, 'include')} \`
		`+ -o ${join(WASM_OUT, 'llhttp_simd.wasm')} \`
		+ ${WASM_LDLIBS}`, { stdio: 'inherit' })
		`+ -`
		`+ -const base64WasmSimd = readFileSync(join(WASM_OUT, 'llhttp_simd.wasm')).toString('base64')`
		`+ -writeFileSync(`
		`+ - join(WASM_OUT, 'llhttp_simd-wasm.js'),`
		+ - `module.exports = '${base64WasmSimd}'\n`
		`+ -)`
		`+ diff --git a/lib/client.js b/lib/client.js`
		`+ index 22cb3903..b9e3d4a1 100644`
		`+ --- a/lib/client.js`
		`+ +++ b/lib/client.js`
		`+ @@ -7,6 +7,7 @@`
		`+ const assert = require('assert')`
		`+ const net = require('net')`
		`+ const http = require('http')`
		`+ +const fs = require('fs/promises')`
		`+ const { pipeline } = require('stream')`
		`+ const util = require('./core/util')`
		`+ const timers = require('./timers')`
		`+ @@ -489,11 +490,9 @@ const createRedirectInterceptor = require('./interceptor/redirectInterceptor')`
		`+ const EMPTY_BUF = Buffer.alloc(0)`
		`+`
		`+ async function lazyllhttp () {`
		`+ - const llhttpWasmData = process.env.JEST_WORKER_ID ? require('./llhttp/llhttp-wasm.js') : undefined`
		`+ -`
		`+ let mod`
		`+ try {`
		`+ - mod = await WebAssembly.compile(Buffer.from(require('./llhttp/llhttp_simd-wasm.js'), 'base64'))`
		`+ + mod = await WebAssembly.compile(await fs.readFile(require.resolve('./llhttp/llhttp_simd.wasm')))`
		`+ } catch (e) {`
		`+ /* istanbul ignore next */`
		`+`
		`+ @@ -501,7 +500,7 @@ async function lazyllhttp () {`
		`+ // being enabled, but the occurring of this other error`
		`+ // * https://github.com/emscripten-core/emscripten/issues/11495`
		`+ // got me to remove that check to avoid breaking Node 12.`
		`+ - mod = await WebAssembly.compile(Buffer.from(llhttpWasmData \|\| require('./llhttp/llhttp-wasm.js'), 'base64'))`
		`+ + mod = await WebAssembly.compile(await fs.readFile(require.resolve('./llhttp/llhttp.wasm.js')))`
		`+ }`
		`+`
		`+ return await WebAssembly.instantiate(mod, {`
		`+ --`
		`+ 2.43.0`
		`+`

nodejs-undici.spec

file modified

+7 -1

		`@@ -19,7 +19,8 @@`
		`Source4: %{npm_name}-sources.sh`

		`# Upstream proposal: https://github.com/nodejs/undici/pull/2403`
		`- Patch: 0001-feat-allow-customization-of-build-environment.patch`
		`+ Patch0: 0001-feat-allow-customization-of-build-environment.patch`
		`+ Patch1: 0001-perf-read-wasm-files-directly.patch`

		`# Binary artifacts in this package are aimed at the wasm32-wasi "architecture".`
		`%global _binaries_in_noarch_packages_terminate_build 0`
		`@@ -70,6 +71,11 @@`
		`mkdir -p %{buildroot}%{nodejs_sitelib}/%{npm_name}`
		`tar -C %{buildroot}%{nodejs_sitelib}/%{npm_name} -xzf %{npm_name}-%{version}.tgz --strip-components=1`
		`cp -prt %{buildroot}%{nodejs_sitelib}/%{npm_name} node_modules_prod node_modules`
		`+ cat << "EOF" > %{buildroot}%{nodejs_sitelib}/%{npm_name}/loader.js`
		`+ 'use strict'`
		`+`
		`+ module.exports = require('node:module').createRequire('%{nodejs_sitelib}/%{npm_name}/loader.js')('./index-fetch.js')`
		`+ EOF`

		`%check`
		`%{__nodejs} -e 'require("./")'`

no initial comment

rebased onto 92eca37

2 years ago

1 new commit added

Read llhttp from .wasm files instead of base64-encoded .js

2 years ago

jstanek commented 2 years ago

Hi, interesting idea; however, I do not think the Fedora package is the best place to implement it. We generally do not want to maintain behavior different from upstream, as that is generally more work than it is worth it.

Have you tried to submit this change to the upstream project? Perhaps they will be interested in it, and then everyone (not just Fedora) can benefit from it.

mochaa commented 2 years ago

This patch is Fedora-only, since upstream's main use case is bundling to a single file and embed it into the binary.

jstanek commented 2 years ago

This patch is Fedora-only, since upstream's main use case is bundling to a single file and embed it into the binary.

Right now, yes; but perhaps it would be beneficial to think about it as "generic Linux distro"-specific. Most of the changes that I propose upstream is about our use case without immediate benefit to the upstream project, but they are getting accepted anyway.

Second point, this broke because we did something different than nodejs upstream project does. Keeping this in Fedora-only leaves us in the same situation - it works now, but there is no guarantee it keeps working. Getting it at least discussed upstream may make them aware that we want to do it this way, and perhaps they try not to break it when making their changes.

mochaa commented 2 years ago

I couldn't think of a nice way to bring this to upstream. I would appreciate it if you could help!
Bringing this to upstream is harder than patching it, since we don't have a preprocessor in JavaScript.

Edited 2 years ago by mochaa

jstanek commented 2 years ago

I'll try to take a look at this closer after the weekend and see what can be done.

mochaa commented 2 years ago

Tested both PRs on my machine (nodejs20 is hex patched due to gcc14 regression? ../../deps/icu-small/source/common/uniset.cpp:1869:1: internal compiler error: in add_AT_die_ref, at dwarf2out.cc:4913), both cjs require and esm import (import statement & deferred import) work, and behavior is identical to upstream.

mochaa commented 2 years ago

I'm submitting this to upstream: https://github.com/nodejs/undici/pull/2643

mochaa commented 2 years ago

Got merged in upstream. I think it should be safe for us to backport the change here?

jstanek commented 2 years ago

Got merged in upstream. I think it should be safe for us to backport the change here?

Yes, it very much seems like it. I will take some time to wrap my head around the new info and possibilities you provided over the various PRs and come up with a plan on what we actually want to do. I might skip the esbuild mess altogether and just update and backport your change.

Thanks very much for all your work, I really appreciate it.

mochaa commented 2 years ago

This patch is essentially the same as the upstream PR. Thanks for the feedback!

p.s. I've tried replacing npx esbuild@* to esbuild in package.json and it works well with golang-github-evanw-esbuild. In this PR's case, we don't need to build the package, though.

jstanek commented 2 years ago

I have pulled the upstream changes as a patch and successfully tested it with rebuild of nodejs20. It works and loads! :tada: I pushed that to all active branches and it should be building now. Unfortunately, I did not manage to mark this PR as merged, so it will unfortunately be just closed. :disappointed: