Skip to content

Fix existing ref counting classes and add new ones.#13984

Merged
markdroth merged 5 commits intogrpc:masterfrom
markdroth:ref_counting
Jan 17, 2018
Merged

Fix existing ref counting classes and add new ones.#13984
markdroth merged 5 commits intogrpc:masterfrom
markdroth:ref_counting

Conversation

@markdroth
Copy link
Copy Markdown
Member

I am splitting this out of #13684 so that it can be used elsewhere.

Specific changes:

  • Fix bugs that prevented the use of the RefCounted and RefCountedWithTracing classes.
  • Implement new Orphanable class and a subclass called InternallyRefCountedWithTracing.

@markdroth markdroth requested a review from dgquintas January 11, 2018 15:45
@grpc-testing
Copy link
Copy Markdown

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link
Copy Markdown

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link
Copy Markdown

[trickle] No significant performance differences

@markdroth markdroth mentioned this pull request Jan 12, 2018
@grpc-testing
Copy link
Copy Markdown

[microbenchmarks] No significant performance differences


namespace grpc_core {

// A base class for orphanable objects.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand on the definition of "orphanable"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

class Orphanable {
public:
// Gives up ownership of the object. The implementation must arrange
// to destroy the object without further interaction from the caller.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add "... must arrange to eventually destroy ..."

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

};

template <typename T, typename Deleter = OrphanableDelete<T>>
using OrphanablePtr = std::unique_ptr<T, Deleter>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of that is that code outside of lib/support will use the abstractions provided by lib/support instead of directly using things provided in the standard library. But code in lib/support can use things provided by the standard library as long as we don't introduce a run-time dependency. In the case of std::unique_ptr<>, the only part that would impose a run-time dependency is the deleter, which we're overriding here.

Note that we do already define grpc_core::UniquePtr<>, which is basically defined as std::unique_ptr<> with a deleter that calls gpr_free():

https://github.com/grpc/grpc/blob/master/src/core/lib/support/memory.h#L51

bool operator==(const RefCountedPtr& other) const {
return value_ == other.value_;
}
bool operator==(T* other) const {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you probably want to make the argument be const T* other. Same on line 88

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@grpc-testing
Copy link
Copy Markdown

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link
Copy Markdown

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

[microbenchmarks] No significant performance differences

@markdroth
Copy link
Copy Markdown
Member Author

Known issues: #13477

@markdroth markdroth merged commit 8afe8d3 into grpc:master Jan 17, 2018
@markdroth markdroth deleted the ref_counting branch January 17, 2018 16:58
@markdroth markdroth mentioned this pull request Jan 17, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants