-
Notifications
You must be signed in to change notification settings - Fork 990
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
chore: ClearInternal now can clear partially #3867
Conversation
Intended for future use - to deallocate large objects gradually. Currently nothing is changed in the functionality besides some cleanups. Signed-off-by: Roman Gershman <[email protected]>
src/core/dense_set.cc
Outdated
constexpr unsigned kArrLen = 32; | ||
ClearItem arr[kArrLen]; | ||
unsigned len = 0; | ||
|
||
for (auto it = entries_.begin(); it != entries_.end(); ++it) { | ||
if (it->IsEmpty()) | ||
size_t end = min<size_t>(entries_.size(), start + limit); |
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.
you mean limit - start
here right ? Cause otherwise:
entries_.size()=10, start=3, limit=8
then end=10
and you will iterate from start=3
up to end=10
when you wanted to start from 3
and end up to limit=8
.
Is there something I am missing here?
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.
unless you mean limit
as count
that is, how many elements from start
you want to delete. (cause in my mind limit is more of like an index limit rather than the total elements you want to delete starting at pos start
)
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.
I meant count, not limit
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.
uh gotcha
Unit failure
We have an issue as well #3743 |
* chore: ClearInternal now can clear partially Intended for future use - to deallocate large objects gradually. Currently nothing is changed in the functionality besides some cleanups. --------- Signed-off-by: Roman Gershman <[email protected]>
Intended for future use - to deallocate large objects gradually. Currently nothing is changed in the functionality besides some cleanups.