Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace explicitly ref-counted objects with smart pointers? #241

Closed
ksubramz opened this issue Oct 6, 2017 · 2 comments
Closed

Replace explicitly ref-counted objects with smart pointers? #241

ksubramz opened this issue Oct 6, 2017 · 2 comments

Comments

@ksubramz
Copy link
Contributor

ksubramz commented Oct 6, 2017

ServiceReferenceBasePrivate

  • ServiceReferenceBase owns the ServiceReferenceBasePrivate object and holds an atomic raw pointer to it.
  • The underlying ServiceReferenceBasePrivate object holds an atomic ref count that indicates how many ServiceReferenceBase objects refer to it.
  • When a ServiceReference's operation makes this ref count zero, it deletes the underlying ServiceReferenceBasePrivate object.

The only thing we guarantee about the underlying ServiceReferenceBasePrivate object is assignment and copying are thread-safe.

Can this be replaced by a shared_ptr, which solves the same problem elegantly?

My guess as to the reason why shared_ptr wasn't chosen is that shared_ptrs are only partially thread-safe - the control block is thread-safe but accessing the actual resource is not thread-safe. But, we do have C++11-compatible atomic_* free functions specialized for shared_ptrs that guarantee that the underlying resource can be safely accessed in a multi-threaded setting (http://en.cppreference.com/w/cpp/memory/shared_ptr/atomic)

Pros:

  • Idiomatic & Modern.
  • Eliminates raw pointer, manual ref counting and automates destruction

Cons:

Key Changes:

In ServiceReferenceBase,

-std::atomic<ServiceReferenceBasePrivate *> d;
+std::shared_ptr<ServiceReferenceBasePrivate> d;

In ServiceReferenceBasePrivate

-std::atomic<int> ref;

Additionally, there will be changes in the code to use atomic_* free functions when accessing the underlying resource.

See also: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4058.pdf (Appendix)


ServiceRegistrationBasePrivate

  • ServiceReferenceBasePrivate and ServiceRegistrationBase each hold a raw pointer to the ServiceRegistrationBasePrivate object
  • In turn, the ServiceRegistrationBasePrivate object has a reference count to indicate how many ServiceReferenceBasePrivate and ServiceRegistrationBase objects refer to it.
  • When the reference count becomes zero, one of the ServiceReferenceBasePrivate and ServiceRegistrationBase object is responsible for deleting the ServiceRegistrationBasePrivate object.

Similarly, can't this mechanism can be replaced by shared_ptrs in both ServiceReferenceBasePrivate and ServiceRegistrationBase classes? i.e.

In ServiceRegistrationBase,

-ServiceRegistrationBasePrivate* d;
+std::shared_ptr<ServiceRegistrationBasePrivate> d;

In ServiceReferenceBasePrivate,

-ServiceRegistrationBasePrivate* const registration;
+const std::shared_ptr<ServiceRegistrationBasePrivate> registration;

In ServiceRegistrationBasePrivate,

-std::atomic<int> ref;

And other related changes.

@saschazelzer
Copy link
Member

I need to read and think about this in more detail later. Just a quick note: std::shared_ptr wasn't used before because the code predates C++11 support in the code-base. I would love to see it being replaced with something more idiomatic, if possible.

@jeffdiclemente
Copy link
Member

closing as this has essentially been resolved by #841

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants