-
Notifications
You must be signed in to change notification settings - Fork 22
Fix incorrect assertion in BM_VecSimBasics::UpdateAtBlockSize benchmark
#819
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
Conversation
returns metadata containers capacity
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #819 +/- ##
=======================================
Coverage 96.66% 96.66%
=======================================
Files 126 126
Lines 7368 7379 +11
=======================================
+ Hits 7122 7133 +11
Misses 246 246 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BM_VecSimBasics::UpdateAtBlockSize benchmark
| return this->backendIndex->indexMetaDataCapacity() + | ||
| this->frontendIndex->indexMetaDataCapacity(); |
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.
No need to lock 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.
i took reference from indexCapacity()
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.
from what i see indexCapacity() is also only used for testing
| size_t indexMetaDataCapacity() const override { | ||
| std::shared_lock<std::shared_mutex> flat_lock(this->flatIndexGuard); | ||
| std::shared_lock<std::shared_mutex> main_lock(this->mainIndexGuard); | ||
| return this->frontendIndex->indexMetaDataCapacity() + | ||
| this->backendIndex->indexMetaDataCapacity(); | ||
| } |
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.
Can this be moved to tiered_index.h?
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.
same
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 0.6
git worktree add -d .worktree/backport-819-to-0.6 origin/0.6
cd .worktree/backport-819-to-0.6
git switch --create backport-819-to-0.6
git cherry-pick -x f24b65afa98f99352f7c1f384e24f2c2a4cbaad1 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 0.7
git worktree add -d .worktree/backport-819-to-0.7 origin/0.7
cd .worktree/backport-819-to-0.7
git switch --create backport-819-to-0.7
git cherry-pick -x f24b65afa98f99352f7c1f384e24f2c2a4cbaad1 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 0.8
git worktree add -d .worktree/backport-819-to-0.8 origin/0.8
cd .worktree/backport-819-to-0.8
git switch --create backport-819-to-0.8
git cherry-pick -x f24b65afa98f99352f7c1f384e24f2c2a4cbaad1 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 8.2
git worktree add -d .worktree/backport-819-to-8.2 origin/8.2
cd .worktree/backport-819-to-8.2
git switch --create backport-819-to-8.2
git cherry-pick -x f24b65afa98f99352f7c1f384e24f2c2a4cbaad1 |
… benchmark (#824) * Fix incorrect assertion in `BM_VecSimBasics::UpdateAtBlockSize` benchmark (#819) * VecSimIndexInterface: introduce indexMetaDataCapacity for testing returns metadata containers capacity * cover for tiered index (cherry picked from commit f24b65a) * Uncomment include for bm_basics_initialize_fp32.h
The
UpdateAtBlockSizebenchmark contained an incorrect assertion that expected metadata containers to shrink by exactly one block size during delete operations:Why it was broken
idToLabelMapping) only shrink whenindexCapacity() >= (indexSize() + 2 * blockSize), not at exact blocksize boundariesindexCapacity()returns the capacity of vector containers, not metadata containers. While the assertion didn't fail, it was testing vector container capacity instead of the intended metadata container capacity, making it ineffective at validating the benchmark's actual purposeWhat was fixed
indexMetaDataCapacity()method: Returns the actual capacity of metadata containers (idToLabelMappingfor Flat,idToMetaDatafor HNSW, combined capacities for tiered indexes)