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

deps: introduce experimental llhttp HTTP parser #24059

Closed
wants to merge 7 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Nov 3, 2018

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 added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 3, 2018
@indutny
Copy link
Member Author

indutny commented Nov 3, 2018

cc @nodejs/http

@indutny
Copy link
Member Author

indutny commented Nov 3, 2018

cc @nodejs/collaborators

@mcollina
Copy link
Member

mcollina 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
Copy link
Member Author

indutny commented Nov 3, 2018 via email

@joyeecheung
Copy link
Member

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

I'm so glad this is finally happening 🎉

@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2018

@refack
Copy link
Contributor

refack 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 Issues and PRs related to the HTTP Parser dependency or the http_parser binding. label Nov 3, 2018
@refack refack added the notable-change PRs with changes that should be highlighted in changelogs. label Nov 3, 2018
@refack
Copy link
Contributor

refack 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 Issues and PRs related to build files or the CI. label Nov 3, 2018
@refack
Copy link
Contributor

refack 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
Copy link
Member

targos 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
Copy link
Member Author

indutny commented Nov 3, 2018

@targos hopefully fixed now.

@indutny
Copy link
Member Author

indutny 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
Copy link
Member

jasnell 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
Copy link
Member Author

indutny commented Nov 3, 2018

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

@jasnell
Copy link
Member

jasnell 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.

deps/http_parser/http_parser.gyp Outdated Show resolved Hide resolved
deps/http_parser/http_parser.gyp Outdated Show resolved Hide resolved
, s_res_H
, s_res_HT
, s_res_HTT
, s_res_HTTP
Copy link
Contributor

Choose a reason for hiding this comment

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

Goodbye old friend

Copy link
Member Author

Choose a reason for hiding this comment

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

@ry you're stalking me

@indutny
Copy link
Member Author

indutny 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
Copy link
Member Author

indutny commented Nov 4, 2018

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

deps/llhttp/llhttp.gyp Outdated Show resolved Hide resolved
@refack
Copy link
Contributor

refack 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.

targos pushed a commit that referenced this pull request Nov 15, 2018
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
targos pushed a commit that referenced this pull request Nov 15, 2018
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
@kibertoad
Copy link
Contributor

@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
Copy link
Member

targos 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
Copy link

zuohuadong commented Nov 21, 2018

love it!!

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

@joyeecheung
Copy link
Member

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

@joyeecheung
Copy link
Member

joyeecheung commented Dec 5, 2018

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

@mcollina
Copy link
Member

mcollina commented Dec 5, 2018

+1

@indutny
Copy link
Member Author

indutny commented Dec 5, 2018

@joyeecheung +1

@targos
Copy link
Member

targos commented Dec 5, 2018

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

@joyeecheung
Copy link
Member

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

@tuananh
Copy link
Contributor

tuananh commented Dec 7, 2018

IMHO ~0 net effect on performance.

why do node decide to adopt this then?

@indutny
Copy link
Member Author

indutny commented Dec 7, 2018

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

@tuananh
Copy link
Contributor

tuananh 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
Copy link
Member Author

indutny commented Dec 7, 2018

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

@addaleax
Copy link
Member

addaleax 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
Labels
build Issues and PRs related to build files or the CI. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. lib / src Issues and PRs related to general changes in the lib or src directory. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.