Project

General

Profile

Task #2613

Eliminate unnecessary usage of shared_ptr in faces and channels

Added by Junxiao Shi over 4 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Category:
Faces
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
3.00 h

Description

In particular:

  • The boost::asio::*::acceptor instance can be a simple member field, as it's not shared with any other object.
  • Sockets can use C++11 move semantics (natively supported by boost) when ownership is transferred from the channel to the face.

History

#1 Updated by Davide Pesavento over 4 years ago

  • Related to Task #1912: Stop using const shared_ptr<T>& parameter type added

#2 Updated by Junxiao Shi over 4 years ago

I basically agree with this issue.

There's one problem to consider: is there a situation that a socket could be shared across multiple faces?

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.

In those situations, shared_ptr shall be kept.

#3 Updated by Davide Pesavento over 4 years ago

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?

#4 Updated by Davide Pesavento over 4 years ago

  • Description updated (diff)

The same change should be done for boost::asio::*::acceptor in all channels. Would you prefer a separate commit for that?

#5 Updated by Junxiao Shi over 4 years ago

make_unique needs a separate commit, in order to highlight its availability in COMMIT_MSG.

I don't care about whether socket and acceptor are separated.

#6 Updated by Davide Pesavento over 4 years ago

Junxiao Shi wrote:

make_unique needs a separate commit, in order to highlight its availability in COMMIT_MSG.

Should it go in ndn-cxx?

#7 Updated by Junxiao Shi over 4 years ago

make_unique needs a separate commit, in order to highlight its availability in COMMIT_MSG.

Should it go in ndn-cxx?

You can.
But if you put it into <ndn-cxx/common.hpp>, NFD is disallowed to using ndn::make_shared due to #2098.

So this needs a separate header in ndn-cxx/util in order to allow usage from NFD and other projects.

#8 Updated by Alex Afanasyev over 4 years ago

Just to records the reason why make_unique could be necessary: http://herbsutter.com/2013/05/29/gotw-89-solution-smart-pointers/ (question no.3)

#9 Updated by Davide Pesavento over 4 years ago

  • % Done changed from 0 to 20

I've encountered a problem.

UnixStreamFace::UnixStreamFace(unique_ptr<UnixStreamFace::protocol::socket> socket)
  : StreamFace<protocol, LocalFace>(FaceUri::fromFd(socket->native_handle()),
                                    FaceUri(socket->local_endpoint()),
                                    std::move(socket), true)
{
}

The move can happen before or after the dereferencing of socket for FaceUri 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.

#10 Updated by Davide Pesavento over 4 years ago

(Unrelated to my previous comment) Alex says, about the new unique_ptr<socket> member variable:

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.
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.

#11 Updated by Davide Pesavento over 4 years ago

What do you propose then?

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:

Ownership of the socket object is retained by the caller, which must guarantee that it is valid until the handler is called.

Strictly speaking, I'm not sure if our current code satisfies this requirement.

#12 Updated by Alex Afanasyev over 4 years ago

What do you propose then?

I wish it was possible to just use unique_ptr 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...

Strictly speaking, I'm not sure if our current code satisfies this requirement.

Yes, I know a few places where code doesn't satisfy this requirement. I just don't feel good about introducing new such places...

#13 Updated by Davide Pesavento over 4 years ago

Alex Afanasyev wrote:

Yes, I know a few places where code doesn't satisfy this requirement. I just don't feel good about introducing new such places...

My patch doesn't introduce any new violations, in fact it may fix some.

#14 Updated by Davide Pesavento over 4 years ago

Replying to a previous concern from Alex:

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.

I don't see how the proposed change prevents using a thread pool. Even with a thread pool (and one io_service per thread) you will still have one single acceptor and one single async_accept 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 unique_ptr<socket> as a member variable of the channel doesn't impose any additional limitations.

#15 Updated by Davide Pesavento over 4 years ago

  • Subject changed from Use unique_ptr for sockets to Use move semantics for sockets
  • Description updated (diff)

Actually, boost sockets natively support move semantics, so unique_ptr is not needed.

#16 Updated by Alex Afanasyev over 4 years ago

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 async_accept at a given time.


I hope there are no issues with socket move semantics in older boost (1.48).

#17 Updated by Davide Pesavento over 4 years ago

Alex Afanasyev wrote:

I hope there are no issues with socket move semantics in older boost (1.48).

According to http://www.boost.org/doc/libs/1_57_0/doc/html/boost_asio/history.html , move semantics for sockets and POSIX descriptors is available since "Asio 1.6.0 / Boost 1.47".

#18 Updated by Alex Afanasyev over 4 years ago

Great. I just worry a little bit that this list not necessarily covers some temporary issues that could have existed at some point.

#19 Updated by Davide Pesavento over 4 years ago

I'm still waiting for suggestions on the problem outlined in note-9

#20 Updated by Alex Afanasyev over 4 years ago

I think I suggested (may be on gerrit) to extend constructor parameters for UnixStreamFace.

Another not so nice way is to create setLocalUri/setRemoteUri methods.

Yet another, customize StreamFace constructor for each protocol type and do faceUri assignments there

#21 Updated by Davide Pesavento over 4 years ago

  • Subject changed from Use move semantics for sockets to Eliminate unnecessary usage of shared_ptr in faces and channels
  • Description updated (diff)
  • % Done changed from 20 to 30

#22 Updated by Davide Pesavento over 4 years ago

  • % Done changed from 30 to 40

First part (acceptors): http://gerrit.named-data.net/1846

#23 Updated by Junxiao Shi over 4 years ago

  • Related to deleted (Task #1912: Stop using const shared_ptr<T>& parameter type)

#24 Updated by Junxiao Shi over 4 years ago

#1912 is irrelevant: it's about passing shared_ptr by reference, not about usage of shared_ptr.

#25 Updated by Davide Pesavento over 4 years ago

Well, "passing shared_ptr by reference" is a "usage of shared_ptr"... Moreover, in the discussion that developed in #1912 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...

#26 Updated by Davide Pesavento over 4 years ago

  • % Done changed from 40 to 80

Second part (sockets): http://gerrit.named-data.net/1830

#27 Updated by Davide Pesavento over 4 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 80 to 100

#28 Updated by Davide Pesavento over 4 years ago

  • Status changed from Code review to Closed

Also available in: Atom PDF