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

benchmark: fix http bench-parser.js #27359

Merged
merged 1 commit into from
Apr 23, 2019
Merged

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 23, 2019

The internal HTTParser reinitialize() function was removed in
ece5073 and replaced with an initialize() function. This broke
benchmark/http/bench-parser.js. This change updates the benchmark so
that it runs again.

/ping @nodejs/async_hooks @Drieger to make sure this is the correct change here.... If so, I would like to fast-track because this issue breaks the nightly benchmark tests on Jenkins.

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem. labels Apr 23, 2019
@Flarna Flarna mentioned this pull request Apr 23, 2019
3 tasks
@refack
Copy link
Contributor

refack commented Apr 23, 2019

CI: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/6412/ (filed with a different test as expected)

@refack refack added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 23, 2019
@refack
Copy link
Contributor

refack commented Apr 23, 2019

We should fast-track this when it passes CI testing.
Please 👍 if you concur.

@BethGriggs BethGriggs mentioned this pull request Apr 23, 2019
The internal HTTParser `reinitialize()` function was removed in
ece5073 and replaced with an `initialize()` function. This broke
benchmark/http/bench-parser.js. This change updates the benchmark so
that it runs again.

PR-URL: nodejs#27359
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@refack
Copy link
Contributor

refack commented Apr 23, 2019

Landed in 62c7b2e
This fixes the test, if the benchmark is wrong it could be fixed in a follow up PR.

@refack refack merged commit 62c7b2e into nodejs:master Apr 23, 2019
targos pushed a commit that referenced this pull request Apr 27, 2019
The internal HTTParser `reinitialize()` function was removed in
ece5073 and replaced with an `initialize()` function. This broke
benchmark/http/bench-parser.js. This change updates the benchmark so
that it runs again.

PR-URL: #27359
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Apr 27, 2019
@Trott Trott deleted the fix-parser-bench branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. fast-track PRs that do not need to wait for 48 hours to land. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants