-
Notifications
You must be signed in to change notification settings - Fork 2
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
[FEATURE] emplace_exists #212
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #212 +/- ##
=======================================
Coverage 99.67% 99.67%
=======================================
Files 47 47
Lines 1845 1852 +7
Branches 5 5
=======================================
+ Hits 1839 1846 +7
Misses 6 6 ☔ View full report in Codecov by Sentry. |
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.
LGTM, just check if you really wanted to remove this one comment.
// Constructing the reference twice for emplace_exists would impact performance. | ||
// No difference for emplace. | ||
seqan::hibf::bit_vector::reference bit_reference{(*this)[idx]}; | ||
if constexpr (check_exists) | ||
exists &= bit_reference; | ||
bit_reference = 1; |
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.
Do you mean that
if constexpr (check_exists)
exists &= (*this)[idx];
(*this)[idx] = 1;
Would construct the reference twice and is therefore slower?
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.
Yes, it would need to compute the bit position twice. Since there isn't much happening in emplace
, doing it twice really affects performance.
emplace 50M/s
emplace_exists 47M/s
emplace_exists* 28M/s
*
is with two references
Needed for dynamic HIBF.
emplace_exists
is around 5% slower thanemplace
.