dgram: support multiple buffers by mcollina · Pull Request #4374 · nodejs/node · GitHub

This PR adds support for:

socket.send(buf, port, host, [, callback])
socket.send([buf1, buf2, ... ], port, host, [, callback])

This is probably missing some benchmarks, and might like an optimization pass, but I would like to get some feedbacks first :).

cc @ronkorving @indutny @saghul

I guess arguments fit on the same line? ;)

Great start, thanks a ton :) What do the benchmarks say at this time?

Current benchmarks comes out slightly slower on the "old" API. These are possibly due to: #4374 (diff)

before:
net/dgram.js len=1 num=100 type=send dur=5: 0.00120
net/dgram.js len=1 num=100 type=recv dur=5: 0.00020
net/dgram.js len=64 num=100 type=send dur=5: 0.07730
net/dgram.js len=64 num=100 type=recv dur=5: 0.01255
net/dgram.js len=256 num=100 type=send dur=5: 0.30837
net/dgram.js len=256 num=100 type=recv dur=5: 0.04958
net/dgram.js len=1024 num=100 type=send dur=5: 0.71537
net/dgram.js len=1024 num=100 type=recv dur=5: 0.12022

After:
net/dgram.js len=1 num=100 type=send dur=5: 0.00106
net/dgram.js len=1 num=100 type=recv dur=5: 0.00018
net/dgram.js len=64 num=100 type=send dur=5: 0.06785
net/dgram.js len=64 num=100 type=recv dur=5: 0.01109
net/dgram.js len=256 num=100 type=send dur=5: 0.27151
net/dgram.js len=256 num=100 type=recv dur=5: 0.04330
net/dgram.js len=1024 num=100 type=send dur=5: 0.70451
net/dgram.js len=1024 num=100 type=recv dur=5: 0.11787

I have no benchmarks yet on the new APIs.

Use common.mustCall here, so that it will make sure that the test will fail if the callback is not invoked.

@thefourtheye @indutny I have updated the tests to follow the current conventions. I will update also the other test to the same on another PR.

Not on this PR, but it would probably be nice to use uv_udp_try_send here, just in case we can send it on the spot.

I would do that after this land. Do you see any issue with switching to uv_udp_try_send?
What is the difference anyway? on what conditions it can be sent on the spot?

It's not switching, the idea is to use it to try to send the datagram on the spot, because the socket might be writable. Have a look at how uv_try_write is used in stream_wrap.cc.

I'm 👍 on that. I'll get it done after this one.

I have done a pass of performance optimization in JS-land, which result in better performance for the single buffer use case (and offset/length is now on par with master).

Here are the full results:
https://gist.github.com/mcollina/e8210d26ca6ef2630e23

Unexpectedly, passing multiple buffers in an array is slower (benchmark/dgram/headers.js) than calling Buffer.concat() in js land for a large payload:

dgram/headers.js len=1024 num=100 chunks=1 headers=4 type=concat dur=5: 0.78476
dgram/headers.js len=1024 num=100 chunks=1 headers=4 type=multi dur=5: 0.67456
dgram/headers.js len=1024 num=100 chunks=2 headers=4 type=concat dur=5: 0.78631
dgram/headers.js len=1024 num=100 chunks=2 headers=4 type=multi dur=5: 0.63593
dgram/headers.js len=1024 num=100 chunks=4 headers=4 type=concat dur=5: 0.75965
dgram/headers.js len=1024 num=100 chunks=4 headers=4 type=multi dur=5: 0.63869
dgram/headers.js len=1024 num=100 chunks=8 headers=4 type=concat dur=5: 0.74356
dgram/headers.js len=1024 num=100 chunks=8 headers=4 type=multi dur=5: 0.59303

The small buffer case (up to 256 bytes) is way faster than calling Buffer.concat().

Any ideas why this is happening? I expected it to be faster.

@trevnorris

nit: function name is slightly incomplete? maybe something like fixAndSumBuffers(). this is nothing serious, and not a blocker.

Fixes: #5124 See: #4374 PR-URL: #5130 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Added ability to dgram.send to send multiple buffers, _writev style. The offset and length parameters in dgram.send are now optional. Refactored the dgram benchmarks, and seperated them from net. Added docs for the new signature. Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Fixes: #4302 PR-URL: #4374

* buffer: - You can now supply an encoding argument when filling a Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying an existing Buffer will also work with Buffer#fill(buffer[, start[, end]]). See the API documentation for details on how this works. (Trevor Norris) #4935 - Buffer#indexOf() no longer requires a byteOffset argument if you also wish to specify an encoding: Buffer#indexOf(val[, byteOffset][, encoding]). (Trevor Norris) #4803 * child_process: spawn() and spawnSync() now support a 'shell' option to allow for optional execution of the given command inside a shell. If set to true, cmd.exe will be used on Windows and /bin/sh elsewhere. A path to a custom shell can also be passed to override these defaults. On Windows, this option allows .bat. and .cmd files to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598 * http_parser: Update to http-parser 2.6.2 to fix an unintentionally strict limitation of allowable header characters. (James M Snell) #5237 * dgram: socket.send() now supports accepts an array of Buffers or Strings as the first argument. See the API docs for details on how this works. (Matteo Collina) #4374 * http: Fix a bug where handling headers will mistakenly trigger an 'upgrade' event where the server is just advertising its protocols. This bug can prevent HTTP clients from communicating with HTTP/2 enabled servers. (Fedor Indutny) #4337 * net: Added a listening Boolean property to net and http servers to indicate whether the server is listening for connections. (José Moreira) #4743 * node: The C++ node::MakeCallback() API is now reentrant and calling it from inside another MakeCallback() call no longer causes the nextTick queue or Promises microtask queue to be processed out of order. (Trevor Norris) #4507 * tls: Add a new tlsSocket.getProtocol() method to get the negotiated TLS protocol version of the current connection. (Brian White) #4995 * vm: Introduce new 'produceCachedData' and 'cachedData' options to new vm.Script() to interact with V8's code cache. When a new vm.Script object is created with the 'produceCachedData' set to true a Buffer with V8's code cache data will be produced and stored in cachedData property of the returned object. This data in turn may be supplied back to another vm.Script() object with a 'cachedData' option if the supplied source is the same. Successfully executing a script from cached data can speed up instantiation time. See the API docs for details. (Fedor Indutny) #4777 * performance: Improvements in: - process.nextTick() (Ruben Bridgewater) #5092 - path module (Brian White) #5123 - querystring module (Brian White) #5012 - streams module when processing small chunks (Matteo Collina) #4354

* buffer: - You can now supply an encoding argument when filling a Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying an existing Buffer will also work with Buffer#fill(buffer[, start[, end]]). See the API documentation for details on how this works. (Trevor Norris) #4935 - Buffer#indexOf() no longer requires a byteOffset argument if you also wish to specify an encoding: Buffer#indexOf(val[, byteOffset][, encoding]). (Trevor Norris) #4803 * child_process: spawn() and spawnSync() now support a 'shell' option to allow for optional execution of the given command inside a shell. If set to true, cmd.exe will be used on Windows and /bin/sh elsewhere. A path to a custom shell can also be passed to override these defaults. On Windows, this option allows .bat. and .cmd files to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598 * http_parser: Update to http-parser 2.6.2 to fix an unintentionally strict limitation of allowable header characters. (James M Snell) #5237 * dgram: socket.send() now supports accepts an array of Buffers or Strings as the first argument. See the API docs for details on how this works. (Matteo Collina) #4374 * http: Fix a bug where handling headers will mistakenly trigger an 'upgrade' event where the server is just advertising its protocols. This bug can prevent HTTP clients from communicating with HTTP/2 enabled servers. (Fedor Indutny) #4337 * net: Added a listening Boolean property to net and http servers to indicate whether the server is listening for connections. (José Moreira) #4743 * node: The C++ node::MakeCallback() API is now reentrant and calling it from inside another MakeCallback() call no longer causes the nextTick queue or Promises microtask queue to be processed out of order. (Trevor Norris) #4507 * tls: Add a new tlsSocket.getProtocol() method to get the negotiated TLS protocol version of the current connection. (Brian White) #4995 * vm: Introduce new 'produceCachedData' and 'cachedData' options to new vm.Script() to interact with V8's code cache. When a new vm.Script object is created with the 'produceCachedData' set to true a Buffer with V8's code cache data will be produced and stored in cachedData property of the returned object. This data in turn may be supplied back to another vm.Script() object with a 'cachedData' option if the supplied source is the same. Successfully executing a script from cached data can speed up instantiation time. See the API docs for details. (Fedor Indutny) #4777 * performance: Improvements in: - process.nextTick() (Ruben Bridgewater) #5092 - path module (Brian White) #5123 - querystring module (Brian White) #5012 - streams module when processing small chunks (Matteo Collina) #4354 PR-URL: #5295

So happy this is released!! Thx to everyone!

* buffer: - You can now supply an encoding argument when filling a Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying an existing Buffer will also work with Buffer#fill(buffer[, start[, end]]). See the API documentation for details on how this works. (Trevor Norris) #4935 - Buffer#indexOf() no longer requires a byteOffset argument if you also wish to specify an encoding: Buffer#indexOf(val[, byteOffset][, encoding]). (Trevor Norris) #4803 * child_process: spawn() and spawnSync() now support a 'shell' option to allow for optional execution of the given command inside a shell. If set to true, cmd.exe will be used on Windows and /bin/sh elsewhere. A path to a custom shell can also be passed to override these defaults. On Windows, this option allows .bat. and .cmd files to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598 * http_parser: Update to http-parser 2.6.2 to fix an unintentionally strict limitation of allowable header characters. (James M Snell) #5237 * dgram: socket.send() now supports accepts an array of Buffers or Strings as the first argument. See the API docs for details on how this works. (Matteo Collina) #4374 * http: Fix a bug where handling headers will mistakenly trigger an 'upgrade' event where the server is just advertising its protocols. This bug can prevent HTTP clients from communicating with HTTP/2 enabled servers. (Fedor Indutny) #4337 * net: Added a listening Boolean property to net and http servers to indicate whether the server is listening for connections. (José Moreira) #4743 * node: The C++ node::MakeCallback() API is now reentrant and calling it from inside another MakeCallback() call no longer causes the nextTick queue or Promises microtask queue to be processed out of order. (Trevor Norris) #4507 * tls: Add a new tlsSocket.getProtocol() method to get the negotiated TLS protocol version of the current connection. (Brian White) #4995 * vm: Introduce new 'produceCachedData' and 'cachedData' options to new vm.Script() to interact with V8's code cache. When a new vm.Script object is created with the 'produceCachedData' set to true a Buffer with V8's code cache data will be produced and stored in cachedData property of the returned object. This data in turn may be supplied back to another vm.Script() object with a 'cachedData' option if the supplied source is the same. Successfully executing a script from cached data can speed up instantiation time. See the API docs for details. (Fedor Indutny) #4777 * performance: Improvements in: - process.nextTick() (Ruben Bridgewater) #5092 - path module (Brian White) #5123 - querystring module (Brian White) #5012 - streams module when processing small chunks (Matteo Collina) #4354 PR-URL: #5295

Fixes a regression introduced by: nodejs#4374. Adds a new test to avoid similar issue in the future. Fixes: nodejs#5398

Fixes a regression introduced by: nodejs#4374. Adds a new test to avoid similar issue in the future. The test is disabled on windows, because this feature never worked there. Fixes: nodejs#5398 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Evan Lucas <evanlucas@me.com>

Fixes a regression introduced by: #4374. Adds a new test to avoid similar issue in the future. The test is disabled on windows, because this feature never worked there. Fixes: #5398 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Evan Lucas <evanlucas@me.com>

Added ability to dgram.send to send multiple buffers, _writev style. The offset and length parameters in dgram.send are now optional. Refactored the dgram benchmarks, and seperated them from net. Added docs for the new signature. Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Fixes: nodejs#4302 PR-URL: nodejs#4374

Fixes: nodejs#5124 See: nodejs#4374 PR-URL: nodejs#5130 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mcollina added dgram Issues and PRs related to the dgram subsystem / UDP. semver-minor PRs that contain new features and should be released in the next minor version. labels Dec 21, 2015

indutny reviewed Dec 21, 2015
View reviewed changes

mcollina force-pushed the new-dgram branch 2 times, most recently from c75640d to 3fe2fac Compare December 21, 2015 09:16

mcollina force-pushed the new-dgram branch from 3fe2fac to b10d44d Compare December 21, 2015 12:26

thefourtheye reviewed Dec 21, 2015
View reviewed changes

mcollina force-pushed the new-dgram branch from b10d44d to c78db77 Compare December 22, 2015 08:18

saghul reviewed Dec 22, 2015
View reviewed changes

mcollina force-pushed the new-dgram branch 3 times, most recently from c1cee98 to 73ada5e Compare December 22, 2015 11:59

mcollina force-pushed the new-dgram branch from 73ada5e to b912dc0 Compare December 22, 2015 12:56

trevnorris reviewed Dec 23, 2015
View reviewed changes

mcollina added a commit that referenced this pull request Feb 8, 2016

doc: clarify dgram socket.send() multi-buffer support

76bc4c7

Fixes: #5124 See: #4374 PR-URL: #5130 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

doc: clarify dgram socket.send() multi-buffer support

21b0b66

Fixes: #5124 See: #4374 PR-URL: #5130 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

doc: clarify dgram socket.send() multi-buffer support

9f7aa6f

Fixes: #5124 See: #4374 PR-URL: #5130 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

AndreasMadsen mentioned this pull request Feb 14, 2016

Benchmark cleanup #5177

Closed

rvagg mentioned this pull request Feb 18, 2016

Release proposal: 5.7.0 (Stable) #5295

Merged

mcollina mentioned this pull request Feb 24, 2016

dgram: handle default address case when offset and length are specified #5407

Closed

4 tasks

mcollina added a commit to mcollina/node that referenced this pull request Feb 27, 2016

dgram: handle default address case when offset and length are specified

bbe70a6

Fixes a regression introduced by: nodejs#4374. Adds a new test to avoid similar issue in the future. Fixes: nodejs#5398

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

doc: clarify dgram socket.send() multi-buffer support

c88425b

Fixes: nodejs#5124 See: nodejs#4374 PR-URL: nodejs#5130 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

bnoordhuis mentioned this pull request May 6, 2016

dgram: socket.send() crash when input array is modified #6616

Closed

mcollina mentioned this pull request Jul 12, 2016

dgram: copy the array when using the list variant in send() #6804

Closed

3 tasks

dgram: support multiple buffers by mcollina · Pull Request #4374 · nodejs/node · GitHub

Uh oh!

Uh oh!

dgram: support multiple buffers - #4374

dgram: support multiple buffers#4374mcollina wants to merge 1 commit intonodejs:masternodejs/node:masterfrom mcollina:new-dgrammcollina/node:new-dgramCopy head branch name to clipboard

Conversation

mcollina commented Dec 21, 2015

Uh oh!

indutny Dec 21, 2015

Choose a reason for hiding this comment

Uh oh!

ronkorving commented Dec 21, 2015

Uh oh!

mcollina commented Dec 21, 2015

Uh oh!

thefourtheye Dec 21, 2015

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 22, 2015

Uh oh!

saghul Dec 22, 2015

Choose a reason for hiding this comment

Uh oh!

mcollina Dec 22, 2015

Choose a reason for hiding this comment

Uh oh!

saghul Dec 22, 2015

Choose a reason for hiding this comment

Uh oh!

mcollina Dec 22, 2015

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 22, 2015

Uh oh!

thefourtheye commented Dec 23, 2015

Uh oh!

trevnorris Dec 23, 2015

Choose a reason for hiding this comment

Uh oh!

mcollina commented Feb 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

dgram: support multiple buffers#4374
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:new-dgram