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.

Sign up for GitHub

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

Jump to bottom

perf_hooks: introduce monitorEventLoopIdleness #31992

Closed
wants to merge 2 commits into from
Closed

perf_hooks: introduce monitorEventLoopIdleness #31992

wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Feb 28, 2020

perf_hooks.monitorEventLoopIdleness tracks the time event loop has
spent waiting for new IO events (i.e. difference between IO poll start
time and IO poll end time). The API is identical to
monitorEventLoopDelay.

NOTE: Some of the trace event names had to be renamed for consistency.

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

cc @jasnell

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. labels Feb 28, 2020
@indutny indutny requested review from jasnell, cjihrig and BridgeAR and removed request for jasnell February 28, 2020 02:17
@indutny
Copy link
Member Author

indutny commented Feb 28, 2020

NOTE: might need a rebase over #31988 once it lands

@addaleax
Copy link
Member

NOTE: might need a rebase over #31988 once it lands

Yeah, I would suggest rebasing it now – it looks like the difference between now and after the rebase would be substantial…

addaleax
addaleax reviewed Feb 28, 2020
View reviewed changes
src/node_perf.cc Outdated Show resolved Hide resolved
@jasnell @indutny
perf,src: add HistogramBase and internal/histogram.js
7675d8e 
Separating this out from the QUIC PR to allow it to be separately
reviewed. The QUIC implementation makes use of the hdr_histogram
for dynamic performance monitoring. This introduces a BaseObject
class that allows the internal histograms to be accessed on the
JavaScript side and adds a generic Histogram class that will be
used by both QUIC and perf_hooks (for the event loop delay
monitoring).

Signed-off-by: James M Snell <jasnell@gmail.com>
@indutny indutny force-pushed the feature/idle-histogram branch 2 times, most recently from 1caf8ed to 6eca3b9 Compare February 28, 2020 03:00
@indutny
Copy link
Member Author

indutny commented Feb 28, 2020

Force-pushed after rebase over #31988. Note that I haven't used HistogramBase since it is incompatible with inheriting from HandleWrap... Unless I am mistaken?

@addaleax
Copy link
Member

Note that I haven't used HistogramBase since it is incompatible with inheriting from HandleWrap... Unless I am mistaken?

ELDHistogram does it (both before and after #31988), so I’m fairly certain that it should work?

@indutny
Copy link
Member Author

indutny commented Feb 28, 2020

@addaleax it does not as far as I can see. HistogramBase was introduced in #31988 so it couldn't have been used before it.

@addaleax
Copy link
Member

Ah sorry, for a moment I thought Histogram inherited from HistogramBase because of the naming ;) Yes, I think it’s currently not possible to use HistogramBase for this.

@indutny
Copy link
Member Author

indutny commented Feb 28, 2020

No problem at all. Perhaps, @jasnell might want to salvage some parts of EventLoopHistogramBinding to make HistogramBase more useful. It is slightly easier to do this with templates.

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 28, 2020
@Trott
Copy link
Member

Trott commented Feb 28, 2020

@nodejs/performance

@indutny
perf_hooks: introduce monitorEventLoopIdleness
2737598 
`perf_hooks.monitorEventLoopIdleness` tracks the time event loop has
spent waiting for new IO events (i.e. difference between IO poll start
time and IO poll end time). The API is identical to
`monitorEventLoopDelay`.

NOTE: Some of the trace event names had to be renamed for consistency.
@indutny indutny force-pushed the feature/idle-histogram branch from 6eca3b9 to 2737598 Compare February 28, 2020 03:46
@jasnell
Copy link
Member

jasnell commented Feb 28, 2020

HistogramBase is so named because it derives from BaseObject. I'm using it in the QUIC impl to record a few different stats but it doesn't have it's own handle.

ELDHistogram derives from HandleWrap because of the timer handle.

We can, and likely should come up with better naming but the two are definitely distinct from one another.

@indutny
Copy link
Member Author

indutny commented Feb 28, 2020

@jasnell Ah, that's good to know. In this case this PR is ready for review once #31988 is landed.

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label Feb 28, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29426/

@indutny
Copy link
Member Author

indutny commented Feb 28, 2020

I had to sleep on it to realize that it is completely complementary to the ELDHistogram, as in the sum of two would always be equal the total time of the loop iteration. Considering this I think that introduction of idle histogram is pointless 😢 Sorry for bothering y'all!

@indutny indutny closed this Feb 28, 2020
@indutny
Copy link
Member Author

indutny commented Feb 28, 2020

Somewhat tangential to this: I think uv_check_t + uv_prepare_t can be used to monitor event loop delay too without having to rely on the timer precision. The loop watchers (uv_prepare_t and uv_check_t) are prepended to the appropriate lists in libuv: newly added watchers will be invoked before the old ones. Do you think I should explore it?

cc @jasnell

@jasnell
Copy link
Member

jasnell commented Feb 29, 2020

If the accuracy would be better or about the same, and if the implementation is straightforward, then worth exploring.

@indutny
Copy link
Member Author

indutny commented Feb 29, 2020

Created a PR: #32018

@mmarchini mmarchini mentioned this pull request Mar 2, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@cjihrig cjihrig Awaiting requested review from cjihrig

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. c++ Issues and PRs that require attention from people who are familiar with C++. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@indutny @addaleax @Trott @jasnell @nodejs-github-bot