chore: introduce sorted_map #1558
Conversation
src/core/zset_test.cc
Outdated
|
|
||
| namespace dfly { | ||
|
|
||
| class ZSetTest : public ::testing::Test { |
There was a problem hiding this comment.
this unit test is not directly related to this Pr but it shows the potential of using a different data structure than skiplist. In this case I compared it to absl::btree.
There was a problem hiding this comment.
Maybe add a comment saying that this is not a real unit-test (in that it doesn't CHECK or EXPECT anything), but rather a way to compare zset and absl::btree_map? Maybe elaborate more on what exactly you're trying to compare (memory usage?)
romange
force-pushed
the
Pr1
branch
3 times, most recently
from
4015ec5 to
38d42c7
Compare
|
|
||
| namespace detail { | ||
|
|
||
| class SortedMap { |
There was a problem hiding this comment.
Please add a comment roughly describing what this class is. "Sorted map, implemented using Redis' dict / ..."
src/core/sorted_map.h
Outdated
| SortedMap(); | ||
| ~SortedMap(); | ||
|
|
||
| static SortedMap* FromListPack(const uint8_t* lp); |
There was a problem hiding this comment.
I'd strongly recommend returning by-value. This class is very small in size, and so that shouldn't be a problem.
Further, you can even make this class movable quite easily, which may be useful.
If you choose to disagree, please document who owns the returned instance. Consider returning unique_ptr if owned by caller.
There was a problem hiding this comment.
The reason I return a pointer is because the context demands so. I have no use for this object on the stack because I must pass its pointer into the CompactObject.
Please note that while it looks like a new class, it's really about consolidation of dispersed code under a single entity.
I will add an ownership comment.
There was a problem hiding this comment.
I'd return a unique_ptr rather than document it, it's self documenting and safer. It also won't have any performance implications (just call .release() from the callsite)
src/core/sorted_map.h
Outdated
| } | ||
|
|
||
| namespace dfly { | ||
|
|
There was a problem hiding this comment.
I'd remove this empty line
src/core/sorted_map.cc
Outdated
| int incr = (in_flags & ZADD_IN_INCR) != 0; | ||
| int nx = (in_flags & ZADD_IN_NX) != 0; | ||
| int xx = (in_flags & ZADD_IN_XX) != 0; | ||
| int gt = (in_flags & ZADD_IN_GT) != 0; | ||
| int lt = (in_flags & ZADD_IN_LT) != 0; |
There was a problem hiding this comment.
Make all of these const bool?
There was a problem hiding this comment.
ok, but again - i just copied the code.
src/core/zset_test.cc
Outdated
| zskiplist* zsl = zslCreate(); | ||
| LOG(INFO) << "zskiplist before: " << zmalloc_used_memory_tl << " bytes"; | ||
|
|
||
| for (int i = 0; i < 10000; ++i) { |
There was a problem hiding this comment.
Consider using 10'000 syntax
src/core/zset_test.cc
Outdated
|
|
||
| namespace dfly { | ||
|
|
||
| class ZSetTest : public ::testing::Test { |
There was a problem hiding this comment.
Maybe add a comment saying that this is not a real unit-test (in that it doesn't CHECK or EXPECT anything), but rather a way to compare zset and absl::btree_map? Maybe elaborate more on what exactly you're trying to compare (memory usage?)
romange
force-pushed
the
Pr1
branch
3 times, most recently
from
48cdc11 to
09741c5
Compare
src/core/sorted_map.h
Outdated
| SortedMap(); | ||
| ~SortedMap(); | ||
|
|
||
| static SortedMap* FromListPack(const uint8_t* lp); |
There was a problem hiding this comment.
I'd return a unique_ptr rather than document it, it's self documenting and safer. It also won't have any performance implications (just call .release() from the callsite)
src/core/sorted_map.h
Outdated
| SortedMap(SortedMap&& other) noexcept = default; | ||
| SortedMap& operator=(SortedMap&& other) noexcept = default; |
There was a problem hiding this comment.
Actually I don't think that the default move operations are a good fit here, as they won't nullify the members of the moved-from object.
If this class, like you implied, is only ever used on the heap then you can delete those as well. If you do think it might be useful to have these movable, then I think you'll have to implement these operations yourself, and make sure that in case the members are nullptr the d'tor works as expected.
romange
force-pushed
the
Pr1
branch
2 times, most recently
from
938dfbd to
8eb67bb
Compare
Consolidate skiplist based zset functionality into a dedicated class called SortedMap. The code is adapted from t_zset.c The old code in t_zset.c is deleted. Signed-off-by: Roman Gershman <[email protected]>
Consolidate skiplist based zset functionality into a dedicated class.