----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24159/#review49376 -----------------------------------------------------------
I'm not really able to fully evaluate this change. Overall it seems like a lot of code. If it passes the tests I'd be happy to ship it: Having said that I do have comments/issues, mostly reasonably simple ones. http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c <https://reviews.apache.org/r/24159/#comment86376> Nit: Don't need the "./" http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c <https://reviews.apache.org/r/24159/#comment86377> Nit: Shouldn't need "../" in current build. http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c <https://reviews.apache.org/r/24159/#comment86379> Don't start identifier with - bad practice (IMO) even if technically allowed by standard. http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c <https://reviews.apache.org/r/24159/#comment86380> I think this is really "ensuring" not just checking so I'd rename it to "ensure_unique". http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c <https://reviews.apache.org/r/24159/#comment86382> Spelling: "straggler" http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c <https://reviews.apache.org/r/24159/#comment86384> 50 seems arbitrary. Care to explain. http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/iocp.c <https://reviews.apache.org/r/24159/#comment86394> As above bad name. http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/iocp.c <https://reviews.apache.org/r/24159/#comment86400> This is a strange name: does it actually read 0 bytes? - Andrew Stitcher On July 31, 2014, 6 p.m., Cliff Jansen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24159/ > ----------------------------------------------------------- > > (Updated July 31, 2014, 6 p.m.) > > > Review request for qpid and Rafael Schloming. > > > Bugs: PROTON-640 > https://issues.apache.org/jira/browse/PROTON-640 > > > Repository: qpid > > > Description > ------- > > This patch follows the QPID AsynchIO.cpp version fairly closely with the > following main differences: > > - addition of async connect > - multiple outstanding concurrent writes (a write pipeline) > - non-buffered reads > - graceful close progressing to hard close (much as proposed in QPID-5668) > > Careful scrutiny of the selector API change is warranted (constructor). > > Thread safety is currently assured only by isolating a paired pn_selector_t > and pn_io_t from other such pairs. If anticipated use cases suggest either > that multiple selectors be used with an io, or with multiple io's, or that a > socket should be movable between selectors during it's lifetime, then > additional locking semantics will be required. These scenarios come for free > in Linux where the OS barrier takes care of concurrency issues. By contrast, > the Windows code implements the selector in user space. > > This API change just forces the pairing of the selector with the io. The > user is still expected to free the selector when done. Perhaps this should > be handled by the io when it closes, or some alternative API mechanism should > be used. In any event, if the supported use cases preclude closely linking > one selector with one io, then this is moot. > > Performance notes. The use of a write pipeline has a significant effect on > sending performance, especially with a small number of connections. It would > probably be a useful enhancement for the QPID code. On the read side, no > async read buffer is used (relying instead on the OS input buffering system). > This is because pn_recv() copies out the bytes anyway (rather than passing a > buffer as in QPID). > > > Diffs > ----- > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt > 1614939 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/io.h > 1614939 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger/messenger.c > 1614939 > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/io.c > 1614939 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/iocp.h > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/iocp.c > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/selector.c > 1614939 > > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/write_pipeline.c > PRE-CREATION > > Diff: https://reviews.apache.org/r/24159/diff/ > > > Testing > ------- > > 2000 msgr-send simultaneous connections to a "msgr-recv -R" instance. 32/64 > bit. winxp and win7. > > > Thanks, > > Cliff Jansen > >
