-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
dgram: improve get-send-queue-info performance #59054
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
base: main
Are you sure you want to change the base?
Conversation
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59054 +/- ##
==========================================
- Coverage 88.54% 88.53% -0.01%
==========================================
Files 704 704
Lines 207843 207863 +20
Branches 40044 40049 +5
==========================================
+ Hits 184028 184035 +7
+ Misses 15858 15856 -2
- Partials 7957 7972 +15
🚀 New features to boost your workflow:
|
| bench.start(); | ||
|
|
||
| if (method === 'size') { | ||
| for (let i = 0; i < n; ++i) server.getSendQueueSize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at some point this will be JIT-eliminated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that getSendQueueSize() will not be JIT eliminated because it still calls into libuv. Im using size and count now to accumulate the results in order to make that explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind, I was looking to HEAD.
0f7af8e to
7f9c6e8
Compare
Commit Queue failed- Loading data for nodejs/node/pull/59054 ✔ Done loading data for nodejs/node/pull/59054 ----------------------------------- PR info ------------------------------------ Title dgram: improve get-send-queue-info performance (#59054) Author Ilyas Shabi <[email protected]> (@IlyasShabi) Branch IlyasShabi:ishabi/get-send-queue-info -> nodejs:main Labels dgram, c++, author ready, needs-ci Commits 3 - dgram: improve get-send-queue-info performance - keep packets unsent to have data in getters - fix benchmark test Committers 1 - ishabi <[email protected]> PR-URL: https://github.com/nodejs/node/pull/59054 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: James M Snell <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/59054 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: James M Snell <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Sun, 13 Jul 2025 22:32:01 GMT ✔ Approvals: 2 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/59054#pullrequestreview-3031186521 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/59054#pullrequestreview-3038885029 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-07-17T21:21:09Z: https://ci.nodejs.org/job/node-test-pull-request/67989/ ⚠ Commits were pushed after the last Full PR CI run: ⚠ - dgram: improve get-send-queue-info performance ⚠ - keep packets unsent to have data in getters ⚠ - fix benchmark test - Querying data for job/node-test-pull-request/67989/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/16423922247 |
7f9c6e8 to
0d00b58
Compare
H4ad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be safer to test on fastApi as well, would you mind adding a test for it? You can use this one as example: https://github.com/nodejs/node/blob/cd0c5159e64353bc057c5704767145b46887c735/test/parallel/test-os-fast.js
Just a little reminder, you need to wrap your function during the test (
testFastOsin the example above) to pass that function to the v8 optimize, it fails if you try to optimize your new method directly.
|
You also need to update the first commit message to reflect the new change: add fast api for get-send-queue-size method |
0d00b58 to
b249db8
Compare
54fbffa to
780bd7e
Compare
|
Benchmark CI: https://github.com/aduh95/node/actions/runs/18478104954 |
|
good work |
780bd7e to
a78fd4b
Compare
a78fd4b to
00ceec3
Compare
00ceec3 to
2e55992
Compare
2e55992 to
49d0d65
Compare
Improve
dgramgetSendQueueSizeandgetSendQueueCountperformance by using V8 fast API.Local benchmark: