enhance: batch evictable warmup by loaded size - #115
Conversation
27c1d0f to
bb26116
Compare
| } | ||
|
|
||
| [[nodiscard]] std::vector<std::vector<cid_t>> | ||
| LoadedSizeBatches(const std::vector<cid_t>& cids, int64_t max_loaded_bytes = kDefaultLoadedSizeBatchBytes) const { |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
| return; | ||
| } | ||
| for (const auto& batch : LoadedSizeBatches(cids)) { | ||
| PinCellsDirect(ctx, batch, warmup_loading_timeout_); |
There was a problem hiding this comment.
[P2] Do not restart the warmup timeout for every batch
Each iteration passes the full positive warmup_loading_timeout_, and DList computes a fresh deadline for every call. Before this change a warmup had one reservation deadline; with N contended batches the same synchronous warmup can block slot creation for up to N times the configured limit, while an asynchronous warmup can occupy a prefetch worker equally long. Establish one deadline before the loop and pass the remaining duration to each batch.
There was a problem hiding this comment.
Addressed in the pending update. PinWarmupCells() now establishes one deadline before iterating and passes only the remaining duration to each batch. Zero timeout remains best-effort and negative timeout remains unbounded. Added coverage in WarmupTimeoutTest.PositiveTimeoutAppliesAcrossAllBatches.
| return; | ||
| } | ||
| for (const auto& batch : LoadedSizeBatches(cids)) { | ||
| PinCellsDirect(ctx, batch, warmup_loading_timeout_); |
There was a problem hiding this comment.
[P2] Do not remap CID batches as UIDs
LoadedSizeBatches() measures the CIDs returned by AllCellIds(), but PinCellsDirect() accepts UIDs and applies cell_id_of() again in CUSTOMIZED mode. The cells loaded for a batch can therefore differ from those whose sizes established the 1 GiB bound, invalidating the cap for a supported mapping mode. Pass these batches to a CID-oriented path such as PinInternal().
There was a problem hiding this comment.
Addressed in the pending update. Warmup already operates on CIDs from AllCellIds(), so both evictable and non-evictable paths now call PinInternal() directly. UID-facing APIs still perform the mapping once. Added non-identity UID-to-CID coverage in WarmupTest.WarmupUsesCidsWithoutUidRemapping.
| return; | ||
| } | ||
| auto batches = LoadedSizeBatches(cids); | ||
| auto deadline = warmup_loading_timeout_.count() > 0 ? std::chrono::steady_clock::now() + warmup_loading_timeout_ |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Shawn Wang <shawn.wang@zilliz.com>
Signed-off-by: Shawn Wang <shawn.wang@zilliz.com>
Signed-off-by: Shawn Wang <shawn.wang@zilliz.com>
49061ad to
a02f129
Compare
| timeout = std::max(std::chrono::milliseconds(0), std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| deadline - std::chrono::steady_clock::now())); | ||
| } | ||
| PinInternal(ctx, batch, timeout); |
There was a problem hiding this comment.
[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.
|
|
||
| [[nodiscard]] std::vector<std::vector<cid_t>> | ||
| LoadedSizeBatches(const std::vector<cid_t>& cids, int64_t max_loaded_bytes = kDefaultLoadedSizeBatchBytes) const { | ||
| std::vector<std::vector<cid_t>> batches; |
There was a problem hiding this comment.
nit: make function name a verb, not noun. such as SplitCellsIntoBatches?
memory_bytesandfile_bytesPinAllCells()as single loading requests