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
♪ Any singleton will do #20273
♪ Any singleton will do #20273
Conversation
68e1d49
to
a43d197
Compare
src/include/any.h
Outdated
*/ | ||
|
||
#ifndef INCLUDE_STATIC_ANY | ||
|
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 think you intended to put #define INCLUDE_STATIC_ANY
here.
a43d197
to
e5cdeb2
Compare
Oops. Well, thank you. |
src/librbd/Journal.cc
Outdated
m_work_queue = new ContextWQ("librbd::journal::work_queue", | ||
cct->_conf->get_val<int64_t>("rbd_op_thread_timeout"), | ||
thread_pool_singleton); | ||
cct->_conf->get_val<int64_t>("rbd_op_thread_timeout"), |
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.
please drop these formatting only changes. otherwise we will have them whenever developer touches a file with different editor settings.
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'll pull these out and drop this in a moment.
src/common/ceph_context.h
Outdated
}; | ||
|
||
// I would use std::type_index, but the version of the library on | ||
// gitbuilder doesn't support it for whatever reason. |
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.
weird ... we should have std::type_index
. could you point me to the build failure? also, we are not using gitbuilder anymore. we are using jenkins for building/testing the PRs.
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.
It was the first push I did for this branch. I'm not sure if they're kept. I could stick type-index back in and repush to show you.
std:any is nice and all, but they don't support uncopyable objects. This is annoying to us, since we have a perfect use case for std::any in the associated object map, but almost everything we want to put into it is uncopyable due to having a mutex or something. Enter ceph::immobile_any<>. A zero overhead type erasure that cannot be moved or copied (you stick it in a container and it stays there) of fixed size that does no allocation. Also unique_any and shared_any, because occasionally move and copy semantics are useful but you still want to be able to store an arbitrary type. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
This cleans up the interface, makes things more robust by using both name and type, and does fewer allocations. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
e5cdeb2
to
143c59c
Compare
@tchaikov All right! I have made all the changes. |
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.
+1
std::type_index type = typeid(T); | ||
|
||
auto i = associated_objs.find(std::make_pair(name, type)); | ||
if (i == associated_objs.cend()) { |
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.
Ok, trying to add a duplicate is /not/ a failure.
Remove the Singleton class and dynamic allocation with ceph::immobile_any. This is similar to std::any except that it allows immovable/uncompyable objects and does no allocations.
Also clean up the interface a bit.
Also actually /get rid/ of the various singletons we're holding on shutdown rather than just letting them dangle.