Skip to content

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 addaleax and jasnell 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