FAQ
In current version of node, socket write is always 'async', which creates
some write contexts, put it into queue, and get it out when write data
finished, and call the callback. But for socket write, if data size is less
than system socket buffer(which can be modified by
/proc/sys/net/core/wmem_max), write() is indeed nonblock, and all async
work can be saved, than the performance of write+callback will be improve
significantly.

I've forked and made a patch:
https://github.com/freedaxin/node/commit/dba71315bf229b7815c08188ef88097bfbf43135

I added a benchmark(benchmark/net_rw.js), and the time consumption
decreased by about 20%.

It doesn't change the user api, but StreamWrap::WriteBuffer() returns a
Number if all data has been written.

I have run `make jslint test`, found two problems, and changed the test
code.
1. In /test/simple/test-debugger-repl.js, one of the child processes always
does not exit, I didn't found the reason, and changed the finish cleanup
behaviours, and passed the test. But I still don't know what's the problem.

2. In /test/simple/test-tcp-wrap-listen.js, because I changed the return
value of StreamWrap::WriteBuffer(), so the test should be modified.

It's still not a perfect patch, add such a sync feature to async node is
somehow incompatible with the code logic. And I've noticed that the stream
related code is not stable yet, so if this patch is acceptable, more
imporvements can be made later.

btw: windows version has not been changed yet.

issue link:
https://github.com/joyent/node/issues/4699

--
--
Job Board: http://jobs.nodejs.org/
Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
You received this message because you are subscribed to the Google
Groups "nodejs" group.
To post to this group, send email to nodejs@googlegroups.com
To unsubscribe from this group, send email to
nodejs+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/nodejs?hl=en?hl=en

---
You received this message because you are subscribed to the Google Groups "nodejs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to nodejs+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Mark Hahn at Feb 1, 2013 at 3:51 am
    That sounds good.
    if data size is less than system socket buffer ... write() is indeed
    nonblock,

    The old method is also non-blocking. I think you meant to say it is sync
    instead of async.

    Some people complain if you don't know whether a callback is going to be
    async or sync. But I believe code should be written so it doesn't matter.


    On Thu, Jan 31, 2013 at 7:41 PM, darcy wrote:

    In current version of node, socket write is always 'async', which creates
    some write contexts, put it into queue, and get it out when write data
    finished, and call the callback. But for socket write, if data size is less
    than system socket buffer(which can be modified by
    /proc/sys/net/core/wmem_max), write() is indeed nonblock, and all async
    work can be saved, than the performance of write+callback will be improve
    significantly.

    I've forked and made a patch:

    https://github.com/freedaxin/node/commit/dba71315bf229b7815c08188ef88097bfbf43135

    I added a benchmark(benchmark/net_rw.js), and the time consumption
    decreased by about 20%.

    It doesn't change the user api, but StreamWrap::WriteBuffer() returns a
    Number if all data has been written.

    I have run `make jslint test`, found two problems, and changed the test
    code.
    1. In /test/simple/test-debugger-repl.js, one of the child processes
    always does not exit, I didn't found the reason, and changed the finish
    cleanup behaviours, and passed the test. But I still don't know what's the
    problem.

    2. In /test/simple/test-tcp-wrap-listen.js, because I changed the return
    value of StreamWrap::WriteBuffer(), so the test should be modified.

    It's still not a perfect patch, add such a sync feature to async node is
    somehow incompatible with the code logic. And I've noticed that the stream
    related code is not stable yet, so if this patch is acceptable, more
    imporvements can be made later.

    btw: windows version has not been changed yet.

    issue link:
    https://github.com/joyent/node/issues/4699

    --
    --
    Job Board: http://jobs.nodejs.org/
    Posting guidelines:
    https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
    You received this message because you are subscribed to the Google
    Groups "nodejs" group.
    To post to this group, send email to nodejs@googlegroups.com
    To unsubscribe from this group, send email to
    nodejs+unsubscribe@googlegroups.com
    For more options, visit this group at
    http://groups.google.com/group/nodejs?hl=en?hl=en

    ---
    You received this message because you are subscribed to the Google Groups
    "nodejs" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to nodejs+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

    --
    --
    Job Board: http://jobs.nodejs.org/
    Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
    You received this message because you are subscribed to the Google
    Groups "nodejs" group.
    To post to this group, send email to nodejs@googlegroups.com
    To unsubscribe from this group, send email to
    nodejs+unsubscribe@googlegroups.com
    For more options, visit this group at
    http://groups.google.com/group/nodejs?hl=en?hl=en

    ---
    You received this message because you are subscribed to the Google Groups "nodejs" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to nodejs+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Darcy at Feb 1, 2013 at 8:42 am
    Yes, I mean write+callback can be sync in some cases.
    If the callback should be promised to be async, we can use
    process.nextTick(cb.bind(this)) in Socket._write().
    But I think in userland it's not important whether write callback is sync
    or async. And in current implement callback is also sync if some error
    occurs.
    On Friday, February 1, 2013 11:50:56 AM UTC+8, Mark Hahn wrote:

    That sounds good.
    if data size is less than system socket buffer ... write() is indeed
    nonblock,

    The old method is also non-blocking. I think you meant to say it is sync
    instead of async.

    Some people complain if you don't know whether a callback is going to be
    async or sync. But I believe code should be written so it doesn't matter.



    On Thu, Jan 31, 2013 at 7:41 PM, darcy <free...@gmail.com <javascript:>>wrote:
    In current version of node, socket write is always 'async', which creates
    some write contexts, put it into queue, and get it out when write data
    finished, and call the callback. But for socket write, if data size is less
    than system socket buffer(which can be modified by
    /proc/sys/net/core/wmem_max), write() is indeed nonblock, and all async
    work can be saved, than the performance of write+callback will be improve
    significantly.

    I've forked and made a patch:

    https://github.com/freedaxin/node/commit/dba71315bf229b7815c08188ef88097bfbf43135

    I added a benchmark(benchmark/net_rw.js), and the time consumption
    decreased by about 20%.

    It doesn't change the user api, but StreamWrap::WriteBuffer() returns a
    Number if all data has been written.

    I have run `make jslint test`, found two problems, and changed the test
    code.
    1. In /test/simple/test-debugger-repl.js, one of the child processes
    always does not exit, I didn't found the reason, and changed the finish
    cleanup behaviours, and passed the test. But I still don't know what's the
    problem.

    2. In /test/simple/test-tcp-wrap-listen.js, because I changed the return
    value of StreamWrap::WriteBuffer(), so the test should be modified.

    It's still not a perfect patch, add such a sync feature to async node is
    somehow incompatible with the code logic. And I've noticed that the stream
    related code is not stable yet, so if this patch is acceptable, more
    imporvements can be made later.

    btw: windows version has not been changed yet.

    issue link:
    https://github.com/joyent/node/issues/4699

    --
    --
    Job Board: http://jobs.nodejs.org/
    Posting guidelines:
    https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
    You received this message because you are subscribed to the Google
    Groups "nodejs" group.
    To post to this group, send email to nod...@googlegroups.com<javascript:>
    To unsubscribe from this group, send email to
    nodejs+un...@googlegroups.com <javascript:>
    For more options, visit this group at
    http://groups.google.com/group/nodejs?hl=en?hl=en

    ---
    You received this message because you are subscribed to the Google Groups
    "nodejs" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to nodejs+un...@googlegroups.com <javascript:>.
    For more options, visit https://groups.google.com/groups/opt_out.

    --
    --
    Job Board: http://jobs.nodejs.org/
    Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
    You received this message because you are subscribed to the Google
    Groups "nodejs" group.
    To post to this group, send email to nodejs@googlegroups.com
    To unsubscribe from this group, send email to
    nodejs+unsubscribe@googlegroups.com
    For more options, visit this group at
    http://groups.google.com/group/nodejs?hl=en?hl=en

    ---
    You received this message because you are subscribed to the Google Groups "nodejs" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to nodejs+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Ben Noordhuis at Feb 1, 2013 at 11:57 am

    On Fri, Feb 1, 2013 at 9:42 AM, darcy wrote:
    Yes, I mean write+callback can be sync in some cases.
    If the callback should be promised to be async, we can use
    process.nextTick(cb.bind(this)) in Socket._write().
    But I think in userland it's not important whether write callback is sync or
    async. And in current implement callback is also sync if some error occurs.
    The callback should always be asynchronous (i.e. run on the next
    tick), or you introduce the risk of stack overflows.

    Consider this code:

    var conn = net.connect(/* ... */, function() {
    function write() { conn.write('PING', write) }
    write();
    });

    If the network connection is fast enough that the write always
    succeeds and the callback is synchronous, you'll overflow the call
    stack in seconds flat.

    --
    --
    Job Board: http://jobs.nodejs.org/
    Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
    You received this message because you are subscribed to the Google
    Groups "nodejs" group.
    To post to this group, send email to nodejs@googlegroups.com
    To unsubscribe from this group, send email to
    nodejs+unsubscribe@googlegroups.com
    For more options, visit this group at
    http://groups.google.com/group/nodejs?hl=en?hl=en

    ---
    You received this message because you are subscribed to the Google Groups "nodejs" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to nodejs+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Darcy at Feb 1, 2013 at 3:25 pm
    ok, I'll use process.nextTick instead.
    On 2月1日, 下午7时50分, Ben Noordhuis wrote:
    On Fri, Feb 1, 2013 at 9:42 AM, darcy wrote:
    Yes, I mean write+callback can be sync in some cases.
    If the callback should be promised to be async, we can use
    process.nextTick(cb.bind(this)) in Socket._write().
    But I think in userland it's not important whether write callback is sync or
    async. And in current implement callback is also sync if some error occurs.
    The callback should always be asynchronous (i.e. run on the next
    tick), or you introduce the risk of stack overflows.

    Consider this code:

    var conn = net.connect(/* ... */, function() {
    function write() { conn.write('PING', write) }
    write();
    });

    If the network connection is fast enough that the write always
    succeeds and the callback is synchronous, you'll overflow the call
    stack in seconds flat.
    --
    --
    Job Board: http://jobs.nodejs.org/
    Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
    You received this message because you are subscribed to the Google
    Groups "nodejs" group.
    To post to this group, send email to nodejs@googlegroups.com
    To unsubscribe from this group, send email to
    nodejs+unsubscribe@googlegroups.com
    For more options, visit this group at
    http://groups.google.com/group/nodejs?hl=en?hl=en

    ---
    You received this message because you are subscribed to the Google Groups "nodejs" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to nodejs+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Isaac Schlueter at Feb 1, 2013 at 4:29 pm

    On Fri, Feb 1, 2013 at 3:50 AM, Ben Noordhuis wrote:
    Consider this code:

    var conn = net.connect(/* ... */, function() {
    function write() { conn.write('PING', write) }
    write();
    });

    If the network connection is fast enough that the write always
    succeeds and the callback is synchronous, you'll overflow the call
    stack in seconds flat.
    In this case, it's fine. Even if it runs synchronously in the binding
    layer, the stream wrapper will detect this and nextTick the cb. Note
    that we call the cb synchronously a few lines below if the queue size
    is 0.

    --
    --
    Job Board: http://jobs.nodejs.org/
    Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
    You received this message because you are subscribed to the Google
    Groups "nodejs" group.
    To post to this group, send email to nodejs@googlegroups.com
    To unsubscribe from this group, send email to
    nodejs+unsubscribe@googlegroups.com
    For more options, visit this group at
    http://groups.google.com/group/nodejs?hl=en?hl=en

    ---
    You received this message because you are subscribed to the Google Groups "nodejs" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to nodejs+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Darcy at Feb 2, 2013 at 1:13 pm
    Yes, it's exactly works as Isaac said, call cb in net._write synchronously
    will be "asynced" by Writable.write if neccessary.

    Well, I want to know is this patch worthy to make a pull request?
    Should I make pull request to node? Or part to libuv and part to node?
    Thanks!
    On Saturday, February 2, 2013 12:29:32 AM UTC+8, Isaac Schlueter wrote:
    On Fri, Feb 1, 2013 at 3:50 AM, Ben Noordhuis wrote:
    Consider this code:

    var conn = net.connect(/* ... */, function() {
    function write() { conn.write('PING', write) }
    write();
    });

    If the network connection is fast enough that the write always
    succeeds and the callback is synchronous, you'll overflow the call
    stack in seconds flat.
    In this case, it's fine. Even if it runs synchronously in the binding
    layer, the stream wrapper will detect this and nextTick the cb. Note
    that we call the cb synchronously a few lines below if the queue size
    is 0.
    --
    --
    Job Board: http://jobs.nodejs.org/
    Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
    You received this message because you are subscribed to the Google
    Groups "nodejs" group.
    To post to this group, send email to nodejs@googlegroups.com
    To unsubscribe from this group, send email to
    nodejs+unsubscribe@googlegroups.com
    For more options, visit this group at
    http://groups.google.com/group/nodejs?hl=en?hl=en

    ---
    You received this message because you are subscribed to the Google Groups "nodejs" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to nodejs+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Mark Hahn at Feb 1, 2013 at 6:41 pm
    One would assume that there is some other async code happening. You have
    to get your data from somewhere and that somewhere is usually async.

    On Fri, Feb 1, 2013 at 3:50 AM, Ben Noordhuis wrote:
    On Fri, Feb 1, 2013 at 9:42 AM, darcy wrote:
    Yes, I mean write+callback can be sync in some cases.
    If the callback should be promised to be async, we can use
    process.nextTick(cb.bind(this)) in Socket._write().
    But I think in userland it's not important whether write callback is sync or
    async. And in current implement callback is also sync if some error
    occurs.

    The callback should always be asynchronous (i.e. run on the next
    tick), or you introduce the risk of stack overflows.

    Consider this code:

    var conn = net.connect(/* ... */, function() {
    function write() { conn.write('PING', write) }
    write();
    });

    If the network connection is fast enough that the write always
    succeeds and the callback is synchronous, you'll overflow the call
    stack in seconds flat.

    --
    --
    Job Board: http://jobs.nodejs.org/
    Posting guidelines:
    https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
    You received this message because you are subscribed to the Google
    Groups "nodejs" group.
    To post to this group, send email to nodejs@googlegroups.com
    To unsubscribe from this group, send email to
    nodejs+unsubscribe@googlegroups.com
    For more options, visit this group at
    http://groups.google.com/group/nodejs?hl=en?hl=en

    ---
    You received this message because you are subscribed to the Google Groups
    "nodejs" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to nodejs+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

    --
    --
    Job Board: http://jobs.nodejs.org/
    Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
    You received this message because you are subscribed to the Google
    Groups "nodejs" group.
    To post to this group, send email to nodejs@googlegroups.com
    To unsubscribe from this group, send email to
    nodejs+unsubscribe@googlegroups.com
    For more options, visit this group at
    http://groups.google.com/group/nodejs?hl=en?hl=en

    ---
    You received this message because you are subscribed to the Google Groups "nodejs" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to nodejs+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupnodejs @
categoriesnodejs
postedFeb 1, '13 at 3:41a
activeFeb 2, '13 at 1:13p
posts8
users4
websitenodejs.org
irc#node.js

People

Translate

site design / logo © 2022 Grokbase