apache / apache/tsfile

[Improvement][C++] Complete and adopt the existing LRU cache infrastructure

Open
#943 0 comments 0 reactions 0 assignees View on GitHub
c++ enhancement feature help wanted performance
Dominant language
Java
Stars
203
Forks
104
Avg merge
2d 6h
Merged PRs (30d)
33

Description

### Motivation

C++ performance improvements such as #885 and #934 need a reusable bounded cache. Without a production-ready cache abstraction, individual components may introduce unbounded per-object caches or duplicate cache implementations, making memory usage and ownership harder to control.

### Current cache mechanisms

The repository currently contains several cache-related implementations:

- `cpp/src/common/cache/lru_cache.h` provides a generic `std::list` + associative-map LRU skeleton with configurable maximum size and elasticity.
- `TsFileIOReader::device_node_cache_` is an active, reader-specific cache protected by a mutex. It is unbounded and backed by a shared `PageArena`, so individual eviction cannot reclaim the cached allocations.
- The Java implementation has a working `LRUCache` based on `LinkedHashMap`, which can serve as a behavioral reference but cannot be reused directly by C++.

### Problems with the existing C++ LRU

The generic C++ LRU is not currently used as a working production cache:

- `getCopy()` and `getRef()` call an undefined `get_nolock()`, so these APIs fail when instantiated.
- `tryGetRef()` copies the cached value into an output parameter despite its name. This is unsuitable for entries containing vectors or other expensive values.
- The comments mention thread safety, but the implementation contains no synchronization.
- `MetadataQuerier` passes `std::mutex` as the third template argument even though that argument is defined as the associative map type.
- The cache operations in `MetadataQuerier` are commented out, and there are no focused C++ unit tests covering the cache behavior.

### Proposed work

1. Define the cache contract clearly, including ownership, reference lifetime, capacity semantics, and whether synchronization is internal or the caller's responsibility.
2. Fix all public APIs and add a non-copying lookup mechanism, such as a pointer/reference accessor with a documented lifetime or a callback-based API.
3. Add unit tests covering insertion, lookup promotion, update, eviction, removal, clearing, bounded capacity, and disabled/unbounded configurations.
4. Correct or remove the inactive `MetadataQuerier` integration.
5. Adopt the completed cache in at least one real component, such as the bounded writer-level schema lookup cache discussed in #934.
6. Add benchmarks or memory measurements demonstrating that the cache improves the target workload while keeping additional memory bounded.

### Acceptance criteria

- All exposed cache APIs compile and have tested behavior.
- The eviction and capacity contract is deterministic and documented.
- Expensive cached values can be accessed without an unconditional copy.
- Thread-safety expectations and cached-value lifetime are explicit.
- At least one production C++ consumer uses the cache.

### Related

- #885
- #934

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reading cpp/src/common/cache/lru_cache.h and the existing MetadataQuerier integration, then compare the Java LRUCache behavior described in the issue. Review TsFileIOReader::device_node_cache_ and the related cache usage before defining the contract and test scope. Done means the exposed APIs compile, focused C++ tests cover the listed behaviors, and a production consumer uses the bounded cache with documented lifetime and thread-safety expectations.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, java
Domain
backend, performance
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.