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: add perf_hooks detail for http request and client #43002

Closed
wants to merge 0 commits into from

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented May 7, 2022

http: add perf_hooks detail

1. http: add perf_hooks detail for http request and client
2. dns,net: move hasObserver out of startPerf and stopPerf
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Affected subsystem: http,net,dns

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels May 7, 2022
@mscdex mscdex added the needs-benchmark-ci PR that need a benchmark CI run. label May 7, 2022
@mscdex
Copy link
Contributor

mscdex commented May 7, 2022

Removing the conditionals is going to unnecessarily make more work on the GC when there are no observers because of the (inline) object being created for every request.

@theanarkh
Copy link
Contributor Author

Removing the conditionals is going to unnecessarily make more work on the GC when there are no observers because of the (inline) object being created for every request.

Thanks for your review. do you mean hasObserver ? I have moved it to startPerf and stopPerf.

@mcollina
Copy link
Member

mcollina commented May 8, 2022

Change looks good. We need the benchmarks run.

@theanarkh
Copy link
Contributor Author

theanarkh commented May 8, 2022

Change looks good. We need the benchmarks run.

Thanks for reply. I wonder what i need to do for benchmarking ?

@mscdex
Copy link
Contributor

mscdex commented May 8, 2022

Thanks for your review. do you mean hasObserver ? I have moved it to startPerf and stopPerf.

Yes. Previously object creation was guarded by an if (hasObserver(..)) but now an object (for example: the one containing type, name, and detail) is created every time whether or not the observer is active.

@theanarkh
Copy link
Contributor Author

Thanks for your review. do you mean hasObserver ? I have moved it to startPerf and stopPerf.

Yes. Previously object creation was guarded by an if (hasObserver(..)) but now an object (for example: the one containing type, name, and detail) is created every time whether or not the observer is active.

Thanks for your review. I have modified the code, please review again.

@theanarkh
Copy link
Contributor Author

@mcollina @mscdex hi, can you please help review this PR again? thanks.

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

@theanarkh
Copy link
Contributor Author

theanarkh commented May 27, 2022

I will separate this PR to two PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants