Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
dgram: fix possibly deoptimizing use of arguments#11242
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Fishrock123 commented Feb 8, 2017
cjihrig commented Feb 8, 2017
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. |
vsemozhetbyt commented Feb 8, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@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 constdgram=require('dgram');for(leti=0;i<1000;i++){constserver=dgram.createSocket('udp4');server.on('listening',()=>{constaddress=server.address();console.log(`server listening ${address.address}:${address.port}`);});server.bind();}Before the fixes: After the fixes: |
cjihrig commented Feb 8, 2017
I haven't benchmarked, but you could do this to avoid using Using |
vsemozhetbyt commented Feb 8, 2017
@cjihrig Should I amend with Sorry, could you explain the second sentence some more, please? |
cjihrig commented Feb 8, 2017
Sorry, I was just saying, we could use |
vsemozhetbyt commented Feb 8, 2017
@cjihrig I've amended the second change, build and tests pass. I've installed R and have launched the |
vsemozhetbyt commented Feb 9, 2017
Benchmarks results: |
vsemozhetbyt commented Feb 10, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@cjihrig@Fishrock123 'use strict';constdgram=require('dgram');console.time('bind');for(leti=0;i<1e4;i++)dgram.createSocket('udp4').bind();console.timeEnd('bind');node.exe before the fix: ~100 ms |
cjihrig commented Feb 10, 2017
@vsemozhetbyt sounds good to me. It might be worth adding a new benchmark for |
vsemozhetbyt commented Feb 10, 2017
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? |
cjihrig commented Feb 10, 2017
Can you squash it please. |
addaleax commented Feb 10, 2017
Yes. :)
It would probably be easiest for everyone if you |
vsemozhetbyt commented Feb 10, 2017
Sorry, I've messed up one of the previous PRs in similar circumstances, attempting to rebase. Is there a guide for this case? |
vsemozhetbyt commented Feb 11, 2017
cjihrig commented Feb 13, 2017
cjihrig commented Feb 14, 2017
@vsemozhetbythere are your changes as a single commit. Can you fix up your branch, and I'll run the CI again. |
vsemozhetbyt commented Feb 14, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@cjihrig I'm afraid I don't know how to do that:( Could you list git commands and edit actions for that? Sorry:( |
cjihrig commented Feb 14, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@vsemozhetbyt you can get rid of the merge commit using |
vsemozhetbyt commented Feb 14, 2017
@cjihrig Thank you. I hope I've done it right. |
cjihrig commented Feb 14, 2017
Looks good. Thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/6416/ |
vsemozhetbyt commented Feb 16, 2017
gibfahn commented Feb 16, 2017
@vsemozhetbyt The Github bot reporting |
cjihrig commented Feb 16, 2017
Landed in adf1ed0. Thanks! |
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: #10323 PR-URL: #11242 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
jasnell commented Feb 16, 2017
@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. |
cjihrig commented Feb 16, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #11242 PR-URL: #11313 Reviewed-By: Brian White <[email protected]>
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: #10323 PR-URL: #11242 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #11242 PR-URL: #11313 Reviewed-By: Brian White <[email protected]>
Refs: #11242 PR-URL: #11313 Reviewed-By: Brian White <[email protected]>
Refs: #11242 PR-URL: #11313 Reviewed-By: Brian White <[email protected]>
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: #10323 PR-URL: #11242 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
jasnell commented Mar 9, 2017
Landed on v6. Would need a backport PR to land on v4 |
vsemozhetbyt commented Mar 9, 2017
Refs: #11242 PR-URL: #11313 Reviewed-By: Brian White <[email protected]>
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: #10323 PR-URL: #11242 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #11242 PR-URL: #11313 Reviewed-By: Brian White <[email protected]>
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):