-
Notifications
You must be signed in to change notification settings - Fork 28
MOD-15578 Track shared SVS thread pool memory & expose it through public API #972
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
Changes from 11 commits
3c2023a
b8ffc81
20d0e09
5f329df
ea2296e
b40e042
5f7ec7a
4d5456f
9d082f0
fda6a43
7c8b871
bd88757
ddc00ec
31a8026
b17a7b8
a923fa7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,11 @@ const char *VecSimCommonStrings::TIERED_SVS_UPDATE_THRESHOLD_STRING = "TIERED_SV | |
| const char *VecSimCommonStrings::TIERED_SVS_THREADS_RESERVE_TIMEOUT_STRING = | ||
| "TIERED_SVS_THREADS_RESERVE_TIMEOUT"; | ||
|
|
||
| const char *VecSimCommonStrings::SHARED_SVS_THREADPOOL_MEMORY_STRING = | ||
| "SHARED_SVS_THREADPOOL_MEMORY"; | ||
|
|
||
| const char *VecSimCommonStrings::SHARED_MEMORY_STRING = "SHARED_MEMORY"; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need both
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — there's no real difference today, so I collapsed them into the single generic The original two-field design came from the ticket spec (umbrella If a second global contributor is ever added, an itemized breakdown can be reintroduced then with real differentiation. |
||
|
|
||
| // Log levels | ||
| const char *VecSimCommonStrings::LOG_DEBUG_STRING = "debug"; | ||
| const char *VecSimCommonStrings::LOG_VERBOSE_STRING = "verbose"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3362,6 +3362,88 @@ TEST(SVSTest, NumThreadsParamIgnored) { | |
| VecSimIndexInterface::logCallback = nullptr; | ||
| } | ||
|
|
||
| // SVS debug info exposes both: | ||
| // * SHARED_MEMORY — top-level field appended by VecSimIndex_DebugInfoIterator | ||
| // (mirrors VecSim_GetSharedMemory()); present on every algorithm. | ||
| // * SHARED_SVS_THREADPOOL_MEMORY — emitted by SVSIndex::debugInfoIterator(). | ||
| // They are sourced from the same VecSimSVSThreadPool::getSharedAllocationSize() | ||
| // (the only contributor to shared memory today), so their values must match. | ||
| TYPED_TEST(SVSTest, debugInfoSharedMemoryEqualsSharedSVSThreadPoolMemory) { | ||
| // Ensure the shared SVS thread pool singleton has allocated memory so both | ||
| // fields report a non-zero value. resize() always lazy-initializes the singleton. | ||
| VecSim_UpdateThreadPoolSize(2); | ||
| ASSERT_GT(VecSim_GetSharedMemory(), 0u); | ||
|
|
||
| size_t dim = 4; | ||
| SVSParams params = {.type = TypeParam::get_index_type(), .dim = dim, .metric = VecSimMetric_L2}; | ||
| VecSimIndex *index = this->CreateNewIndex(params); | ||
| ASSERT_INDEX(index); | ||
|
|
||
| VecSimDebugInfoIterator *infoIterator = VecSimIndex_DebugInfoIterator(index); | ||
|
|
||
| bool seen_shared_memory = false; | ||
| bool seen_shared_svs = false; | ||
| uint64_t global_value = 0; | ||
|
cursor[bot] marked this conversation as resolved.
Outdated
|
||
| uint64_t shared_memory_value = 0; | ||
| uint64_t shared_svs_value = 0; | ||
| while (VecSimDebugInfoIterator_HasNextField(infoIterator)) { | ||
| VecSim_InfoField *f = VecSimDebugInfoIterator_NextField(infoIterator); | ||
| if (!strcmp(f->fieldName, VecSimCommonStrings::SHARED_MEMORY_STRING)) { | ||
| ASSERT_FALSE(seen_shared_memory) << "SHARED_MEMORY appears more than once"; | ||
| ASSERT_EQ(f->fieldType, INFOFIELD_UINT64); | ||
| shared_memory_value = f->fieldValue.uintegerValue; | ||
| seen_shared_memory = true; | ||
| } else if (!strcmp(f->fieldName, | ||
| VecSimCommonStrings::SHARED_SVS_THREADPOOL_MEMORY_STRING)) { | ||
| ASSERT_FALSE(seen_shared_svs) << "SHARED_SVS_THREADPOOL_MEMORY appears more than once"; | ||
| ASSERT_EQ(f->fieldType, INFOFIELD_UINT64); | ||
| shared_svs_value = f->fieldValue.uintegerValue; | ||
| seen_shared_svs = true; | ||
| } | ||
| } | ||
| EXPECT_TRUE(seen_shared_memory) << "SHARED_MEMORY field missing from SVS debug info"; | ||
| EXPECT_TRUE(seen_shared_svs) | ||
| << "SHARED_SVS_THREADPOOL_MEMORY field missing from SVS debug info"; | ||
| EXPECT_EQ(shared_memory_value, shared_svs_value) | ||
| << "SHARED_MEMORY and SHARED_SVS_THREADPOOL_MEMORY should report the same bytes " | ||
| "(only the SVS thread pool contributes to VecSim shared memory today)"; | ||
| EXPECT_EQ(shared_memory_value, VecSim_GetSharedMemory()); | ||
|
|
||
| VecSimDebugInfoIterator_Free(infoIterator); | ||
| VecSimIndex_Free(index); | ||
|
|
||
| // Reset the shared singleton pool to size 1 so the next test is not affected. | ||
| // Use VecSimSVSThreadPool::resize(1) directly (matching other thread-pool tests) | ||
| // to avoid the write-mode side effect that VecSim_UpdateThreadPoolSize(0) carries. | ||
| VecSimSVSThreadPool::resize(1); | ||
| } | ||
|
|
||
|
Comment on lines
+3402
to
+3404
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a validation that the tracked memory actually increased/decreased as expected (rather than just that both APIs return the same value) ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a new test |
||
| // VecSim shared memory must actually track the SVS thread-pool allocation: | ||
| // it grows when the pool grows and shrinks when the pool shrinks. Without this, | ||
| // SHARED_MEMORY could be a constant and debugInfoSharedMemoryEqualsSharedSVSThreadPoolMemory | ||
| // would still pass (all three readouts share the same getSharedAllocationSize() source). | ||
| TYPED_TEST(SVSTest, sharedMemoryTracksThreadPoolResize) { | ||
| // Use the C API to ensure it is covered. VecSim_UpdateThreadPoolSize(0) | ||
| // sets WriteInPlace and pool size 1. | ||
| VecSim_UpdateThreadPoolSize(0); | ||
| size_t mem_baseline = VecSim_GetSharedMemory(); | ||
|
|
||
| // Grow the pool (sets WriteAsync). | ||
| VecSim_UpdateThreadPoolSize(8); | ||
| ASSERT_EQ(VecSimSVSThreadPool::poolSize(), 8u); | ||
| size_t mem_8 = VecSim_GetSharedMemory(); | ||
| EXPECT_GT(mem_8, mem_baseline) << "shared memory must grow when the pool grows"; | ||
|
|
||
| // Shrink back to size 1 (still WriteAsync). | ||
| VecSim_UpdateThreadPoolSize(1); | ||
| ASSERT_EQ(VecSimSVSThreadPool::poolSize(), 1u); | ||
| size_t mem_after = VecSim_GetSharedMemory(); | ||
| EXPECT_LT(mem_after, mem_8) << "shared memory must shrink when the pool shrinks"; | ||
|
|
||
| // Restore to default baseline. | ||
| VecSim_UpdateThreadPoolSize(0); | ||
| } | ||
|
|
||
| #else // HAVE_SVS | ||
|
|
||
| TEST(SVSTest, svs_not_supported) { | ||
|
|
||
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.
Only one pair was added no? How come it increased by 3?
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's actually +1 for the new field and +2 because the old 23 was already wrong.
Before the change the real count was 25 (1 ALGORITHM + 9 from addCommonInfoToIterator + 15 SVS-specific), but the comment just said "update this when needed" and it looks like nobody had.
So adding the shared-memory field bumped the correct count to 26, not 24.
I added a little breakdown in the comment so it doesn't drift again. Worth noting it's only a reserve() hint, not a hard size, so the stale value wasn't actually causing any bug — just an extra realloc — which is probably why it slipped by.