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

http: fixup perf regression #38110

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 6, 2021

/cc @mcollina

Signed-off-by: James M Snell jasnell@gmail.com

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Apr 6, 2021
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm, this does the trick.

@richardlau

This comment has been minimized.

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 6, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@mcollina
Copy link
Member

mcollina commented Apr 9, 2021

@nodejs/testing @nodejs/build this test keep failing test.parallel/test-zlib-unused-weak. This looks a lot like a flaky test or a regression - the code in the test should not be affected by this change.

@lpinca
Copy link
Member

lpinca commented Apr 9, 2021

I think it makes sense to fast-track #38149.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Only call into hrtime if there's an observer

Also, fix up some previously missed changes from the original refactor

Signed-off-by: James M Snell <jasnell@gmail.com>
Refs: nodejs#37937
Refs: nodejs#37136
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 9, 2021

@jasnell
Copy link
Member Author

jasnell commented Apr 9, 2021

YAY!!! Finally a green CI!

jasnell added a commit that referenced this pull request Apr 9, 2021
Only call into hrtime if there's an observer

Also, fix up some previously missed changes from the original refactor

Signed-off-by: James M Snell <jasnell@gmail.com>
Refs: #37937
Refs: #37136

PR-URL: #38110
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Apr 9, 2021

Landed in 40ace47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants