dgram: fix possibly deoptimizing use of arguments #11242
Conversation
nodejs-github-bot
added
the
dgram
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. |
|
@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 FWIW, this is some weird test example (partly taken from 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();
}Before the fixes: After the fixes: |
|
I haven't benchmarked, but you could do this to avoid using Using |
|
@cjihrig Should I amend with Sorry, could you explain the second sentence some more, please? |
|
Sorry, I was just saying, we could use |
|
@cjihrig I've amended the second change, build and tests pass. I've installed R and have launched the |
Benchmarks results: |
|
@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 |
|
@vsemozhetbyt sounds good to me. It might be worth adding a new benchmark for |
|
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? |
|
Can you squash it please. |
Yes. :)
It would probably be easiest for everyone if you |
|
Sorry, I've messed up one of the previous PRs in similar circumstances, attempting to rebase. Is there a guide for this case? |
|
@vsemozhetbyt here are your changes as a single commit. Can you fix up your branch, and I'll run the CI again. |
|
@cjihrig I'm afraid I don't know how to do that:( Could you list git commands and edit actions for that? Sorry:( |
|
@vsemozhetbyt you can get rid of the merge commit using |
|
@cjihrig Thank you. I hope I've done it right. |
|
Looks good. Thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/6416/ |
|
@vsemozhetbyt The Github bot reporting |
|
Landed in adf1ed0. Thanks! |
|
@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. |
|
The problem seems to be related to the ARM cluster move - nodejs/build#611 (comment). |
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 |
Checklist
vcbuild test(Windows) passesAffected core subsystem(s)
dgram
Socket.prototype.bind()could be called without any parameters, so these twoargumets[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):
After the fixes (click me):