-
Notifications
You must be signed in to change notification settings - Fork 27
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
base: main
Are you sure you want to change the base?
Changes from all 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 |
|---|---|---|
|
|
@@ -3362,6 +3362,72 @@ TEST(SVSTest, NumThreadsParamIgnored) { | |
| VecSimIndexInterface::logCallback = nullptr; | ||
| } | ||
|
|
||
| // The SHARED_MEMORY field is a top-level field appended by | ||
| // VecSimIndex_DebugInfoIterator (mirrors VecSim_GetSharedMemory()); it is | ||
| // present on every algorithm. Verify it appears exactly once in an SVS | ||
| // response and reports the same bytes as the public API. | ||
| TYPED_TEST(SVSTest, debugInfoSharedMemoryMatchesApi) { | ||
| // Ensure the shared SVS thread pool singleton has allocated memory so the | ||
| // field reports 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; | ||
| uint64_t shared_memory_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; | ||
| } | ||
| } | ||
| EXPECT_TRUE(seen_shared_memory) << "SHARED_MEMORY field missing from SVS debug info"; | ||
| 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 debugInfoSharedMemoryMatchesApi would | ||
| // still pass (both 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.
Why do we need both
SHARED_MEMORYandSHARED_SVS_THREADPOOL_MEMORY_STRING? is there any difference between them?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.
Good catch — there's no real difference today, so I collapsed them into the single generic
SHARED_MEMORYfield (a923fa7).The original two-field design came from the ticket spec (umbrella
GLOBAL_MEMORY/SHARED_MEMORY+ per-contributorSHARED_SVS_THREADPOOL_MEMORYbreakdown). But the SVS thread pool is the only process-wide contributor today, so both always reported the exact same bytes — the breakdown carried no extra information. YAGNI: droppedSHARED_SVS_THREADPOOL_MEMORYand kept onlySHARED_MEMORY+VecSim_GetSharedMemory().If a second global contributor is ever added, an itemized breakdown can be reintroduced then with real differentiation.