Repository navigation
fix(cpp): complete and adopt bounded lru cache - #967
Rayan-and-beyond wants to merge 4 commits into
Conversation
| * Legacy overload retained for source compatibility. Despite its historic | ||
| * name it copies; new code should use the pointer overload or getPtr(). | ||
| */ | ||
| bool tryGetRef(const Key& k, Value& vOut) { return tryGetCopy(k, vOut); } |
There was a problem hiding this comment.
Is there a concrete downstream compatibility requirement for retaining these aliases? I couldn't find any in-repository callers of tryGet(), get(), or the copying tryGetRef(Key, Value&); the production reader already uses tryGetCopy(). Unless downstream compatibility is required, could we remove the compatibility-only aliases, especially this overload? Having tryGetRef either copy or return a cached pointer depending on the output parameter type makes the API easy to misuse.
| } | ||
| ASSERT_EQ(io_reader.TEST_device_node_cache_size(), capacity); | ||
| const int64_t memory_at_capacity = io_reader.TEST_reader_memory_bytes(); | ||
| ASSERT_GT(memory_at_capacity, 0); |
There was a problem hiding this comment.
Could we guard the memory snapshot and related assertions with #ifdef ENABLE_MEM_STAT, while keeping the cache-capacity and eviction/reload checks enabled? ENABLE_MEM_STAT=OFF is a supported CMake configuration, and ModStat::get_stat() returns 0 in that build, so this unconditional ASSERT_GT(memory_at_capacity, 0) fails. I reproduced this failure in TsFileReaderTest.DeviceNodeCacheIsBoundedAndReloadsEvictedDevices with memory statistics disabled. The later memory-equality assertions also become vacuous (0 == 0) in this configuration.
|
Potential follow-up (can be separate from this PR): cache resolved per-series metadata so repeated queries can reuse more than the device's top index node. A device-node cache hit skips reading/deserializing that node, but This also leaves a hot-entry memory-growth case: after warming one device, I repeated For a subsequent optimization, could we consider a bounded, reader-owned A repeated-query benchmark tracking metadata lookup/read/parse counts and retained memory would make both the performance benefit and the memory behavior measurable. |
closes #943
this finishes the existing c++ lru cache contract and puts it on the reader device-node path.
std::mutexas the map typevalidation:
TsFile_Testtarget builds successfully with-j1on the constrained vmlibtsfilelinks with optional codecs/antlr disabledgit diff --checkclean