Using croaring library instead of MaskCollection #4356
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ray6080
requested review from
ray6080 and
andyfengHKU
and removed request for
benjaminwinger
ray6080
approved these changes
Oct 14, 2024
src/include/common/mask.h
Outdated
| } | ||
| maskData = std::make_unique<MaskData>(maxOffset + 1); | ||
| } | ||
| explicit RoaringBitMapSemiMask(common::table_id_t tableID, common::offset_t maxOffset) |
There was a problem hiding this comment.
In my understanding, bitmap should be a single word. so can u rename this to RoaringBitmapSemiMask?
src/include/common/mask.h
Outdated
| struct RoaringBitMapSemiMaskUtil { | ||
| static std::shared_ptr<RoaringBitMapSemiMask> createRoaringBitMapSemiMask( | ||
| common::table_id_t tableID, common::offset_t maxOffset) { | ||
| if (maxOffset > std::numeric_limits<u_int32_t>::max()) { |
There was a problem hiding this comment.
Suggested change
| if (maxOffset > std::numeric_limits<u_int32_t>::max()) { | |
| if (maxOffset > std::numeric_limits<uint32_t>::max()) { |
| @@ -36,7 +36,7 @@ class ScanNodeTableSharedState { | |||
| common::node_group_idx_t currentUnCommittedGroupIdx; | |||
| common::node_group_idx_t numCommittedNodeGroups; | |||
| common::node_group_idx_t numUnCommittedNodeGroups; | |||
| std::unique_ptr<common::NodeVectorLevelSemiMask> semiMask; | |||
| std::shared_ptr<common::RoaringBitMapSemiMask> semiMask; | |||
There was a problem hiding this comment.
Wonder if we should switch to shared_ptr here. I don't see clear win or loss of using shared_ptr here except that it avoids passing raw pointers around while making the life cycle of semiMask less obvious. @andyfengHKU should also comment here.
| } | ||
|
|
||
| // include&exclude | ||
| std::vector<common::offset_t> range(uint32_t start, uint32_t end) override { |
There was a problem hiding this comment.
Wonder if there is a shortcut when no value is masked at all.
There was a problem hiding this comment.
It seems that there is no.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Using croaring library instead of MaskCollection, and using new mask in NodeGroup::scan.
This improves performance by roughly 60% QPS in twitter2010,query like 'match (a:vertex)-[f]-(b) where a.ID=3961 return b'.
In SF100,single query like 'match (a:person {id:24189255912498})-[f*2]-(b) return distinct b limit 20' improve 35%
However,more performance testing is needed.
Contributor agreement