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

perf_hooks: reset prev_ before starting ELD timer #26693

Closed
wants to merge 1 commit into from
Closed

perf_hooks: reset prev_ before starting ELD timer #26693

wants to merge 1 commit into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Mar 15, 2019

reset ELDHistogram.prev_ before staring timer to ensure that start
timer doesn't leak across disable() enable() calls.

Checklist
  • 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

@jasnell I have seen this nit late during review of #26556 and it's not directly related to your changes. But feel free to integrate it into your PR to avoid a later on merge conflict.

reset `ELDHistogram.prev_` before staring timer to ensure that start
timer doesn't leak across `disable()` `enable()` calls.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 15, 2019
@jasnell
Copy link
Member

jasnell commented Mar 16, 2019

Doh! Good catch

@richardlau
Copy link
Member

Can this be tested for?

@Flarna
Copy link
Member Author

Flarna commented Mar 16, 2019

@richardlau sure but timeout based test tend to be flaky - or take a long time to have a good security margin.
Currently the whole ELD functionality does not testing the actual values just the presence of them.

@jasnell
Copy link
Member

jasnell commented Mar 16, 2019 via email

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2019
ZYSzys pushed a commit to zys-contrib/node that referenced this pull request Mar 20, 2019
reset `ELDHistogram.prev_` before staring timer to ensure that start
timer doesn't leak across `disable()` `enable()` calls.

PR-URL: nodejs#26693
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
@ZYSzys
Copy link
Member

ZYSzys commented Mar 20, 2019

Landed in 614a747 🎉

@ZYSzys ZYSzys closed this Mar 20, 2019
@Flarna Flarna deleted the perf_hooks_reset_prev branch March 20, 2019 07:03
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
reset `ELDHistogram.prev_` before staring timer to ensure that start
timer doesn't leak across `disable()` `enable()` calls.

PR-URL: nodejs#26693
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
targos pushed a commit that referenced this pull request Mar 27, 2019
reset `ELDHistogram.prev_` before staring timer to ensure that start
timer doesn't leak across `disable()` `enable()` calls.

PR-URL: #26693
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yongsheng Zhang <[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. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants