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: precise mode for monitorEventLoopDelay #32102

Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 5, 2020

Introduce precise mode of monitorEventLoopDelay by using
uv_prepare_t to calculate time difference between two
uv_prepare_t callbacks.

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 @nodejs/performance


NOTE: Moved from #32018 since github didn't let me reopen the PR after force push... 😢

Introduce precise mode of `monitorEventLoopDelay` by using
`uv_prepare_t` to calculate time difference between two
`uv_prepare_t` callbacks.
@indutny indutny requested review from jasnell and addaleax March 5, 2020 04:31
@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 Mar 5, 2020
@indutny
Copy link
Member Author

indutny commented Mar 5, 2020

@jasnell I've added a test here to check that both modes actually capture the event loop delay. Let's see if CI is happy with it.


// Since we pass `timer_` to `HandleWrap` constructor - we have to
// initialize it here. It is equally important to have it initialized for
// correct operation of `Close()` below.
Copy link
Member

Choose a reason for hiding this comment

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

If you turn the above lines into

ELDHistogram::ELDHistogram(
    Environment* env,
    Local<Object> wrap,
    int32_t resolution)
  : HandleWrap(env,
               wrap,
               resolution > 0 ?
                   reinterpret_cast<uv_handle_t*>(&timer_) :
                   reinterpret_cast<uv_handle_t*>(&prepare_),
               AsyncWrap::PROVIDER_ELDHISTOGRAM),
    Histogram(1, 3.6e12),
    resolution_(resolution) {
  ...

Then you only need to initialize one handle, and don’t need the custom ::Close() override … and, going one step further, you could even save some memory and turn the timer_/prepare_ fields into e.g.

union {
  uv_handle_t handle;
  uv_timer_t timer;
  uv_prepare_t prepare;
} handle_;

@Flarna
Copy link
Member

Flarna commented Mar 5, 2020

The problem I see with this approach is that it includes I/O time. If the event loop is completely idle it's not a big deal as no measurements are done.
But if there are callbacks every now and then (e.g. try setInterval(() => {}, 100) it looks like your event loop time is problematic.

I think this could be improved by adding the number of measurements done (Refs: #26556 (comment)) as this allows users to correlate measured delays with complete observation time.

@indutny indutny closed this Mar 5, 2020
@indutny
Copy link
Member Author

indutny commented Mar 5, 2020

@Flarna you are right 🤦

@indutny indutny deleted the feature/simplify-eventloopdelay-take-2 branch March 5, 2020 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants