net: refactor overloaded argument handling #11667
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Member Author
| CI: https://ci.nodejs.org/job/node-test-pull-request/6676/ |
joyeecheung commented Mar 3, 2017
lib/net.js Outdated
Member Author
There was a problem hiding this comment.
The doc says
If the hostname is omitted, the server will accept connections on the unspecified IPv6 address (::) when IPv6 is available, or the unspecified IPv4 address (0.0.0.0) otherwise.
But looks like previously it just went straight to IPv4? (This PR doesn't change that)
Contributor
There was a problem hiding this comment.
this is reference to the comment, // Undefined host, listens on unspecified IPv4 address?
Member Author
There was a problem hiding this comment.
@sam-github Oh looks like Server.prototype._listen2 would ignore addressType when address is null and try to bind the unspecified IPv6 address...nevermind then :)
mscdex reviewed Mar 3, 2017
lib/net.js Outdated
Contributor
There was a problem hiding this comment.
I thought this was still slow (at least in some cases)? /cc @jasnell
Member Author
There was a problem hiding this comment.
AFAIK it is slower when you can skip converting it into array under some condition(and only when you call it with arguments that are named), but not when you are always converting it and heavily overloading it like this...
Contributor
There was a problem hiding this comment.
I was thinking it more had to do with the number of arguments passed or something along those lines...
Member
There was a problem hiding this comment.
The slowness does not only come from the rest parameter itself. Its presence disables Crankshaft for this function and it's possible that TurboFan generates slower code.
Member Author
There was a problem hiding this comment.
Related: https://fhinkel.github.io/six-speed/#test-rest says rest parameters are faster, but our restparams-bench benchmark says otherwise...though none of them are testing overloaded arguments
mscdex reviewed Mar 3, 2017
lib/net.js Outdated
Contributor
There was a problem hiding this comment.
Unintentional whitespace change?
Member Author
| Previous CI failed in a bizarre way...(says "All checks have failed" but https://ci.nodejs.org/job/node-test-commit/8233/ is mostly green..?) New CI: https://ci.nodejs.org/job/node-test-pull-request/6678/ |
Member Author
| Nothing with enough confidence shows up in the benchmark results.. Benchmark resultsimprovement confidence p.value net/net-c2s-cork.js dur=5 type="buf" len=1024 -2.31 % 0.26941557 net/net-c2s-cork.js dur=5 type="buf" len=128 1.61 % 0.35423657 net/net-c2s-cork.js dur=5 type="buf" len=16 -0.15 % 0.93907021 net/net-c2s-cork.js dur=5 type="buf" len=32 3.33 % 0.08961369 net/net-c2s-cork.js dur=5 type="buf" len=4 0.62 % 0.74934703 net/net-c2s-cork.js dur=5 type="buf" len=512 1.27 % 0.66222617 net/net-c2s-cork.js dur=5 type="buf" len=64 2.31 % 0.24750306 net/net-c2s-cork.js dur=5 type="buf" len=8 1.79 % 0.31765058 net/net-c2s.js dur=5 type="asc" len=102400 -2.65 % 0.34713637 net/net-c2s.js dur=5 type="asc" len=16777216 1.85 % 0.70764228 net/net-c2s.js dur=5 type="buf" len=102400 4.10 % 0.38920034 net/net-c2s.js dur=5 type="buf" len=16777216 -5.24 % 0.15459963 net/net-c2s.js dur=5 type="utf" len=102400 -2.31 % 0.53227401 net/net-c2s.js dur=5 type="utf" len=16777216 0.28 % 0.94308128 net/net-pipe.js dur=5 type="asc" len=102400 2.79 % 0.61437373 net/net-pipe.js dur=5 type="asc" len=16777216 2.38 % 0.63068650 net/net-pipe.js dur=5 type="buf" len=102400 2.66 % 0.60057916 net/net-pipe.js dur=5 type="buf" len=16777216 -0.88 % 0.77870985 net/net-pipe.js dur=5 type="utf" len=102400 -5.81 % 0.13508888 net/net-pipe.js dur=5 type="utf" len=16777216 2.76 % 0.09846037 net/net-s2c.js dur=5 type="asc" len=102400 1.83 % 0.49287261 net/net-s2c.js dur=5 type="asc" len=16777216 -0.78 % 0.75951114 net/net-s2c.js dur=5 type="buf" len=102400 1.77 % 0.37888441 net/net-s2c.js dur=5 type="buf" len=16777216 -0.83 % 0.76237945 net/net-s2c.js dur=5 type="utf" len=102400 2.04 % 0.25163787 net/net-s2c.js dur=5 type="utf" len=16777216 -2.33 % 0.46584938 net/tcp-raw-c2s.js dur=5 type="asc" len=102400 1.84 % 0.48664417 net/tcp-raw-c2s.js dur=5 type="asc" len=16777216 2.99 % 0.31863285 net/tcp-raw-c2s.js dur=5 type="buf" len=102400 -1.23 % 0.43411749 net/tcp-raw-c2s.js dur=5 type="buf" len=16777216 -0.87 % 0.68639700 net/tcp-raw-c2s.js dur=5 type="utf" len=102400 2.95 % 0.09170229 net/tcp-raw-c2s.js dur=5 type="utf" len=16777216 1.44 % 0.51893571 net/tcp-raw-pipe.js dur=5 type="asc" len=102400 1.00 % 0.86801283 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216 -1.41 % 0.52368250 net/tcp-raw-pipe.js dur=5 type="buf" len=102400 2.81 % 0.52655840 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216 1.43 % 0.55855929 net/tcp-raw-pipe.js dur=5 type="utf" len=102400 55.24 % 0.06649256 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216 -0.47 % 0.84940769 net/tcp-raw-s2c.js dur=5 type="asc" len=102400 -0.98 % 0.48191208 net/tcp-raw-s2c.js dur=5 type="asc" len=16777216 2.25 % 0.33763947 net/tcp-raw-s2c.js dur=5 type="buf" len=102400 -1.83 % 0.28434154 net/tcp-raw-s2c.js dur=5 type="buf" len=16777216 0.89 % 0.39528692 net/tcp-raw-s2c.js dur=5 type="utf" len=102400 -1.13 % 0.45419167 net/tcp-raw-s2c.js dur=5 type="utf" len=16777216 3.17 % 0.15172657 |
Member Author
| This is causing some consistent errors on windows-fanned with vcbt2015(RUN_SUBSET = 2)..I'll split the commits and try to find out what is causing this |
joyeecheung changed the title Member Author
| Turns out it removed the case |
joyeecheung changed the title Member Author
Benchmark numbersimprovement confidence p.value net/net-c2s-cork.js dur=5 type="buf" len=1024 0.51 % 0.847875581 net/net-c2s-cork.js dur=5 type="buf" len=128 1.64 % 0.546539268 net/net-c2s-cork.js dur=5 type="buf" len=16 0.61 % 0.810465920 net/net-c2s-cork.js dur=5 type="buf" len=32 0.32 % 0.878093841 net/net-c2s-cork.js dur=5 type="buf" len=4 1.38 % 0.467924486 net/net-c2s-cork.js dur=5 type="buf" len=512 -0.08 % 0.976736133 net/net-c2s-cork.js dur=5 type="buf" len=64 4.72 % * 0.040232528 net/net-c2s-cork.js dur=5 type="buf" len=8 1.65 % 0.451122281 net/net-c2s.js dur=5 type="asc" len=102400 -3.94 % 0.080389731 net/net-c2s.js dur=5 type="asc" len=16777216 3.07 % 0.243184465 net/net-c2s.js dur=5 type="buf" len=102400 -1.99 % 0.407039086 net/net-c2s.js dur=5 type="buf" len=16777216 1.94 % 0.376930199 net/net-c2s.js dur=5 type="utf" len=102400 -3.40 % 0.050772843 net/net-c2s.js dur=5 type="utf" len=16777216 -0.66 % 0.688283137 net/net-pipe.js dur=5 type="asc" len=102400 2.86 % 0.189891017 net/net-pipe.js dur=5 type="asc" len=16777216 1.28 % 0.567203334 net/net-pipe.js dur=5 type="buf" len=102400 -0.04 % 0.980100699 net/net-pipe.js dur=5 type="buf" len=16777216 -1.60 % 0.495281133 net/net-pipe.js dur=5 type="utf" len=102400 0.12 % 0.950469747 net/net-pipe.js dur=5 type="utf" len=16777216 -0.76 % 0.617628652 net/net-s2c.js dur=5 type="asc" len=102400 -2.91 % 0.109145558 net/net-s2c.js dur=5 type="asc" len=16777216 -1.11 % 0.450574592 net/net-s2c.js dur=5 type="buf" len=102400 -1.68 % 0.294086039 net/net-s2c.js dur=5 type="buf" len=16777216 3.28 % 0.147678955 net/net-s2c.js dur=5 type="utf" len=102400 -0.88 % 0.615214884 net/net-s2c.js dur=5 type="utf" len=16777216 -0.72 % 0.709583085 net/tcp-raw-c2s.js dur=5 type="asc" len=102400 -2.01 % 0.449092918 net/tcp-raw-c2s.js dur=5 type="asc" len=16777216 0.96 % 0.663394878 net/tcp-raw-c2s.js dur=5 type="buf" len=102400 -2.15 % 0.463581812 net/tcp-raw-c2s.js dur=5 type="buf" len=16777216 0.55 % 0.849280563 net/tcp-raw-c2s.js dur=5 type="utf" len=102400 -0.13 % 0.960543687 net/tcp-raw-c2s.js dur=5 type="utf" len=16777216 -0.62 % 0.806655208 net/tcp-raw-pipe.js dur=5 type="asc" len=102400 3.42 % 0.595403945 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216 123.35 % ** 0.005935044 net/tcp-raw-pipe.js dur=5 type="buf" len=102400 12.58 % * 0.035927483 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216 41.86 % 0.104380751 net/tcp-raw-pipe.js dur=5 type="utf" len=102400 -2.46 % 0.782995053 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216 18.08 % 0.308727856 net/tcp-raw-s2c.js dur=5 type="asc" len=102400 -3.76 % 0.297974161 net/tcp-raw-s2c.js dur=5 type="asc" len=16777216 2.32 % 0.441501849 net/tcp-raw-s2c.js dur=5 type="buf" len=102400 -2.41 % 0.511049295 net/tcp-raw-s2c.js dur=5 type="buf" len=16777216 -1.40 % 0.732423279 net/tcp-raw-s2c.js dur=5 type="utf" len=102400 -4.65 % 0.105507183 net/tcp-raw-s2c.js dur=5 type="utf" len=16777216 -1.56 % 0.462116407 The tcp-raw-pipe seems to make a difference, but running it again... See numbersimprovement confidence p.value net/tcp-raw-pipe.js dur=5 type="asc" len=102400 -1.02 % 0.9438923 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216 165.70 % 0.2828262 net/tcp-raw-pipe.js dur=5 type="buf" len=102400 -11.90 % 0.2488332 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216 -41.46 % 0.3789885 net/tcp-raw-pipe.js dur=5 type="utf" len=102400 -13.93 % 0.3436313 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216 25.75 % 0.3271788 And run it master against master... See numbers(wait wat)improvement confidence p.value net/tcp-raw-pipe.js dur=5 type="asc" len=102400 -12.05 % 0.4330058 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216 58.87 % 0.4848385 net/tcp-raw-pipe.js dur=5 type="buf" len=102400 -5.68 % 0.6042492 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216 15.40 % 0.4404852 net/tcp-raw-pipe.js dur=5 type="utf" len=102400 -15.17 % 0.2378737 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216 20.97 % 0.4273859 ¯\(ツ)/¯ I would interpret this as "no significant difference" and the first results as "coincidence". CI is green. Need LTGMs.. |
3 tasks
Member
| Based on onboarding-extras, cc/ @bnoordhuis, @indutny, @nodejs/streams |
jasnell reviewed Mar 4, 2017
lib/net.js Outdated
Member
There was a problem hiding this comment.
For the TODO, can you please use the TODO (@joyeecheung): syntax?
jasnell approved these changes Mar 4, 2017
Member Author
| Added handle after TODO @jasnell :) CITGM against master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/621/ |
gibfahn reviewed Mar 5, 2017
lib/net.js Outdated
Member
There was a problem hiding this comment.
Super-nit, but none of the other TODOs in the codebase have an @ in them, the most standard style I can find is:
// TODO(joyeecheung): use ... Member Author
| @gibfahn OK, fixed. BTW this convention can be mentioned in STYLE_GUIDE.md? CITGM is |
Member
| @joyeecheung Looks like the only thing that failed in citgm on this PR but not in master was serialport on Ubuntu 14.04, failure was: I reran just serialport on this PR (failed) and on master (passed), so the failure does seem to be consistent. |
Member
| cc/ @nodejs/citgm |
Member Author
| @gibfahn Ooops, sorry for not checking it more carefully.. Rebased against the latest master and tried again to be certain...If it still fails, I will try different commits to see which one is causing this. |
Make normalizeArgs return either [options, null] or [options, cb] (the second element won't be undefined anymore) and avoid OOB read
Use Socket.prototype.connect.call instead of .apply when the number of arguments is certain(returned by normalizeArgs).
* Separate backlogFromArgs and options.backlog * Comment the overloading process
joyeecheung added a commit that referenced this pull request Mar 8, 2017
* Make normalizeArgs return either [options, null] or [options, cb] (the second element won't be undefined anymore) and avoid OOB read * Use Socket.prototype.connect.call instead of .apply when the number of arguments is certain(returned by normalizeArgs). * Rename some args[i] for readability * Refactor Server.prototype.listen, separate backlogFromArgs and options.backlog, comment the overloading process, refactor control flow PR-URL: #11667 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
3 tasks
evanlucas pushed a commit that referenced this pull request Mar 8, 2017
* Make normalizeArgs return either [options, null] or [options, cb] (the second element won't be undefined anymore) and avoid OOB read * Use Socket.prototype.connect.call instead of .apply when the number of arguments is certain(returned by normalizeArgs). * Rename some args[i] for readability * Refactor Server.prototype.listen, separate backlogFromArgs and options.backlog, comment the overloading process, refactor control flow PR-URL: #11667 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
watilde added a commit to watilde/node that referenced this pull request Mar 12, 2017
A module `assertPort` in `lib/internal/net.js` is not used anymore. Refs: nodejs#11667
2 tasks
watilde added a commit that referenced this pull request Mar 14, 2017
A module `assertPort` in `lib/internal/net.js` is not used anymore. Refs: #11667 PR-URL: #11812 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 14, 2017
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: #11684 Ref: #11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: nodejs#11684 Ref: nodejs#11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
* Make normalizeArgs return either [options, null] or [options, cb] (the second element won't be undefined anymore) and avoid OOB read * Use Socket.prototype.connect.call instead of .apply when the number of arguments is certain(returned by normalizeArgs). * Rename some args[i] for readability * Refactor Server.prototype.listen, separate backlogFromArgs and options.backlog, comment the overloading process, refactor control flow PR-URL: nodejs#11667 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
A module `assertPort` in `lib/internal/net.js` is not used anymore. Refs: nodejs#11667 PR-URL: nodejs#11812 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: nodejs#11684 Ref: nodejs#11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Contributor
| is this something we want to backport to v6.x? |
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: #11684 Ref: #11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: #11684 Ref: #11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
3 tasks
watilde added a commit to watilde/node that referenced this pull request May 5, 2017
A module `assertPort` in `lib/internal/net.js` is not used anymore. Refs: nodejs#11667 PR-URL: nodejs#11812 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Contributor
| ping |
Member Author
Member Author
| So this PR depends on #4039 which is a 7.x semver-major. Added don't land label |
watilde added a commit that referenced this pull request May 12, 2017
A module `assertPort` in `lib/internal/net.js` is not used anymore. Refs: #11667 PR-URL: #11812 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: nodejs/node#11684 Ref: nodejs/node#11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Destructure arguments when it will always be converted into an arrayof arguments is certain(returned by normalizeArgs)
(the second element won't be undefined anymore) and avoid OOB read
options.backlog, avoid making options a handle, comment the
overloading process
Pending benchmark results...
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
net