Repository navigation
perf(cpp): cache deserialized device index nodes - #1091
Merged
Merged
Conversation
Each device lookup walks the device-level index tree with search_from_internal_node(), which used to read and deserialize the same node from the file every time. A leaf node holds up to max_degree_of_index_node entries, so a scan over N devices in one leaf deserialized that node N times -- together with the heap allocations for every device id in it -- and this dominated the profile's malloc/free. Add a small LRU (capacity 16) keyed by the node's file offset that keeps the deserialized node and the arena backing it. Cached nodes are immutable: callers only binary-search them and clone entries into their own arena, so they must not destroy them (the previous code called destroy() on the temporary node; that is no longer done). 3.2M-row scan over 1000 devices: 0.180s -> 0.113s (~37%); malloc/free samples in the profile drop from ~2800 to ~280.
The member was default-constructed, so it used common::Cache's default bounds (64 entries, elasticity 10) instead of INDEX_NODE_CACHE_CAPACITY. Caught by the new test that asserts the cache honours its capacity. test(cpp): cover the device index node cache Repeated lookups across 40 devices (small index degree, several levels) must return the same indexes and keep the cache within capacity, and reset() must drop every cached node.
…rena TsFileIOReader::reset() destroyed tsfile_meta_page_arena_ while tsfile_meta_ still held the MetaIndexNode graph allocated inside it; the shared_ptr deleters then ran the nodes' destructors (and their children vectors) on freed memory at reader destruction. Clear the map first, while the arena is alive. Found by IndexNodeCacheTest.ResetClearsCache.
reset() frees tsfile_meta_page_arena_, and TsFileMeta::bloom_filter_ is placement-new'ed inside that arena. The reader's own destructor runs ~TsFileMeta later, which dereferenced the freed BloomFilter and called destroy() on it: a use-after-free that MSVC debug-heap and ASan report as an access violation (IndexNodeCacheTest.ResetClearsCache failed on Windows) while release builds on Linux/macOS silently read stale memory. Destroy the bloom filter and clear the pointer while the arena is still alive, alongside the existing index-node-map cleanup.
get_cached_index_node() handed out only the node, while the arena holding the node, its children and their names stayed owned by the cache entry. An LRU eviction or replacement of that entry (possible from another query on the same reader, which the header says is supported) released the arena while the caller was still walking the node: use-after-free on the node and on the node destructor when the caller's shared_ptr was released. Return the whole CachedIndexNode and hold it for the duration of the search, mirroring get_cached_device_node(). Also validate the [start_offset, end_offset) pair before allocating, as the device-node path already does, so a corrupt index cannot turn into a ~4 GiB or truncated allocation. Tests: assert the second lookup round performs no new node reads (the fixture now has few enough nodes to fit the 16-entry cache), so a broken cache can no longer pass as long as its size is non-zero.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1091 +/- ##
===========================================
+ Coverage 66.93% 66.94% +0.01%
===========================================
Files 788 788
Lines 57536 57573 +37
Branches 9297 9304 +7
===========================================
+ Hits 38509 38543 +34
+ Misses 17358 17355 -3
- Partials 1669 1675 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
What changed in this PR
Adds an LRU cache for deserialized index-tree nodes in TsFileIOReader to avoid repeated file reads/deserialization during device lookups, significantly reducing allocator churn and improving scan performance.
Changes:
- Introduces an index-node LRU cache keyed by node file offset, storing both
MetaIndexNodeand its backingPageArena. - Updates
search_from_internal_node()to consult the cache and removes the previous per-lookup node destruction. - Adds targeted tests to verify cache reuse across lookups and clearing on
reset().
| File | Description |
|---|---|
| cpp/test/file/index_node_cache_test.cc | Adds tests validating cache hit behavior across repeated lookups and cache clearing on reader reset. |
| cpp/src/file/tsfile_io_reader.h | Declares cached node container, cache members, cache access API, and test hooks. |
| cpp/src/file/tsfile_io_reader.cc | Implements cached node loading/insertion and integrates caching into internal-node traversal; clears cache on reset. |
Review effort: Lite
CTest runs cases in parallel processes; two can seed the name generator in the same clock tick, and one then removes the file the other is opening (E_ALREADY_EXIST from TsFileWriter::open, observed on the MSVC Release job). Append the pid and a per-process counter, as the writer tests already do.
Cache::insert() replaces an existing entry with a move assignment. For CachedIndexNode/CachedDeviceNode the implicit member-wise form assigns the arena first, so when the cache was the sole owner the arena's pages were freed before the old node's deleter ran on them: a use-after-free when two queries missed the same offset and the second inserted after the first. Re-check the cache under the lock before inserting (as the device-node path already does), and give both cached types assignment operators that release the node before the arena.
Several fixtures build their .tsfile name from a clock count alone. CTest runs cases in parallel processes, so two can read the same tick and then one removes the file the other is opening or reopening (E_ALREADY_EXIST / E_FILE_OPEN_ERR, seen as SEGFAULT on macOS). Append the process id and a per-process counter, the pattern the writer tests already use. Covers table_read_failure, table_field_filter, chunk_reader_resource, aligned_chunk_reader_bulk, aligned_field_filter_regression, masked_bulk_copy and timeseries_schema_codec.
… perf/index-node-cache
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
std::atomicis used here but this diff doesn't add#include <atomic>in this file. This will… · Newdata_bufis only needed as a temporary read buffer for deserialization, but it's allocated from… · New
1 resolved since last review
Review effort: Lite
Copilot stopped reviewing on behalf of
ColinLeeo due to an error
November 13, 2026 11:01
getpid()/_getpid() are used in the fixtures unconditionally, but the process.h/unistd.h block landed inside the ENABLE_THREADS guard in aligned_chunk_reader_bulk_test and chunk_reader_resource_test, which would break a build with threads disabled.
… perf/index-node-cache
The buffer used to deserialize a cached node was allocated from the node's arena, so every cache entry pinned read_size bytes until eviction; use a temporary array instead. Also make the cache test file name unique per process.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every device lookup walks the device-level index tree with
search_from_internal_node(), which read and deserialized the same index node from the file each time — using a scratch arena that it destroyed right after. A leaf node holds up tomax_degree_of_index_nodeentries, so scanning N devices that live in one leaf deserialized that node N times, together with the heap allocations for every device id it contains. This dominated the profile's malloc/free samples (~2800 of ~11000 active samples).Change
MetaIndexNodetogether with thePageArenathat backs it.search_from_internal_node()consults the cache and only reads/deserializes on a miss; entries are cloned into the caller's arena as before.destroy()on the temporary node after use; that is removed, since it would invalidate the cached children vector.reset(), and access is mutex-guarded so parallel readers stay safe.Benchmark
3.2M rows / 1000 devices, full scan, single thread (alternating dylib A/B):
Profile: malloc/free samples drop from ~2800 to ~280. Python test suite wall time also drops from 17.7 s to 9.6 s on the same machine.
Testing
Full C++ suite: 1454 tests passed (includes the fault-injection reader tests and the reader memory-accounting tests).