You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@tsfile.apache.org by "Rayan-and-beyond (via GitHub)" <gi...@apache.org> on 2026/09/17 20:56:14 UTC
[PR] fix(cpp): complete and adopt bounded lru cache [tsfile]
Rayan-and-beyond opened a new pull request, #967:
URL: https://github.com/apache/tsfile/pull/967
closes #943
this finishes the existing c++ lru cache contract and puts it on the reader device-node path.
- fixes the broken lookup APIs and adds explicit non-copying pointer/reference access
- documents caller-side synchronization, reference lifetime, elasticity, and unbounded mode
- removes the stale metadataquerier cache wiring that used std::mutex as the map type
- replaces the reader's unbounded shared-arena device cache with a 64-entry lru where each entry owns its arena, so eviction can actually reclaim metadata pages
- adds focused lru behavior coverage plus a reader regression that fills past capacity, reloads an evicted device, and checks reader metadata memory stays flat
validation:
- 6/6 focused lrucache gtests pass
- device-node bounded/reload regression passes
- changed production objects compile and libtsfile links in the focused c++ build with optional codecs/antlr disabled
- git diff --check clean
the vm cannot finish linking the monolithic tsfile_test binary within its memory envelope, so i linked and ran the two touched test surfaces directly from the same built objects/libs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): complete and adopt bounded lru cache [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo commented on code in PR #967:
URL: https://github.com/apache/tsfile/pull/967#discussion_r4241507724
##########
cpp/test/reader/tsfile_reader_test.cc:
##########
@@ -495,6 +495,62 @@ TEST_F(TsFileReaderTest, ReadsThroughRandomAccessReadFile) {
reader.close();
}
+TEST_F(TsFileReaderTest, DeviceNodeCacheIsBoundedAndReloadsEvictedDevices) {
+ const size_t capacity = TsFileIOReader::TEST_device_node_cache_capacity();
+ const size_t device_count = capacity + 8;
+ const std::string measurement = "value";
+
+ for (size_t i = 0; i < device_count; ++i) {
+ const std::string device = "root.cache.d" + std::to_string(i);
+ ASSERT_EQ(tsfile_writer_->register_timeseries(
+ device, MeasurementSchema(measurement, INT32, PLAIN,
+ UNCOMPRESSED)),
+ E_OK);
+ TsRecord record(static_cast<int64_t>(i), device);
+ record.add_point(measurement, static_cast<int32_t>(i));
+ ASSERT_EQ(tsfile_writer_->write_record(record), E_OK);
+ }
+ ASSERT_EQ(tsfile_writer_->flush(), E_OK);
+ ASSERT_EQ(tsfile_writer_->close(), E_OK);
+
+ TsFileIOReader io_reader;
+ ASSERT_EQ(io_reader.init(file_name_), E_OK);
+ PageArena pa;
+ pa.init(512, MOD_TSFILE_READER);
+
+ auto load_device = [&](size_t i) {
+ const std::string device = "root.cache.d" + std::to_string(i);
+ auto device_id = std::make_shared<StringArrayDeviceID>(device);
+ TsFileSeriesScanIterator* ssi = nullptr;
+ ASSERT_EQ(io_reader.alloc_ssi(device_id, measurement, ssi, pa), E_OK);
+ ASSERT_NE(ssi, nullptr);
+ io_reader.revert_ssi(ssi);
+ pa.reset();
+ EXPECT_LE(io_reader.TEST_device_node_cache_size(), capacity);
+ };
+
+ for (size_t i = 0; i < capacity; ++i) {
+ load_device(i);
+ }
+ 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);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): complete and adopt bounded lru cache [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo commented on PR #967:
URL: https://github.com/apache/tsfile/pull/967#issuecomment-6109171779
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 `load_timeseries_index_for_ssi()` still calls `load_measurement_index_entry()` and then reads/parses the selected timeseries metadata. The measurement lookup reaches `MetaIndexNode::binary_search_children()`, whose `clone(pa_)` copies the measurement name into the cached node's arena.
This also leaves a hot-entry memory-growth case: after warming one device, I repeated `alloc_ssi()` / `revert_ssi()` 10,000 times, resetting the query arena after each iteration. The cache remained at one entry, while reader memory accounting grew from 1,072 to 84,688 bytes. This allocation behavior predates this PR. Bounding entry count does not prevent an individual retained arena from growing with repeated lookups. The lookup-result copy should use a query-owned arena, or another ownership-safe result representation.
For a subsequent optimization, could we consider a bounded, reader-owned `(device, measurement)` cache of parsed metadata with an owning handle, building on `PreparedSeries` / `init_prepared()`? A hit could bypass measurement-index traversal and repeated metadata I/O/deserialization, while each query keeps its own filter, scan cursor, and decoding buffers. Entries should be loaded lazily, have a memory budget, and share aligned time metadata where appropriate. The miss path would still need the allocation-lifetime fix above.
A repeated-query benchmark tracking metadata lookup/read/parse counts and retained memory would make both the performance benefit and the memory behavior measurable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): complete and adopt bounded lru cache [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo merged PR #967:
URL: https://github.com/apache/tsfile/pull/967
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): complete and adopt bounded lru cache [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo commented on code in PR #967:
URL: https://github.com/apache/tsfile/pull/967#discussion_r4241289107
##########
cpp/src/common/cache/lru_cache.h:
##########
@@ -79,39 +87,56 @@ class Cache {
cache_[k] = entries_.begin();
prune();
}
- /**
- for backward compatibility. redirects to tryGetCopy()
- */
- bool tryGet(const Key& kIn, Value& vOut) { return tryGetCopy(kIn, vOut); }
- bool tryGetCopy(const Key& kIn, Value& vOut) {
- Value tmp;
- if (!tryGetRef_nolock(kIn, tmp)) {
+ /** Backward-compatible copying lookup. */
+ bool tryGet(const Key& k, Value& vOut) { return tryGetCopy(k, vOut); }
+
+ bool tryGetCopy(const Key& k, Value& vOut) {
+ const Value* value = getPtr(k);
+ if (value == nullptr) {
return false;
}
- vOut = tmp;
+ vOut = *value;
return true;
}
- bool tryGetRef(const Key& kIn, Value& vOut) {
- return tryGetRef_nolock(kIn, vOut);
- }
/**
- * The const reference returned here is only
- * guaranteed to be valid till the next insert/delete
- * in multi-threaded apps use getCopy() to be threadsafe
+ * Non-copying lookup. On success vOut points at the cached value and the
+ * entry is promoted to most-recently-used.
*/
- const Value& getRef(const Key& k) { return get_nolock(k); }
+ bool tryGetRef(const Key& k, const Value*& vOut) {
+ vOut = getPtr(k);
+ return vOut != nullptr;
+ }
/**
- added for backward compatibility
+ * 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); }
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org