★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24059Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

deps: introduce experimental `llhttp` HTTP parser #24059

Closed
wants to merge 7 commits into from
+7,253 −77

Conversation

Reviewers

@ry ry

@bnoordhuis bnoordhuis

@rvagg rvagg

@joyeecheung joyeecheung

@richardlau richardlau

@mcollina mcollina

@refack refack

@jasnell jasnell

@cjihrig cjihrig

@devsnek devsnek

@ryzokuken ryzokuken

Assignees
No one assigned
Labels
build dont-land-on-v10.x dont-land-on-v6.x dont-land-on-v8.x http_parser lib / src notable-change
Projects
None yet
Milestone
No milestone
@indutny
Copy link
Member

commented Nov 3, 2018 •
edited

llhttp is modern, written in human-readable TypeScript, verifiable, and
is very easy to maintain.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the lib / src label Nov 3, 2018

@indutny indutny requested a review from bnoordhuis Nov 3, 2018

@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 3, 2018

cc @nodejs/http
@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 3, 2018

cc @nodejs/collaborators
@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 3, 2018

It’s not clear which part are written in TypeScript. I did some experiments recently, and I found out that a major bottleneck in our http implementation are C++/JS transitions.

May I ask why this was not sent as a major contribution to http_parser?

Would you be ok in transfering this to the foundation?
@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 3, 2018

@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 3, 2018

ci: https://ci.nodejs.org/job/node-test-pull-request/18315/
@devsnek

devsnek approved these changes Nov 3, 2018
View changes

Copy link
Member

left a comment

I'm so glad this is finally happening 🎉
@mscdex

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 3, 2018

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/257/
@refack

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 3, 2018

@indutny I was wondering when will/if you'll PR this ;)
Last time you commented about this, you said it's slower then http_parser? Did you find how to optimized that?

@refack refack added the http_parser label Nov 3, 2018

@refack

refack reviewed Nov 3, 2018
View changes

Show resolved Hide resolved deps/http_parser/http_parser.gyp Outdated

@refack refack added the notable-change label Nov 3, 2018

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 3, 2018

Only trunk and upcoming 6.0.1 version of clang are supported at the moment is this a limitation of llparse, since the generated code seems to be compiling on GCC4.9?

@refack refack added the build label Nov 3, 2018

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 3, 2018

Windows compilation errors:

12:25:31   src\http_parser.c(5840): note: Conversion from 'void*' to pointer to non-'void' requires an explicit cast

12:25:31 src\http_parser.c(827): warning C4065: switch statement contains 'default' but no 'case' labels [c:\workspace\node-compile-windows\deps\http_parser\http_parser.vcxproj]

12:25:31 src\http_parser.c(6036): error C2664: 'int (http_parser_t *,const char *,const char *)': cannot convert argument 2 from 'void *' to 'const char *' [c:\workspace\node-compile-windows\deps\http_parser\http_parser.vcxproj]
12:25:31   src\http_parser.c(6036): note: Conversion from 'void*' to pointer to non-'void' requires an explicit cast
@targos

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 3, 2018

There seems to be some Windows incompatibilities.
Errors all look like that:

12:25:31 src\api.c(48): error C2440: '=': cannot convert from 'void *' to 'http_parser_settings_t *' [c:\workspace\node-compile-windows\deps\http_parser\http_parser.vcxproj]
12:25:31   src\api.c(48): note: Conversion from 'void*' to pointer to non-'void' requires an explicit cast
@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 3, 2018

@targos hopefully fixed now.
@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 3, 2018

@refack

Only trunk and upcoming 6.0.1 version of clang are supported at the moment is this a limitation of llparse, since the generated code seems to be compiling on GCC4.9?

This is no longer relevant. llparse generates very fast C code, and bitcode is no longer required for the builds.
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 3, 2018

I like the direction here and I'm definitely +1... but, I'd prefer to be a bit conservative on the approach with this... rather than removing and replacing the existing http_parser immediately, perhaps the new parser can be landed as experimental alongside and enabled with a command-line flag? e.g. --experimental-http-parser? At least through 11.x ... then, we can switch it in as the default with an --legacy-http-parser option in 12.x to use the old one, with an eye towards removing it in 13.x.
@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 3, 2018

@jasnell that would require a lot of #ifdefs. Are you okay with that?
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 3, 2018

While I'd generally prefer not to have to go that route, yes, I think it would be best... if only because of (a) how critical this particular bit of the code is for Node.js and (b) how performance and security sensitive this particular bit of code has always been.
@refack

refack reviewed Nov 3, 2018
View changes

Show resolved Hide resolved deps/http_parser/http_parser.gyp Outdated
Show resolved Hide resolved deps/http_parser/http_parser.gyp Outdated
@ry

ry reviewed Nov 3, 2018
View changes

deps/http_parser/http_parser.c Outdated
, s_res_H
, s_res_HT
, s_res_HTT
, s_res_HTTP

This comment has been minimized.

Sign in to view
Copy link
@ry

ry Nov 3, 2018

Contributor

Goodbye old friend

This comment has been minimized.

Sign in to view
Copy link
@indutny

indutny Nov 3, 2018

Author Member

@ry you're stalking me
@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 3, 2018

