chore(DenseSet): defrag all links in a chain #4019
Conversation
src/core/score_map.cc
Outdated
| bool reallocated = false; | ||
| while (ptr->IsLink()) { | ||
| reallocated |= body(ptr->AsLink()); | ||
| ptr = ptr->Next(); |
There was a problem hiding this comment.
Need to add a test, we don't actually hit this condition (and we do not iterate over the links -- and I want this tested.)
There was a problem hiding this comment.
IMO kinda hard to test TraverseApply because of how it's encapsulated. So looking for alternatives 😄
Signed-off-by: kostas <[email protected]>
src/core/dense_set.cc
Outdated
| fun(ptr->AsLink()); | ||
| ptr = ptr->Next(); |
There was a problem hiding this comment.
| fun(ptr->AsLink()); | |
| ptr = ptr->Next(); | |
| DenseLinkKey* link = ptr->AsLink(); | |
| fun(link); | |
| ptr = &link->next; |
src/core/dense_set.cc
Outdated
| @@ -840,4 +840,20 @@ size_t DenseSet::SizeSlow() { | |||
| return size_; | |||
| } | |||
|
|
|||
| size_t DenseSet::IteratorBase::TraverseApply(DensePtr* ptr, std::function<void(DensePtr*)> fun) { | |||
| if (!ptr->IsLink()) { | |||
| fun(ptr); | |||
There was a problem hiding this comment.
chain link ends with a regular DensePtr, so the way the function is written, it will skip the last node in the chain
There was a problem hiding this comment.
I added LOGS to check TraverseApply with the tests before I pushed and your comment explains why there were links with only one element -- I was missing the last one :/ I fixed this. Also does that mean we have the same issue in Scan ?
// Check home bucket
if (!curr->IsEmpty() && !curr->IsDisplaced()) {
// scanning add all entries in a given chain
while (true) {
cb(curr->GetObject());
if (!curr->IsLink())
break;
DensePtr* mcurr = const_cast<DensePtr*>(curr);
if (ExpireIfNeeded(mcurr, &mcurr->AsLink()->next) && !mcurr->IsLink()) {
break;
}
curr = &curr->AsLink()->next;
}
}
cb not called on the last element because we break when if (!curr->IsLink())
There was a problem hiding this comment.
seems like it! how come our tests did not catch it?
There was a problem hiding this comment.
I do not know! I will fix it 😄
There was a problem hiding this comment.
code is correct 🤣 we apply the cb on the last node and GetObject() takes care of that... 🤦
src/core/dense_set.cc
Outdated
| @@ -840,4 +840,27 @@ size_t DenseSet::SizeSlow() { | |||
| return size_; | |||
| } | |||
|
|
|||
| size_t DenseSet::IteratorBase::TraverseApply(DensePtr* ptr, std::function<void(DensePtr*)> fun) { | |||
| if (!ptr->IsLink()) { | |||
There was a problem hiding this comment.
you can simplify the code if you just delete these 3 lines.
| if (cb) { | ||
| cb((sds)obj, (sds)new_obj); | ||
| bool reallocated = false; | ||
| auto body = [ratio, &cb, &reallocated](auto* ptr) { |
There was a problem hiding this comment.
nit: since there are many pointers of differrent types, it could be nice to use explicit DensePtr here and also below in StringMap
There was a problem hiding this comment.
I wanted to do the same, the problem is DensePtr is private in this context so it can't really be used
We did not properly defrag all links in a chain. This PR adds this.