Skip to content

src,lib: add performance.uvMetricsInfo#54413

Merged
nodejs-github-bot merged 9 commits into
nodejs:mainfrom
RafaelGSS:add-uv-metrics-loop
Aug 30, 2024
Merged

src,lib: add performance.uvMetricsInfo#54413
nodejs-github-bot merged 9 commits into
nodejs:mainfrom
RafaelGSS:add-uv-metrics-loop

Conversation

@RafaelGSS

Copy link
Copy Markdown
Member

This commit exposes a new API to the perf_hooks.performance module. This wraps uv_metrics_info into
performance.uvMetricsInfo() function.

cc: @trevnorris

This commit exposes a new API to the perf_hooks.performance
module. This wraps uv_metrics_info into
performance.uvMetricsInfo() function.
@RafaelGSS RafaelGSS added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 16, 2024
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 16, 2024

@trevnorris trevnorris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I really appreciate you doing this. Thanks very much.

Comment thread doc/api/perf_hooks.md Outdated
Comment thread doc/api/perf_hooks.md Outdated
@avivkeller avivkeller added the perf_hooks Issues and PRs related to the implementation of the Performance Timing API. label Aug 16, 2024
@RafaelGSS
RafaelGSS requested a review from trevnorris August 16, 2024 20:49

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

Comment thread doc/api/perf_hooks.md Outdated
Comment thread doc/api/perf_hooks.md Outdated
Comment thread doc/api/perf_hooks.md Outdated
Comment thread doc/api/perf_hooks.md Outdated
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
@RafaelGSS RafaelGSS added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 17, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 17, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented Aug 17, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.31%. Comparing base (123693c) to head (0e9cf0e).
Report is 155 commits behind head on main.

Files with missing lines Patch % Lines
src/node_perf.cc 95.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54413      +/-   ##
==========================================
+ Coverage   87.10%   87.31%   +0.21%     
==========================================
  Files         648      649       +1     
  Lines      182215   182783     +568     
  Branches    34955    35046      +91     
==========================================
+ Hits       158712   159606     +894     
+ Misses      16789    16465     -324     
+ Partials     6714     6712       -2     
Files with missing lines Coverage Δ
lib/internal/perf/nodetiming.js 94.64% <100.00%> (+0.26%) ⬆️
src/node_perf.cc 86.83% <95.00%> (+0.73%) ⬆️

... and 117 files with indirect coverage changes

@H4ad H4ad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with some non-blocking comments.

Comment thread lib/internal/perf/uv_metrics.js Outdated
Comment thread src/node_perf.cc
Comment on lines +271 to +283
Local<Object> obj = Object::New(env->isolate());
obj->Set(env->context(),
env->loop_count(),
Integer::NewFromUnsigned(env->isolate(), metrics.loop_count))
.Check();
obj->Set(env->context(),
env->events(),
Integer::NewFromUnsigned(env->isolate(), metrics.events))
.Check();
obj->Set(env->context(),
env->events_waiting(),
Integer::NewFromUnsigned(env->isolate(), metrics.events_waiting))
.Check();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a note for the future optimization: maybe we should use a buffer to store these values to avoid creating and returning js objects from c++.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I have plans to return a typed array and build the object in JS land. I'll do it in a follow up PR.

Comment thread test/parallel/test-performance-uvmetricsinfo.js Outdated
@RafaelGSS
RafaelGSS requested review from H4ad and tniessen August 20, 2024 16:38
@RafaelGSS
RafaelGSS force-pushed the add-uv-metrics-loop branch from 6654a2b to c1385ed Compare August 20, 2024 16:38
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@RafaelGSS RafaelGSS mentioned this pull request Aug 30, 2024
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++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. 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

Development

Successfully merging this pull request may close these issues.