Alright, pushed the --experimental-http-parser configure flag. It appears to be building/running just fine, and the tests pass for both modes. What I'm not sure about is the shared_* options...

@jasnell PTAL

@indutny indutny changed the title deps: replace `http_parser` with `llhttp` deps: introduce experimental `llhttp` HTTP parser Nov 4, 2018

@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 4, 2018

New CI: https://ci.nodejs.org/job/node-test-pull-request/18322/ (testing only http_parser mode)
@refack

refack reviewed Nov 4, 2018
View changes

Show resolved Hide resolved deps/llhttp/llhttp.gyp Outdated
@refack

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 4, 2018

What I'm not sure about is the shared_* options...

IIUC node_shared_http_parser is for systems that provide their own packdged http_parser.so (Debian for example).
I believe llhttp.so is very rare to find, so I'd say --experimental-http-parser overrides --shared_http_parser. You could put an "if (both): exit configure.py" guard.

BridgeAR added a commit that referenced this pull request Nov 15, 2018

@BridgeAR
2018-11-15, Version 11.2.0 (Current) 
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    [#24059](#24059)
* util:
  * The `util.inspect` `depth` option default is reset to 2.
    [#24326](#24326)
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    [#23822](#23822)
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    [#23987](#23987),
    [#24348](#24348)
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      [#24034](https.//github.com//pull/24034)
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    [#24300](#24300)
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    [#24170](#24170)
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    [#24136](#24136)
Unverified
The committer email address is not verified.
Learn about signing commits
Loading status checks…
e7a0250

BridgeAR added a commit that referenced this pull request Nov 15, 2018

@BridgeAR
2018-11-15, Version 11.2.0 (Current) 
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    [#24059](#24059)
* util:
  * The `util.inspect` `depth` option default is reset to 2.
    [#24326](#24326)
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected.
    [#24322](#24322)
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    [#23822](#23822)
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    [#23987](#23987),
    [#24348](#24348)
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      [#24034](https.//github.com//pull/24034)
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    [#24300](#24300)
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    [#24170](#24170)
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    [#24136](#24136)
Unverified
The committer email address is not verified.
Learn about signing commits
Loading status checks…
31166a0

BridgeAR added a commit that referenced this pull request Nov 15, 2018

@BridgeAR
2018-11-15, Version 11.2.0 (Current) 
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* util:
  * The `util.inspect` `depth` option default is reset to 2.
    #24326
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136
Unverified
The committer email address is not verified.
Learn about signing commits
Loading status checks…
911ad9c

BridgeAR added a commit that referenced this pull request Nov 15, 2018

@BridgeAR
2018-11-15, Version 11.2.0 (Current) 
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136
Unverified
The committer email address is not verified.
Learn about signing commits
Loading status checks…
de843d1

targos added a commit that referenced this pull request Nov 15, 2018

@BridgeAR @targos
2018-11-15, Version 11.2.0 (Current) 
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
aa8a34b

targos added a commit that referenced this pull request Nov 15, 2018

@BridgeAR @targos
2018-11-15, Version 11.2.0 (Current) 
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
a19e1ab

targos added a commit that referenced this pull request Nov 15, 2018

@BridgeAR @targos
2018-11-15, Version 11.2.0 (Current) 
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136

PR-URL: #24350
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
406d9ca

This was referenced Nov 15, 2018

Closed
Closed
Closed
Closed
Closed
Closed
@kibertoad

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 17, 2018

@indutny How does one opt-in into using it? Or it has to be consumed on a library level, e. g. from Express.js side?
@targos

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 17, 2018

@kibertoad it is currently a build-time flag, so you have to compile Node by passing the --experimental-http-parser flag to ./configure.
When enabled, it replaces the HTTP parser entirely.
@zuohuadong

This comment has been minimized.

Sign in to view
Copy link

commented Nov 21, 2018 •
edited

love it!!

Can I use it by default in the upcoming Node.js 12?
@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 5, 2018

PR in progress to make this a runtime flag: #24739
@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 5, 2018 •
edited

Should this be dont-land-on-v10.x and dont-land-on-v11.x, BTW?
@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 5, 2018

+1
@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 5, 2018

@joyeecheung +1
@targos

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 5, 2018

Too late for 11.x. This is already in v11. 2.0

@mcollina mcollina added dont-land-on-v6.x dont-land-on-v8.x dont-land-on-v10.x labels Dec 5, 2018

@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 5, 2018

@targos oops, I meant 8.x, somehow my fingers were not controlled by my brain..
@tuananh

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Dec 7, 2018 •
edited

IMHO ~0 net effect on performance.

why do node decide to adopt this then?
@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 7, 2018

@tuananh this section answers it: https://github.com/indutny/llhttp#why
@tuananh

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Dec 7, 2018

@indutny by adopting llhttp, isn't it gonna be adding more maintenance responsibility as we need to maintain both llparse and llhttp?
@indutny

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 7, 2018

Good point. Still both libraries are way more maintainable than http_parser in its current form.
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 7, 2018

@tuananh See also the discussion in #24730 – adopting llhttp as the default parser in Node 12 may give us the freedom to abandon http_parser at some point in its lifetime, if we decide that maintaining http_parser is unreasonable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.