bluestore: use btree_map for allocator #7269
Conversation
| #include <iterator> | ||
| #include <map> | ||
| #include <ostream> | ||
| using namespace std; |
There was a problem hiding this comment.
using namespace in header files is not a good practice in general, as it brings the entire namespace into the global namespace. as a user, I wouldn't expect that behavior from #include "btree_interval_set.h". the std namespace has tons of names, so conflicts are likely in a large codebase, and they're hard to track down because they generally show up in templates or function overload resolution
all of the std types appear to be qualified with std:: already, so you can probably just take that line out
liewegas
force-pushed
the
wip-bluestore-btree
branch
from
0becae1
to
792a789
Compare
From https://github.com/algorithm-ninja/cpp-btree. Signed-off-by: Sage Weil <sage@redhat.com>
A few small changes to interval_map to deal with the fact that any btree change invalidates all iterators. Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
btree_map is about twice as memory efficient. Signed-off-by: Sage Weil <sage@redhat.com>
btree_map uses about half the memory. According to the cpp-btree docs they are also faster than STL maps... especially when they get big. Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
liewegas
force-pushed
the
wip-bluestore-btree
branch
from
792a789
to
bb9343e
Compare
|
fixed std usage in interval_set and btree_interval_set |
| if (p != kv_free.end()) | ||
| next = p->first; | ||
| --p; | ||
| kv_free.erase(p); |
There was a problem hiding this comment.
It looks like btree_map::erase returns an iterator for the next item:
See btree_map_container declaration:
...
// Erase the specified iterator from the btree. The iterator must be valid
// (i.e. not equal to end()). Return an iterator pointing to the node after
// the one that was erased (or end() if none exists).
iterator erase(const iterator &iter) {
return this->tree_.erase(iter);
}
...
Thus you probably can have something like
p = kv.free.erase(p);
and remove other stuff to locate the next entry
There was a problem hiding this comment.
The btree_map erase() returns an iterator for the next element. Perfect! Reported-by: Igor Fedotov <ifedotov@mirantis.com> Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
This mirrors the condition above. Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
|
@liewegas, I made some unit tests to verify both interval sets implementations. They pass OK. PR is made to your repo since btree version isn't merged yet. Sorry if that's inappropriate. |
UT for interval_set implementations
bluestore: use btree_map for allocator Reviewed-by: Igor Fedotov <ifedotov@mirantis.com> Reviewed-by: Casey Bodley <cbodley@redhat.com>
https://code.google.com/p/cpp-btree/wiki/UsageInstructions#Memory_usage_comparison
https://code.google.com/p/cpp-btree/wiki/UsageInstructions#Performance_comparison