-
Notifications
You must be signed in to change notification settings - Fork 6
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
Changes to rcu_obj_base and rcu_retire #13
base: master
Are you sure you want to change the base?
Conversation
Test/paulmck/rcu.hpp
Outdated
class rcu_obj_base: private rcu_head { | ||
D deleter; | ||
template<typename T, typename D = default_delete<T>> | ||
class rcu_obj_base: private rcu_head, private std::tuple<D> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nifty. I've never seen this pattern before and am thus mildly skeptical of it; but I don't immediately see anything wrong with it. It's also an improvement over my hack (on old line 44) of default-constructing a deleter every time we need one — that hack was definitely bad.
Does the Standard (or at least any major vendor) actually guarantee that is_empty_v<tuple<E>>
whenever is_empty_v<E>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find anything in the standard I am afraid. The reason why I am suggesting this is that libstdc++'s std::unique_ptr
implementation stores its pointer and deleter like this:
typedef std::tuple<typename _Pointer::type, _Dp> __tuple_type;
__tuple_type _M_t;
libstdc++ does not actually provide that guarantee. It will disable EBO in if the argument is a final class or a tuple. According to their comments,
// Using EBO for elements that are tuples causes ambiguous base errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh; well, having a member of type tuple<X,E>
is a bit different from inheriting from X, tuple<E>
. I'd much prefer that we (which is to say "not me" for the foreseeable future) find a way of triggering the empty-base optimization that definitely does work on current GCC, Clang, and MSVC.
I think this std::tuple
trick works for libstdc++ but not for libc++.
https://wandbox.org/permlink/H0PB0OjM9dH6kVCa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it on clang, assuming that it would use libc++. It looks like I was wrong. When I installed libc++ and told clang to use it, the empty base class optimization stopped working.
I will try to find something else that works better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find anything else in the standard library, but it looks like boost::compressed_pair will do EBO: https://wandbox.org/permlink/xbT0JaR5UNy4ZOzQ
I suppose it could be used like this:
namespace detail {
struct rcu_obj_base_empty_type {};
}
template<typename T, typename D = default_delete<T>>
class rcu_obj_base:
private rcu_head,
private boost::compressed_pair<D, detail::rcu_obj_base_empty_type> {
// ...
Otherwise, I suppose that we will need to roll our own.
Test/paulmck/rcu.hpp
Outdated
public ptr_type { | ||
rcu_obj_base_ni(ptr_type p) : ptr_type(std::move(p)) {} | ||
}; | ||
(new rcu_obj_base_ni(std::move(ptr)))->retire(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right. new rcu_obj_base_ni
causes memory allocation, doesn't it? At retire time? On a system that may not even have a heap?
But maybe the old code also had this bug (if it is a bug). This code doesn't look familiar to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version also allocated memory on the free path. There aren't all that many options when T doesn't have spare memory to store the deleter and the rcu_head.
It might be good to support custom allocators though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then I have no objection to changing the old code to this new code. However, I protest that I don't understand where the old code came from or why it's suddenly a good idea to allocate on the retire path. :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it isn't a good idea to allocate on the free path. However, I don't think that rcu_retire has any other option.
This fixes a discrepancy between nonempty deleters, which will get constructed when the rcu_obj_base is constructed, and empty deleters, which were only getting constructed when retire() is run. Signed-off-by: Lance Roy <[email protected]>
I added an unique_ptr overload of rcu_retire just in case you want it. Otherwise, this PR is just simplification.