Skip to content

enhance: batch evictable warmup by loaded size #115

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.

Sign up for GitHub

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

Open
sparknack wants to merge 3 commits into
base: master
Choose a base branch
from
Open

enhance: batch evictable warmup by loaded size #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a542242
enhance: batch evictable warmup by loaded size
sparknack Jul 29, 2026
9611919
fix: correct batched cache warmup semantics
sparknack Jul 30, 2026
a02f129
test: cover warmup shared admission deadline
sparknack Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 50 additions & 7 deletions include/cachinglayer/CacheSlot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,10 @@ class CacheSlot final : public std::enable_shared_from_this<CacheSlot<CellT>> {
return;

case CacheWarmupPolicy::CacheWarmupPolicy_Async: {
auto cids = AllCellIds();

if (prefetch_pool) {
std::weak_ptr<CacheSlot<CellT>> weak_self = this->shared_from_this();
auto token = warmup_cancel_source_.getToken();
prefetch_pool->add([weak_self, cids = std::move(cids), token]() {
prefetch_pool->add([weak_self, token]() {
try {
auto self = weak_self.lock();
if (!self || token.isCancellationRequested()) {
Expand All @@ -130,7 +128,7 @@ class CacheSlot final : public std::enable_shared_from_this<CacheSlot<CellT>> {
OpContext warmup_ctx(token);
// Note: caller ctx is not captured - async warmup intentionally inherits only the
// cancellation token, not trace context, storage usage, or other caller metadata.
self->PinCellsDirect(&warmup_ctx, cids, self->warmup_loading_timeout_);
self->PinWarmupCells(&warmup_ctx);
// If the slot is not evictable, we don't need to pin the cells anymore after warmup.
self->skip_pin_.store(!self->evictable_, std::memory_order_release);
} catch (const std::exception& e) {
Expand All @@ -144,16 +142,15 @@ class CacheSlot final : public std::enable_shared_from_this<CacheSlot<CellT>> {
// Fallback to sync if no pool provided
LOG_WARN("[MCL] Async warmup requested but no prefetch pool provided, falling back to sync");
// TODO: Warmup is not tracked for now
PinCellsDirect(ctx, cids, warmup_loading_timeout_);
PinWarmupCells(ctx);
skip_pin_.store(!evictable_, std::memory_order_release);
return;
}

case CacheWarmupPolicy::CacheWarmupPolicy_Sync: {
auto cids = AllCellIds();
// Sync warmup (original behavior)
// TODO: Warmup is not tracked for now
PinCellsDirect(ctx, cids, warmup_loading_timeout_);
PinWarmupCells(ctx);
skip_pin_.store(!evictable_, std::memory_order_release);
return;
}
Expand Down Expand Up @@ -372,6 +369,51 @@ class CacheSlot final : public std::enable_shared_from_this<CacheSlot<CellT>> {
return cids;
}

[[nodiscard]] std::vector<std::vector<cid_t>>
LoadedSizeBatches(const std::vector<cid_t>& cids, int64_t max_loaded_bytes = kDefaultLoadedSizeBatchBytes) const {

@buqian-zilliz buqian-zilliz Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Size warmup batches to the effective admission limit

The fixed 1 GiB loaded-size cap is independent of DList's configured capacity and the actual reservation (loaded + overhead delta) * loading_resource_factor. A smaller cache can therefore group individually admissible cells into a permanently rejected batch; the failure propagates out of PinCellsDirect() and the remaining batches are never warmed. Derive the batch target from the effective admission limit, or split and retry a batch rejected as oversized.

@sparknack sparknack Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We intentionally keep warmup batches coarse and best-effort. Making the target capacity-aware or recursively splitting rejected batches would fragment loading when resources are already tight; in that situation we prefer rejecting the batch and stopping warmup instead of forcing admission of a few cells. This PR lowers the default cap from 1 GiB to 512 MiB as a more conservative bound, but does not add split-and-retry behavior.

std::vector<std::vector<cid_t>> batches;

@zhengbuqian zhengbuqian Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: make function name a verb, not noun. such as SplitCellsIntoBatches?

std::vector<cid_t> batch;
int64_t batch_loaded_bytes = 0;
batch.reserve(cids.size());

for (auto cid : cids) {
auto loaded_size = translator_->estimated_byte_size_of_cell(cid).first;
auto cell_loaded_bytes = std::max(loaded_size.memory_bytes, loaded_size.file_bytes);
if (!batch.empty() && cell_loaded_bytes > max_loaded_bytes - batch_loaded_bytes) {
batches.push_back(std::move(batch));
batch = {};
batch_loaded_bytes = 0;
}
batch.push_back(cid);
batch_loaded_bytes += cell_loaded_bytes;
}

if (!batch.empty()) {
batches.push_back(std::move(batch));
}
return batches;
}

void
PinWarmupCells(OpContext* ctx) {
auto cids = AllCellIds();
if (!evictable_) {
PinInternal(ctx, cids, warmup_loading_timeout_);
return;
}
auto batches = LoadedSizeBatches(cids);
auto deadline = warmup_loading_timeout_.count() > 0 ? std::chrono::steady_clock::now() + warmup_loading_timeout_

@buqian-zilliz buqian-zilliz Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Do not charge load time against the reservation timeout

warmup_loading_timeout_ is the wait timeout passed to ReserveLoadingResourceWithTimeout, but this absolute deadline also consumes time spent in prior PinInternal() calls doing translator_->get_cells() I/O or waiting for another thread's load future. If the first batch reserves immediately but loads longer than the configured timeout, the next batch receives 0 and switches to best-effort admission, so transient resource contention aborts warmup even though none of the reservation-wait budget was used. Preserve a cumulative budget for DList reservation waits only, or explicitly introduce and enforce a separate end-to-end warmup deadline.

@buqian-zilliz buqian-zilliz Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue still exists. The shared deadline subtracts time spent loading earlier batches and waiting for their pins, so a later reservation can receive a 0 ms best-effort timeout even though the configured reservation-wait budget was never used.

@sparknack sparknack Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behavior is intentional. For batched warmup, warmup_loading_timeout_ is a shared wall-clock admission window, not a cumulative budget that counts only time spent inside DList reservation waits. Once earlier load or pin work consumes that window, later batches use timeout=0 instead of extending synchronous slot creation or prefetch-worker occupancy with another blocking wait. In-flight loads are not cancelled, so this is a soft admission deadline rather than a hard end-to-end timeout.

Added WarmupTimeoutTest.SlowFirstBatchExhaustsSharedAdmissionDeadline in 49061ad to lock this behavior: the first batch loads past the deadline, and the later contended batch fails best-effort instead of waiting for newly available resources.

: std::chrono::steady_clock::time_point::max();
for (const auto& batch : batches) {
auto timeout = warmup_loading_timeout_;
if (timeout.count() > 0) {
timeout = std::max(std::chrono::milliseconds(0), std::chrono::duration_cast<std::chrono::milliseconds>(
deadline - std::chrono::steady_clock::now()));
}
PinInternal(ctx, batch, timeout);

@buqian-zilliz buqian-zilliz Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Do not cycle oversized warmups through the cache

Each PinInternal() result is discarded at the end of the iteration, so the completed batch is unpinned and becomes evictable before the next reservation. When the slot is larger than cache capacity but each 512 MiB batch is individually admissible, DList can evict earlier warmup batches and existing cache entries to admit every later batch. The previous aggregate reservation rejected this case before get_cells() I/O; this loop can instead read the entire slot while at most the final cache-sized portion remains resident. Bound the total successful warmup work, or use a no-eviction admission mode for warmup, rather than cycling the same capacity through all batches.

}
}

std::shared_ptr<CellAccessor<CellT>>
PinInternal(OpContext* ctx, const std::vector<cid_t>& cids, std::chrono::milliseconds timeout) {
std::vector<folly::SemiFuture<internal::ListNode::NodePin>> futures;
Expand Down Expand Up @@ -740,6 +782,7 @@ class CacheSlot final : public std::enable_shared_from_this<CacheSlot<CellT>> {
std::chrono::milliseconds warmup_loading_timeout_{0};
// Bind and Unbind must use the same runtime-unit metadata even if Meta is later modified.
std::optional<LoadingOverheadConfig> loading_overhead_config_;
static constexpr int64_t kDefaultLoadedSizeBatchBytes = 1LL << 29;
std::atomic<bool> warmup_called_{false};
std::atomic<bool> skip_pin_{false};
};
Expand Down
Loading
Loading