src: refactor http parser binding initialization by joyeecheung · Pull Request #54276 · nodejs/node

joyeecheung · 2024-08-08T22:22:21Z

Use the per-isolate template to build constants and functions which is faster
Use Array::New() with prebuilt vectors which is faster
Register external references so the binding can be included in the snapshot

- Use the per-isolate template to build constants and functions which is faster - Use Array::New() with prebuilt vectors which is faster - Register external references so the binding can be included in the snapshot

nodejs-github-bot · 2024-08-08T22:22:25Z

Review requested:

@nodejs/http
@nodejs/net

codecov · 2024-08-09T05:49:45Z

Codecov Report

Attention: Patch coverage is 94.79167% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.10%. Comparing base (9e6c526) to head (c13202c).
Report is 451 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_http_parser.cc	94.79%	1 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54276      +/-   ##
==========================================
- Coverage   87.10%   87.10%   -0.01%     
==========================================
  Files         647      647              
  Lines      181754   181782      +28     
  Branches    34880    34887       +7     
==========================================
+ Hits       158323   158346      +23     
+ Misses      16742    16736       -6     
- Partials     6689     6700      +11

Files with missing lines	Coverage Δ
src/node_external_reference.h	`100.00% <ø> (ø)`
src/node_http_parser.cc	`83.11% <94.79%> (+0.09%)`	⬆️

... and 24 files with indirect coverage changes

mcollina

lgtm

ronag

Just a thought. Could we use a wasm build similar to undici?

mcollina · 2024-08-10T15:14:00Z

I'm not sure wasm is available on all platforms.

ronag · 2024-08-10T17:04:33Z

Your mean fetch (undici) doesn't work or all platforms?

nodejs-github-bot · 2024-08-10T22:29:55Z

Commit Queue failed

- Loading data for nodejs/node/pull/54276
✔  Done loading data for nodejs/node/pull/54276
----------------------------------- PR info ------------------------------------
Title      src: refactor http parser binding initialization (#54276)
Author     Joyee Cheung <[email protected]> (@joyeecheung)
Branch     joyeecheung:http-parser-binding -> nodejs:main
Labels     c++, http_parser, needs-ci
Commits    1
 - src: refactor http parser binding initialization
Committers 1
 - Joyee Cheung <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54276
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54276
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 08 Aug 2024 22:22:21 GMT
   ✔  Approvals: 5
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/54276#pullrequestreview-2229382852
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54276#pullrequestreview-2229715600
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/54276#pullrequestreview-2229828935
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/54276#pullrequestreview-2231440372
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/54276#pullrequestreview-2231464359
   ✔  Last GitHub CI successful
   ✘  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10334911205

mcollina · 2024-08-12T07:36:32Z

Your mean fetch (undici) doesn't work or all platforms?

I have a rough recall of this being a problem, or at least with some ./configure flags.

joyeecheung · 2024-08-12T12:38:10Z

I'm not sure wasm is available on all platforms.

I am guessing by that you mean wasm cannot be built on all platforms (not that a pre-built one cannot be loaded on all platforms). For the parsing on the server side I think there were some experiments with this using https://github.com/ShogunPanda/milo @ShogunPanda knows more.

nodejs-github-bot · 2024-08-12T12:39:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61053/

nodejs-github-bot · 2024-08-12T16:52:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61064/

nodejs-github-bot · 2024-08-14T12:10:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61105/

mcollina · 2024-08-14T13:06:45Z

I am guessing by that you mean wasm cannot be built on all platforms (not that a pre-built one cannot be loaded on all platforms).

@joyeecheung ah right! That was the reason.

nodejs-github-bot · 2024-08-14T21:22:00Z

Landed in 503cf16

- Use the per-isolate template to build constants and functions which is faster - Use Array::New() with prebuilt vectors which is faster - Register external references so the binding can be included in the snapshot PR-URL: #54276 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>

src: refactor http parser binding initialization

c13202c

- Use the per-isolate template to build constants and functions which is faster - Use Array::New() with prebuilt vectors which is faster - Register external references so the binding can be included in the snapshot

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. needs-ci PRs that need a full CI run. labels Aug 8, 2024

joyeecheung mentioned this pull request Aug 8, 2024

FATAL ERROR v8::FromJust Maybe value is Nothing #54186

Closed

targos approved these changes Aug 9, 2024

View reviewed changes

mcollina approved these changes Aug 9, 2024

View reviewed changes

ShogunPanda approved these changes Aug 9, 2024

View reviewed changes

aymen94 approved these changes Aug 9, 2024

View reviewed changes

addaleax approved these changes Aug 10, 2024

View reviewed changes

ronag approved these changes Aug 10, 2024

View reviewed changes

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 10, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 10, 2024

jasnell approved these changes Aug 11, 2024

View reviewed changes

joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 12, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2024

Ethan-Arrowood approved these changes Aug 12, 2024

View reviewed changes

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 14, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 14, 2024

nodejs-github-bot merged commit 503cf16 into nodejs:main Aug 14, 2024

RafaelGSS mentioned this pull request Aug 19, 2024

v22.7.0 proposal #54452

Merged

targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024

Uh oh!

Conversation

joyeecheung commented Aug 8, 2024

Uh oh!

nodejs-github-bot commented Aug 8, 2024

Uh oh!

codecov bot commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 10, 2024

Uh oh!

ronag commented Aug 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 10, 2024

Uh oh!

mcollina commented Aug 12, 2024

Uh oh!

joyeecheung commented Aug 12, 2024

Uh oh!

nodejs-github-bot commented Aug 12, 2024

Uh oh!

nodejs-github-bot commented Aug 12, 2024

Uh oh!

nodejs-github-bot commented Aug 14, 2024

Uh oh!

mcollina commented Aug 14, 2024

Uh oh!

nodejs-github-bot commented Aug 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

codecov bot commented Aug 9, 2024 •

edited

Loading

ronag commented Aug 10, 2024 •

edited

Loading