chore: several improvements around sorted map #1699
Conversation
romange
commented
Aug 15, 2023
romange
commented
- Pass memory_resource to sorted_map.
- Get rid of GetDict leaky accessor in SortedMap and introduce a proper Scan method.
- Introduce a proper BPTree type inside SortedMap::DFImpl.
- Added a test for bptree_test that covers sds comparison (apparently, sdscmp can return values outside of [-1, 1] range). Fixed bptree code to support a proper spec for three-way comparison.
|
|
||
| } // namespace | ||
|
|
||
| class BPTreeSetTest : public ::testing::Test { |
There was a problem hiding this comment.
this class existed before.
| } | ||
|
|
||
| struct ZsetPolicy { |
There was a problem hiding this comment.
i just moved this policy up to be able to use it in tests
| uint64_t ubound = stack.back().second; | ||
| stack.pop_back(); | ||
|
|
||
| if (!Validate(node, ubound)) | ||
| if (!ValidateNode<Node, BPTreePolicy<uint64_t>>(node, ubound)) |
There was a problem hiding this comment.
made ValidateNode generic - to be able to use it for differrent types of trees
| Clear(); | ||
| } | ||
|
|
||
| pair<void*, bool> ScoreMap::AddOrUpdate(string_view field, double value) { |
There was a problem hiding this comment.
expose internal objects managed by ScoreMap so we could
insert them into BPTree
| return dictScan(this->dict, cursor, scanCb, NULL, &cb); | ||
| } | ||
|
|
||
| int SortedMap::DfImpl::ScoreSdsPolicy::KeyCompareTo::operator()(ScoreSds a, ScoreSds b) const { |
There was a problem hiding this comment.
implement comparison for ScoreMap internal objects.
1. Pass memory_resource to sorted_map. 2. Get rid of GetDict leaky accessor in SortedMap and introduce a proper Scan method. 3. Introduce correct BPTree type inside SortedMap::DFImpl. 4. Added a test for bptree_test that covers sds comparison (apparently, sdscmp can return values outside of [-1, 1] range). Fixed bptree code to support a proper spec for three-way comparison. 5. Expose pointers to internal objects allocated by score_map so we could insert them into bptree. Signed-off-by: Roman Gershman <roman@dragonflydb.io>
src/core/sorted_map.h
Outdated
| dict* GetDict() const { | ||
| return std::get<RdImpl>(impl_).dict; | ||
| uint64_t Scan(uint64_t cursor, absl::FunctionRef<void(std::string_view, double)> cb) const { | ||
| return std::visit(Overload{[&](const auto& impl) { return impl.Scan(cursor, cb); }}, impl_); |
There was a problem hiding this comment.
Stop using the overload 🤣
There was a problem hiding this comment.
@dranikpg Why? it's a nice utility ? :D
There was a problem hiding this comment.
Because it works without in all those cases, its an auto lambda and visit([](auto& impl) { ... }) does the job. I pointed it out already in the last PR, but the code was already written 🙂
src/core/sorted_map.cc
Outdated
| auto scanCb = [](void* privdata, const dictEntry* de) { | ||
| auto* cb = (absl::FunctionRef<void(std::string_view, double)>*)privdata; | ||
|
|
||
| sds key = (sds)de->key; | ||
| double score = *(double*)dictGetVal(de); | ||
| (*cb)(std::string_view(key, sdslen(key)), score); | ||
| }; |
There was a problem hiding this comment.
Got it, it decays to a function pointer below only if it doesn't capture anything, nit reinterpret_cast is by the styleguide
src/core/score_map.h
Outdated
| // Returns true if field was added | ||
| // otherwise updates its value and returns false. | ||
| bool AddOrUpdate(std::string_view field, double value); | ||
| std::pair<void*, bool> AddOrUpdate(std::string_view field, double value); |
There was a problem hiding this comment.
- You forgot to update the comment
- Its strange you now have a nice external interface for both sorted set types, but return void* for the element
There was a problem hiding this comment.
As usual, you are welcomed to fix it up later :)
| SortedMap::SortedMap(PMR_NS::memory_resource* mr) : impl_(RdImpl()), mr_res_(mr) { | ||
| std::visit(Overload{[](RdImpl& impl) { impl.Init(); }, [mr](DfImpl& impl) { impl.Init(mr); }}, | ||
| impl_); |
There was a problem hiding this comment.
nit: can also be shortened at least twice 🙂
There was a problem hiding this comment.
Note, it has differrent implementations here.
btw, please remember that the end goal is to get rid of RdImpl so all this boilerplate is temporary.
Similarly, I am planning to get rid of the old set implementation some time this year.
src/core/sorted_map.cc
Outdated
| double GetObjScore(const void* obj) { | ||
| DoubleIntUnion u; | ||
| sds s = (sds)obj; | ||
| char* ptr = s + sdslen(s) + 1; | ||
| u.u64 = absl::little_endian::Load64(ptr); | ||
|
|
||
| return u.score; | ||
| } | ||
|
|
There was a problem hiding this comment.
return absl::bit_cast<double>(absl::little_endian::Load64(ptr)) 😅
Signed-off-by: Roman Gershman <roman@dragonflydb.io>
