-
Notifications
You must be signed in to change notification settings - Fork 660
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
Replace deleted elements at addition #418
Conversation
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.
It looks like the code is not thread safe.
I guess we should write parallel stress tests to find those bugs.
5503715
to
7023e13
Compare
7023e13
to
22cdb46
Compare
7db5671
to
ef7e383
Compare
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.
Thank you!
I think the main thing to consider is the interface - using flag instead of separate method.
static const unsigned char DELETE_MARK = 0x01; | ||
|
||
size_t max_elements_{0}; | ||
size_t cur_element_count{0}; | ||
mutable std::atomic<size_t> cur_element_count{0}; // current number of elements |
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.
Why is it atomic, shouldn't it be locked all the times by external locks?
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 think the parameter in the index should be renamed as there is a name collision. Maybe also run the performance regression tests. Otherwise looks great to me! Thank you!
Added a new method to replace a deleted element when a new one is added:
labeltype addPointToVacantPlace(const void* data_point, labeltype label)
The method is the same as
addPoint
but allows to replace a deleted element with a new one to save memory. Note: addPointToVacantPlace is much slower than addPoint, because replacement is a heavy operation.Edit: