https://redmine.named-data.net/https://redmine.named-data.net/favicon.ico?14759811232015-03-03T08:58:31ZNDN project issue tracking systemNFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=87882015-03-03T08:58:31ZDavide Pesavento
<ul><li><strong>Related to</strong> <i><a class="issue tracker-3 status-6 priority-2 priority-default closed" href="/issues/1912">Task #1912</a>: Stop using const shared_ptr<T>& parameter type</i> added</li></ul> NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=87902015-03-03T09:00:59ZJunxiao Shi
<ul></ul><p>I basically agree with this issue.</p>
<p>There's one problem to consider: is there a situation that a socket could be shared across multiple faces?<br><br>
Ethernet unicast faces on the same NIC may need to share the pcap handle in order to send packets, while receiving is dispatched by the channel.<br><br>
In those situations, <code>shared_ptr</code> shall be kept.</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=87912015-03-03T09:04:49ZDavide Pesavento
<ul></ul><p>There's no such thing as a unicast Ethernet face (yet). In any case, why can't you have multiple pcap handles on the same interface?</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=87922015-03-03T09:12:42ZDavide Pesavento
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/8792/diff?detail_id=8062">diff</a>)</li></ul><p>The same change should be done for <code>boost::asio::*::acceptor</code> in all channels. Would you prefer a separate commit for that?</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=87932015-03-03T09:16:27ZJunxiao Shi
<ul></ul><p><code>make_unique</code> needs a separate commit, in order to highlight its availability in COMMIT_MSG.</p>
<p>I don't care about whether socket and acceptor are separated.</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=87942015-03-03T09:23:02ZDavide Pesavento
<ul></ul><p>Junxiao Shi wrote:</p>
<blockquote>
<p><code>make_unique</code> needs a separate commit, in order to highlight its availability in COMMIT_MSG.</p>
</blockquote>
<p>Should it go in ndn-cxx?</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=87952015-03-03T09:27:23ZJunxiao Shi
<ul></ul><blockquote>
<blockquote>
<p><code>make_unique</code> needs a separate commit, in order to highlight its availability in COMMIT_MSG.</p>
</blockquote>
<p>Should it go in ndn-cxx?</p>
</blockquote>
<p>You can.<br>
But if you put it into <code><ndn-cxx/common.hpp></code>, NFD is disallowed to <code>using ndn::make_shared</code> due to <a class="issue tracker-3 status-5 priority-2 priority-default closed" title="Task: Mark common.hpp as implementation detail (Closed)" href="https://redmine.named-data.net/issues/2098">#2098</a>.<br><br>
So this needs a separate header in ndn-cxx/util in order to allow usage from NFD and other projects.</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=88002015-03-03T10:42:36ZAlex Afanasyev
<ul></ul><p>Just to records the reason why <code>make_unique</code> could be necessary: <a href="http://herbsutter.com/2013/05/29/gotw-89-solution-smart-pointers/">http://herbsutter.com/2013/05/29/gotw-89-solution-smart-pointers/</a> (question no.3)</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=88032015-03-03T12:47:22ZDavide Pesavento
<ul><li><strong>% Done</strong> changed from <i>0</i> to <i>20</i></li></ul><p>I've encountered a problem.</p>
<pre><code>UnixStreamFace::UnixStreamFace(unique_ptr<UnixStreamFace::protocol::socket> socket)
: StreamFace<protocol, LocalFace>(FaceUri::fromFd(socket->native_handle()),
FaceUri(socket->local_endpoint()),
std::move(socket), true)
{
}
</code></pre>
<p>The move can happen before or after the dereferencing of <code>socket</code> for <code>FaceUri</code> construction, because the order of parameter evaluation is unspecified in C++. If the move happens before any deref, the above code will result in a crash.</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=88042015-03-03T13:08:36ZDavide Pesavento
<ul></ul><p>(Unrelated to my previous comment) Alex says, about the new <code>unique_ptr<socket></code> member variable:</p>
<blockquote>
<p>honestly, i don't like this to be member variable, as it is just a transitional value. This change prevents any future optimization with thread pooling.<br>
In different words. My opinion is that class members should be only about state that is inherent to the abstraction (channel). This socket that will belong to the Face should not belong (at any stage of the process) to the channel.</p>
</blockquote>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=88052015-03-03T13:09:40ZDavide Pesavento
<ul></ul><p>What do you propose then?</p>
<p>Right now, the socket is not really owned by anyone, it is kept alive only because it is bound to the handler and Asio happens to keep a copy of the handler. Asio doc clearly states:</p>
<blockquote>
<p>Ownership of the socket object is retained by the caller, which must guarantee that it is valid until the handler is called.</p>
</blockquote>
<p>Strictly speaking, I'm not sure if our current code satisfies this requirement.</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=88092015-03-03T14:41:52ZAlex Afanasyev
<ul></ul><blockquote>
<p>What do you propose then?</p>
</blockquote>
<p>I wish it was possible to just use <code>unique_ptr</code> for bind call, so the socket is just owned by bind until it is finally assigned to the face. If it is not possible (and I'm guessing that it is not), so I would prefer keeping shared_ptr, at least for the bind phase...</p>
<blockquote>
<p>Strictly speaking, I'm not sure if our current code satisfies this requirement.</p>
</blockquote>
<p>Yes, I know a few places where code doesn't satisfy this requirement. I just don't feel good about introducing new such places...</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=88102015-03-03T15:14:01ZDavide Pesavento
<ul></ul><p>Alex Afanasyev wrote:</p>
<blockquote>
<p>Yes, I know a few places where code doesn't satisfy this requirement. I just don't feel good about introducing new such places...</p>
</blockquote>
<p>My patch doesn't introduce any new violations, in fact it may fix some.</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=88122015-03-03T17:28:33ZDavide Pesavento
<ul></ul><p>Replying to a previous concern from Alex:</p>
<blockquote>
<p>honestly, i don't like this to be member variable, as it is just a transitional value. This change prevents any future optimization with thread pooling.</p>
</blockquote>
<p>I don't see how the proposed change prevents using a thread pool. Even with a thread pool (and one <code>io_service</code> per thread) you will still have one single <code>acceptor</code> and one single <code>async_accept</code> outstanding at any given time. In other words, the channel will still have to deal with at most one socket at a time. Therefore keeping a <code>unique_ptr<socket></code> as a member variable of the channel doesn't impose any additional limitations.</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=88352015-03-04T10:01:56ZDavide Pesavento
<ul><li><strong>Subject</strong> changed from <i>Use unique_ptr for sockets</i> to <i>Use move semantics for sockets</i></li><li><strong>Description</strong> updated (<a title="View differences" href="/journals/8835/diff?detail_id=8099">diff</a>)</li></ul><p>Actually, boost sockets natively support move semantics, so <code>unique_ptr</code> is not needed.</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=88572015-03-04T16:58:55ZAlex Afanasyev
<ul></ul><p>Davide, I got the point and ok with the approach you're taking. It's just felt a little strange as I forgot that there is exactly one <code>async_accept</code> at a given time.</p>
<hr>
<p>I hope there are no issues with socket move semantics in older boost (1.48).</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=89062015-03-05T15:22:14ZDavide Pesavento
<ul></ul><p>Alex Afanasyev wrote:</p>
<blockquote>
<p>I hope there are no issues with socket move semantics in older boost (1.48).</p>
</blockquote>
<p>According to <a href="http://www.boost.org/doc/libs/1_57_0/doc/html/boost_asio/history.html">http://www.boost.org/doc/libs/1_57_0/doc/html/boost_asio/history.html</a> , move semantics for sockets and POSIX descriptors is available since "Asio 1.6.0 / Boost 1.47".</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=89092015-03-05T15:27:36ZAlex Afanasyev
<ul></ul><p>Great. I just worry a little bit that this list not necessarily covers some temporary issues that could have existed at some point.</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=89482015-03-06T16:08:47ZDavide Pesavento
<ul></ul><p>I'm still waiting for suggestions on the problem outlined in note-9</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=89492015-03-06T16:19:18ZAlex Afanasyev
<ul></ul><p>I think I suggested (may be on gerrit) to extend constructor parameters for UnixStreamFace.</p>
<p>Another not so nice way is to create setLocalUri/setRemoteUri methods.</p>
<p>Yet another, customize StreamFace constructor for each protocol type and do faceUri assignments there</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=89542015-03-08T16:01:50ZDavide Pesavento
<ul><li><strong>Subject</strong> changed from <i>Use move semantics for sockets</i> to <i>Eliminate unnecessary usage of shared_ptr in faces and channels</i></li><li><strong>Description</strong> updated (<a title="View differences" href="/journals/8954/diff?detail_id=8184">diff</a>)</li><li><strong>% Done</strong> changed from <i>20</i> to <i>30</i></li></ul> NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=89572015-03-08T19:50:30ZDavide Pesavento
<ul><li><strong>% Done</strong> changed from <i>30</i> to <i>40</i></li></ul><p>First part (acceptors): <a href="http://gerrit.named-data.net/1846">http://gerrit.named-data.net/1846</a></p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=90182015-03-09T20:39:34ZJunxiao Shi
<ul><li><strong>Related to</strong> deleted (<i><a class="issue tracker-3 status-6 priority-2 priority-default closed" href="/issues/1912">Task #1912</a>: Stop using const shared_ptr<T>& parameter type</i>)</li></ul> NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=90202015-03-09T20:41:41ZJunxiao Shi
<ul></ul><p><a class="issue tracker-3 status-6 priority-2 priority-default closed" title="Task: Stop using const shared_ptr<T>& parameter type (Rejected)" href="https://redmine.named-data.net/issues/1912">#1912</a> is irrelevant: it's about passing shared_ptr by reference, not about usage of shared_ptr.</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=90212015-03-09T20:53:03ZDavide Pesavento
<ul></ul><p>Well, "passing shared_ptr by reference" <em>is</em> a "usage of shared_ptr"... Moreover, in the discussion that developed in <a class="issue tracker-3 status-6 priority-2 priority-default closed" title="Task: Stop using const shared_ptr<T>& parameter type (Rejected)" href="https://redmine.named-data.net/issues/1912">#1912</a> we agreed to follow Sutter's guidelines, and this task is very related to that (don't use shared_ptr if there's no shared ownership; use by-value unique_ptr or move semantics for "sinks"). Anyway, I don't really care...</p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=90892015-03-12T18:35:56ZDavide Pesavento
<ul><li><strong>% Done</strong> changed from <i>40</i> to <i>80</i></li></ul><p>Second part (sockets): <a href="http://gerrit.named-data.net/1830">http://gerrit.named-data.net/1830</a></p>
NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=91522015-03-15T16:06:23ZDavide Pesavento
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Code review</i></li><li><strong>% Done</strong> changed from <i>80</i> to <i>100</i></li></ul> NFD - Task #2613: Eliminate unnecessary usage of shared_ptr in faces and channelshttps://redmine.named-data.net/issues/2613?journal_id=92442015-03-19T06:45:36ZDavide Pesavento
<ul><li><strong>Status</strong> changed from <i>Code review</i> to <i>Closed</i></li></ul>