You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Cliff Jansen <cl...@gmail.com> on 2014/07/31 20:00:56 UTC
Review Request 24159: Provide a native IO completion port layer similar to
the QPID version for Proton.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24159/
-----------------------------------------------------------
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
Re: Review Request 24159: Provide a native IO completion port layer similar
to the QPID version for Proton.
Posted by Cliff Jansen <cl...@gmail.com>.
> On Aug. 1, 2014, 8:58 p.m., Andrew Stitcher wrote:
> > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/windows/iocp.c, line 577
> > <https://reviews.apache.org/r/24159/diff/1/?file=646944#file646944line577>
> >
> > This is a strange name: does it actually read 0 bytes?
Yes it does. The accepted way to get an async notification that unread bytes are available for reading on an IOCP registered handle is to perform a normal IOCP-capable read operation with a byte count of zero. complete_read() is where the IOCP completion event from the completion port is handled (just marks the selectable as PN_READABLE).
Thanks very much for the review. I will fix the other raised issues as suggested.
- Cliff
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24159/#review49376
-----------------------------------------------------------
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
>
>
Re: Review Request 24159: Provide a native IO completion port layer similar
to the QPID version for Proton.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 24159: Provide a native IO completion port layer similar
to the QPID version for Proton.
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24159/#review49772
-----------------------------------------------------------
Ship it!
I've only really looked at the non windows portion of this, but it seems fine to me.
- Rafael Schloming
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
>
>