dgram: fix possibly deoptimizing use of arguments - #11242
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
Are you seeing performance improvements? Can you display the benchmarking results in a more readable way? This is a lot of text to jump back and forth in. |
Sorry, something went wrong.
|
@cjihrig Sorry, I am not very good at Node.js benchmarks and I has not Rscript on my machine to get the proper comparison. Could I present results in some another way? I don't think we could see any performance improvements in common cases, and I am not a heavy user of dgram module to imagine valid use cases to see these performance improvements. Maybe, somebody else could? FWIW, this is some weird test example (partly taken from socket.bind() doc) to get deopt: const dgram = require('dgram');
for (let i = 0; i < 1000; i++) {
const server = dgram.createSocket('udp4');
server.on('listening', () => {
const address = server.address();
console.log(`server listening ${address.address}:${address.port}`);
});
server.bind();
}node --trace_opt --trace_deopt test.js | findstr bind Before the fixes: [marking 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> for optimized recompilation, reason: hot and stable, ICs with typeinfo: 15/34 (44%), generic ICs: 3/34 (8%)]
[compiling method 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> using Crankshaft]
[optimizing 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> - took 0.000, 0.000, 0.000 ms]
[completed optimizing 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)>]
[deoptimizing (DEOPT eager): begin 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> (opt #9) @16, FP to SP delta: 68, caller sp: 0x0038ee2c]
reading input frame Socket.bind => node=120, args=2, height=5; inputs:
0: 0x19b90a19 ; [fp - 8] 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)>
translating frame Socket.bind => node=120, height=16
0x0038ee14: [top + 16] <- 0x19b90a19 ; function 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> (input #0)
[deoptimizing (eager): end 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> @16 => node=120, pc=0x344c4dce, caller sp=0x0038ee2c, state=TOS_REGISTER, took 16.000 ms]
[removing optimized code for: Socket.bind]
[evicting entry from optimizing code map (notify deoptimized) for 19B8E465 <SharedFunctionInfo Socket.bind>]
[marking 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> for optimized recompilation, reason: hot and stable, ICs with typeinfo: 15/34 (44%), generic ICs: 3/34 (8%)]
[compiling method 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> using Crankshaft]
[optimizing 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> - took 0.000, 0.000, 0.000 ms]
[completed optimizing 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)>]
[deoptimizing (DEOPT eager): begin 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> (opt #10) @16, FP to SP delta: 68, caller sp: 0x0038ee2c]
reading input frame Socket.bind => node=120, args=2, height=5; inputs:
0: 0x19b90a19 ; [fp - 8] 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)>
translating frame Socket.bind => node=120, height=16
0x0038ee14: [top + 16] <- 0x19b90a19 ; function 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> (input #0)
[deoptimizing (eager): end 19B90A19 <JS Function Socket.bind (SharedFunctionInfo 19B8E465)> @16 => node=120, pc=0x344c4dce, caller sp=0x0038ee2c, state=TOS_REGISTER, took 0.000 ms]
[removing optimized code for: Socket.bind]
[evicting entry from optimizing code map (notify deoptimized) for 19B8E465 <SharedFunctionInfo Socket.bind>]
After the fixes: [marking 3E110A19 <JS Function Socket.bind (SharedFunctionInfo 3E10E465)> for optimized recompilation, reason: hot and stable, ICs with typeinfo: 17/38 (44%), generic ICs: 0/38 (0%)] [compiling method 3E110A19 <JS Function Socket.bind (SharedFunctionInfo 3E10E465)> using Crankshaft] [optimizing 3E110A19 <JS Function Socket.bind (SharedFunctionInfo 3E10E465)> - took 0.000, 0.000, 0.000 ms] [completed optimizing 3E110A19 <JS Function Socket.bind (SharedFunctionInfo 3E10E465)>] [deoptimizer unlinked: Socket.bind / 3e110a19] [evicting entry from optimizing code map (deoptimized code) for 3E10E465 <SharedFunctionInfo Socket.bind>] |
Sorry, something went wrong.
|
I haven't benchmarked, but you could do this to avoid using arguments in the address calculation. Using arguments for the callback like you currently are seems like the simplest way to go, since it can be in a number of locations. |
Sorry, something went wrong.
|
@cjihrig Should I amend with address_ ? Sorry, could you explain the second sentence some more, please? |
Sorry, something went wrong.
|
Sorry, I was just saying, we could use _address instead of arguments if benchmarking shows it's OK. The second sentence was basically that your first change on line 145 LGTM. |
Sorry, something went wrong.
|
@cjihrig I've amended the second change, build and tests pass. I've installed R and have launched the node benchmark/compare.js ... dgram ... phase. It proceeds very slowly (5% only for ~40 min). Maybe we could run another CI meanwhile? |
Sorry, something went wrong.
Benchmarks results:
improvement confidence p.value dgram\\array-vs-concat.js dur=5 type="concat" chunks=1 num=100 len=1024 -0.07 % 0.819142606 dgram\\array-vs-concat.js dur=5 type="concat" chunks=1 num=100 len=256 0.65 % * 0.029980094 dgram\\array-vs-concat.js dur=5 type="concat" chunks=1 num=100 len=512 0.53 % 0.105329248 dgram\\array-vs-concat.js dur=5 type="concat" chunks=1 num=100 len=64 -0.15 % 0.539509788 dgram\\array-vs-concat.js dur=5 type="concat" chunks=2 num=100 len=1024 -0.04 % 0.877992589 dgram\\array-vs-concat.js dur=5 type="concat" chunks=2 num=100 len=256 -0.42 % 0.224659267 dgram\\array-vs-concat.js dur=5 type="concat" chunks=2 num=100 len=512 -0.35 % 0.200748080 dgram\\array-vs-concat.js dur=5 type="concat" chunks=2 num=100 len=64 -0.55 % 0.123158638 dgram\\array-vs-concat.js dur=5 type="concat" chunks=4 num=100 len=1024 -0.31 % 0.241236440 dgram\\array-vs-concat.js dur=5 type="concat" chunks=4 num=100 len=256 -0.00 % 0.999443074 dgram\\array-vs-concat.js dur=5 type="concat" chunks=4 num=100 len=512 -0.14 % 0.592581146 dgram\\array-vs-concat.js dur=5 type="concat" chunks=4 num=100 len=64 -0.84 % * 0.021566262 dgram\\array-vs-concat.js dur=5 type="concat" chunks=8 num=100 len=1024 -0.33 % 0.236997003 dgram\\array-vs-concat.js dur=5 type="concat" chunks=8 num=100 len=256 0.02 % 0.943577183 dgram\\array-vs-concat.js dur=5 type="concat" chunks=8 num=100 len=512 -0.44 % 0.093730052 dgram\\array-vs-concat.js dur=5 type="concat" chunks=8 num=100 len=64 -0.28 % 0.289756919 dgram\\array-vs-concat.js dur=5 type="multi" chunks=1 num=100 len=1024 0.44 % 0.060660498 dgram\\array-vs-concat.js dur=5 type="multi" chunks=1 num=100 len=256 0.25 % 0.320558233 dgram\\array-vs-concat.js dur=5 type="multi" chunks=1 num=100 len=512 0.39 % 0.177161838 dgram\\array-vs-concat.js dur=5 type="multi" chunks=1 num=100 len=64 0.36 % 0.182711880 dgram\\array-vs-concat.js dur=5 type="multi" chunks=2 num=100 len=1024 0.12 % 0.613099185 dgram\\array-vs-concat.js dur=5 type="multi" chunks=2 num=100 len=256 0.19 % 0.504940429 dgram\\array-vs-concat.js dur=5 type="multi" chunks=2 num=100 len=512 0.12 % 0.678952500 dgram\\array-vs-concat.js dur=5 type="multi" chunks=2 num=100 len=64 -0.16 % 0.459314273 dgram\\array-vs-concat.js dur=5 type="multi" chunks=4 num=100 len=1024 0.53 % 0.088714998 dgram\\array-vs-concat.js dur=5 type="multi" chunks=4 num=100 len=256 0.72 % * 0.030422914 dgram\\array-vs-concat.js dur=5 type="multi" chunks=4 num=100 len=512 0.40 % 0.184751838 dgram\\array-vs-concat.js dur=5 type="multi" chunks=4 num=100 len=64 0.29 % 0.320490675 dgram\\array-vs-concat.js dur=5 type="multi" chunks=8 num=100 len=1024 0.40 % 0.135630861 dgram\\array-vs-concat.js dur=5 type="multi" chunks=8 num=100 len=256 0.42 % 0.093813494 dgram\\array-vs-concat.js dur=5 type="multi" chunks=8 num=100 len=512 0.25 % 0.303448782 dgram\\array-vs-concat.js dur=5 type="multi" chunks=8 num=100 len=64 0.22 % 0.333867175 dgram\\multi-buffer.js dur=5 type="recv" chunks=1 num=100 len=1024 -0.52 % 0.051782242 dgram\\multi-buffer.js dur=5 type="recv" chunks=1 num=100 len=256 -0.25 % 0.200875743 dgram\\multi-buffer.js dur=5 type="recv" chunks=1 num=100 len=64 -0.36 % 0.156919544 dgram\\multi-buffer.js dur=5 type="recv" chunks=2 num=100 len=1024 -0.65 % ** 0.009810655 dgram\\multi-buffer.js dur=5 type="recv" chunks=2 num=100 len=256 -0.32 % 0.216226970 dgram\\multi-buffer.js dur=5 type="recv" chunks=2 num=100 len=64 0.00 % 0.998397455 dgram\\multi-buffer.js dur=5 type="recv" chunks=4 num=100 len=1024 -0.30 % 0.293122198 dgram\\multi-buffer.js dur=5 type="recv" chunks=4 num=100 len=256 -0.30 % 0.369189963 dgram\\multi-buffer.js dur=5 type="recv" chunks=4 num=100 len=64 0.34 % 0.394163637 dgram\\multi-buffer.js dur=5 type="recv" chunks=8 num=100 len=1024 0.05 % 0.854895663 dgram\\multi-buffer.js dur=5 type="recv" chunks=8 num=100 len=256 0.45 % 0.070950431 dgram\\multi-buffer.js dur=5 type="recv" chunks=8 num=100 len=64 0.55 % 0.053218194 dgram\\multi-buffer.js dur=5 type="send" chunks=1 num=100 len=1024 -0.23 % 0.369869995 dgram\\multi-buffer.js dur=5 type="send" chunks=1 num=100 len=256 -0.45 % * 0.037902697 dgram\\multi-buffer.js dur=5 type="send" chunks=1 num=100 len=64 -0.47 % 0.096614111 dgram\\multi-buffer.js dur=5 type="send" chunks=2 num=100 len=1024 -0.29 % 0.270257446 dgram\\multi-buffer.js dur=5 type="send" chunks=2 num=100 len=256 -0.16 % 0.539737074 dgram\\multi-buffer.js dur=5 type="send" chunks=2 num=100 len=64 -0.39 % 0.110370236 dgram\\multi-buffer.js dur=5 type="send" chunks=4 num=100 len=1024 -0.19 % 0.597112456 dgram\\multi-buffer.js dur=5 type="send" chunks=4 num=100 len=256 -0.29 % 0.308934876 dgram\\multi-buffer.js dur=5 type="send" chunks=4 num=100 len=64 -0.08 % 0.796715902 dgram\\multi-buffer.js dur=5 type="send" chunks=8 num=100 len=1024 0.56 % * 0.036801865 dgram\\multi-buffer.js dur=5 type="send" chunks=8 num=100 len=256 -0.01 % 0.974705939 dgram\\multi-buffer.js dur=5 type="send" chunks=8 num=100 len=64 0.64 % * 0.016531293 dgram\\offset-length.js dur=5 type="recv" num=100 len=1 -3.16 % 0.084384305 dgram\\offset-length.js dur=5 type="recv" num=100 len=1024 -0.60 % 0.358465196 dgram\\offset-length.js dur=5 type="recv" num=100 len=256 0.03 % 0.972613434 dgram\\offset-length.js dur=5 type="recv" num=100 len=64 -1.05 % * 0.033493587 dgram\\offset-length.js dur=5 type="send" num=100 len=1 -1.74 % 0.051167510 dgram\\offset-length.js dur=5 type="send" num=100 len=1024 0.06 % 0.963471910 dgram\\offset-length.js dur=5 type="send" num=100 len=256 -0.63 % 0.342649711 dgram\\offset-length.js dur=5 type="send" num=100 len=64 -0.21 % 0.741684095 dgram\\single-buffer.js dur=5 type="recv" num=100 len=1 0.26 % 0.532516397 dgram\\single-buffer.js dur=5 type="recv" num=100 len=1024 0.24 % 0.609156242 dgram\\single-buffer.js dur=5 type="recv" num=100 len=256 -0.22 % 0.409761765 dgram\\single-buffer.js dur=5 type="recv" num=100 len=64 -0.04 % 0.909745426 dgram\\single-buffer.js dur=5 type="send" num=100 len=1 -0.17 % 0.674389873 dgram\\single-buffer.js dur=5 type="send" num=100 len=1024 -0.56 % 0.117767197 dgram\\single-buffer.js dur=5 type="send" num=100 len=256 0.70 % 0.394189582 dgram\\single-buffer.js dur=5 type="send" num=100 len=64 0.29 % 0.434809490 |
Sorry, something went wrong.
|
@cjihrig @Fishrock123 'use strict';
const dgram = require('dgram');
console.time('bind');
for (let i = 0; i < 1e4; i++) dgram.createSocket('udp4').bind();
console.timeEnd('bind');node.exe before the fix: ~100 ms |
Sorry, something went wrong.
|
@vsemozhetbyt sounds good to me. It might be worth adding a new benchmark for bind() in a separate PR. |
Sorry, something went wrong.
|
There was a conflict with the recent #11243, so I've resolved it in the GitHub. But this has added a merge commit. Is it OK? Will it be squashed? Or should I resolve in another way next time in conflict case? |
Sorry, something went wrong.
|
Can you squash it please. |
Sorry, something went wrong.
Yes. :)
It would probably be easiest for everyone if you git rebase to resolve conflicts, if you feel comfortable doing that. |
Sorry, something went wrong.
|
Sorry, I've messed up one of the previous PRs in similar circumstances, attempting to rebase. Is there a guide for this case? |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
@vsemozhetbyt here are your changes as a single commit. Can you fix up your branch, and I'll run the CI again. |
Sorry, something went wrong.
|
@cjihrig I'm afraid I don't know how to do that:( Could you list git commands and edit actions for that? Sorry:( |
Sorry, something went wrong.
|
@vsemozhetbyt you can get rid of the merge commit using git reset --hard HEAD~1. Then, git pull --rebase origin master (your remote and branch names may vary). You should have to resolve conflicts in lib/dgram.js. Once you finish the rebase, you should have a single commit that shows up as the first entry in git log. You can then push the branch with the --force to update this PR. |
Sorry, something went wrong.
|
@cjihrig Thank you. I hope I've done it right. |
Sorry, something went wrong.
|
Looks good. Thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/6416/ |
Sorry, something went wrong.
|
@vsemozhetbyt The Github bot reporting test/arm as a failure when it's actually passing in CI is an existing issue (that I think has actually just been fixed). If it's green in ci.nodejs.org then it should be fine. |
Sorry, something went wrong.
|
@cjihrig @vsemozhetbyt ... it appears that one of the recent dgram related commits that landed today maybe causing some failures in CI in arm (see https://ci.nodejs.org/job/node-test-binary-arm/6233/RUN_SUBSET=1,label=pi1-raspbian-wheezy/console for example). I'm not sure exactly which commit may have done it, but I'm seeing the same failure across multiple independent CI runs. |
Sorry, something went wrong.
|
The problem seems to be related to the ARM cluster move - nodejs/build#611 (comment). |
Sorry, something went wrong.
This commit adds a guard against an out of bounds access of arguments, and replaces another use of arguments with a named function parameter. Refs: nodejs#10323 PR-URL: nodejs#11242 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed on v6. Would need a backport PR to land on v4 |
Sorry, something went wrong.
Sorry, something went wrong.
Checklist
Affected core subsystem(s)
dgram
Socket.prototype.bind() could be called without any parameters, so these two argumets[i] access could be out of bounds and in some cases, they could possibly cause deoptimizations.
See #10323 and this comment.
Simple benchmarks show no performance degradation after these fixes:
Before the fixes (click me):