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

net,dns:move hasObserver out of perf function #43217

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented May 27, 2022

move the hasObserver out of startPerf and stopPerf
to avoid generating useless objects when these are no observer.

See #43002

  • 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: net,dns

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added dns Issues and PRs related to the dns subsystem. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels May 27, 2022
@theanarkh theanarkh force-pushed the move_hasObserver_out_of_perf_function branch from 689ad95 to 7cb6990 Compare May 27, 2022 01:40
@theanarkh
Copy link
Contributor Author

@mcollina Hello, can you help review this PR? 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 theanarkh force-pushed the move_hasObserver_out_of_perf_function branch 2 times, most recently from 896bf69 to 18b93f8 Compare May 31, 2022 17:59
move the hasObserver out of startPerf and stopPerf
to avoid generating useless objects when these are no observer
@theanarkh theanarkh force-pushed the move_hasObserver_out_of_perf_function branch from 18b93f8 to 42c6e46 Compare June 1, 2022 04:39
@theanarkh
Copy link
Contributor Author

@mcollina @JungMinu Hi, can you please help trigger CI ? 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

lgtm

Thanks. can you please help trigger CI ?

@nodejs-github-bot

This comment was marked as outdated.

@theanarkh
Copy link
Contributor Author

@mcollina @JungMinu Hi, can you please help trigger CI again ? thanks !

@nodejs-github-bot

This comment was marked as outdated.

@ShogunPanda
Copy link
Contributor

@theanarkh I took care of it. Enjoy :)

@theanarkh
Copy link
Contributor Author

@theanarkh I took care of it. Enjoy :)

Thank you very much 😊.

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 3, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@aduh95
Copy link
Contributor

aduh95 commented Jun 5, 2022

test-net-connect-reset-until-connected is consistently failing on SmartOS CI (timeout).

@theanarkh
Copy link
Contributor Author

@RaisinTen @aduh95 @ShogunPanda @mcollina @JungMinu Hi, can you please help merge this PR ? Thanks !

@RaisinTen
Copy link
Contributor

RaisinTen commented Jun 9, 2022

test-net-connect-reset-until-connected is consistently failing on SmartOS CI (timeout).

That one seems to be passing in the latest CI run - https://ci.nodejs.org/job/node-test-pull-request/44373/, landing now.

@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 9, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 9, 2022
@nodejs-github-bot nodejs-github-bot merged commit 85f8821 into nodejs:master Jun 9, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 85f8821

danielleadams pushed a commit that referenced this pull request Jun 11, 2022
move the hasObserver out of startPerf and stopPerf
to avoid generating useless objects when these are no observer

PR-URL: #43217
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@danielleadams danielleadams mentioned this pull request Jun 11, 2022
danielleadams pushed a commit that referenced this pull request Jun 13, 2022
move the hasObserver out of startPerf and stopPerf
to avoid generating useless objects when these are no observer

PR-URL: #43217
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 13, 2022
move the hasObserver out of startPerf and stopPerf
to avoid generating useless objects when these are no observer

PR-URL: #43217
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
move the hasObserver out of startPerf and stopPerf
to avoid generating useless objects when these are no observer

PR-URL: #43217
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
move the hasObserver out of startPerf and stopPerf
to avoid generating useless objects when these are no observer

PR-URL: #43217
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
move the hasObserver out of startPerf and stopPerf
to avoid generating useless objects when these are no observer

PR-URL: nodejs/node#43217
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
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. dns Issues and PRs related to the dns subsystem. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants