enhance: support transactional loading overhead policies - #109
Conversation
| } | ||
|
|
||
| const auto bound = std::max(capacity_bytes_, input.max_runtime_unit_bytes); | ||
| return std::max(observed_inflight_bytes_, std::min(input.sum_active_overhead_bytes, bound)); |
There was a problem hiding this comment.
BudgetBoundCalculator uses observed_inflight_bytes_ as a hard floor via the outer std::max, which only clears when the owner publishes a new calculator. Unlike ExecutorBoundCalculator whose min(sum_active, bound) drains to zero as work completes, this floor stays pinned even after sum_active_overhead_bytes drains to 0, so a one-shot shrink publishing BudgetBoundCalculator(cap, N) holds N bytes reserved in DList until the owner republishes. The Manager owner contract says the floor should hold the reservation only 'before existing work drains' but never requires a post-drain republication, so those bytes are never reclaimed.
There was a problem hiding this comment.
Fixed in 789e554. BudgetBoundCalculator now returns zero once tracked active demand drains, so observed_inflight is only a floor while demand is live. Added BudgetBoundReleasesInflightFloorAfterTrackedDemandDrains.
| auto name_it = name_to_handle.find(update->group); | ||
| if (name_it != name_to_handle.end()) { | ||
| auto& state = dimension_group_state_.at(name_it->second); | ||
| if (!state.typed) { |
There was a problem hiding this comment.
The guard that rejects a typed policy update on an existing legacy group is deliberate and pinned by TypedUpdateRejectsLegacyGroup, and it is protective: legacy reserves never populate runtime_unit_ref_counts, so letting a typed calculator take over a live legacy group would compute targets with max_runtime_unit=0 and could release overhead for in-flight legacy loads. The real but minor consequence is that a group name first registered via the legacy API cannot be upgraded to the typed API in place while registrations are alive, so an in-place migration must use a distinct group name (the trigger group kLoadTransientOverheadGroup is downstream and does not exist in this repo). Worth a doc note; the suggested 'let typed supersede legacy' remedy would be unsafe.
There was a problem hiding this comment.
Documented in 789e554. The legacy guard remains deliberate: a live legacy Group is not promoted because existing reservations do not have typed runtime-unit state. Typed Groups must publish the initial policy before CacheSlot membership; an in-place migration must use a new Group name.
|
|
||
| if (decrease.AnyGTZero()) { | ||
| auto scaled_decrease = decrease * eviction_config_.loading_resource_factor; | ||
| total_loading_size_ -= scaled_decrease; |
There was a problem hiding this comment.
The policy path adjusts DList's total_loading_size_ (reserveResourceInternalImpl on growth, direct subtraction on shrink) but never touches the cache_loading_bytes gauge, which is only moved on the CacheSlot/Manager reserve/release path. After shrink-then-drain the gauge is left permanently over-reporting: reserve 100 (gauge=100), publish a calculator lowering the target to 50 (total_loading_size_-=50, gauge unchanged), then release — the tracker returns a 50 delta, so the gauge lands at 50 while true reserved is 0; expansion under-reports symmetrically. Emit the corresponding gauge delta from the policy path (the gauge is labelled by cell_data_type, which the policy path lacks, so the fix needs a labelling decision). Note the new DListTest policy tests call DList directly and bypass the gauge, so this drift is untested.
There was a problem hiding this comment.
Fixed in 789e554 by decoupling the per-load gauge from mutable Group reservation deltas. CacheSlot now increments and decrements the same loaded_resource + loading_overhead request estimate, while policy updates only adjust DList admission bookkeeping. Added an active-load policy-shrink regression test and clarified the metric help text.
| if (increase.AnyGTZero()) { | ||
| auto rollback = []() {}; | ||
| auto [success, _] = | ||
| reserveResourceInternalImpl(increase * eviction_config_.loading_resource_factor, rollback); |
There was a problem hiding this comment.
[P2] Apply rounding to absolute targets instead of each delta
Scaling each signed policy delta independently makes total_loading_size_ drift when loading_resource_factor is fractional. With a factor of 1.5, target changes 0 -> 1 -> 2 -> 0 apply scaled deltas +2, +2, -3, leaving one byte reserved although the tracker target is zero; repeating the cycle accumulates the error and can eventually reject valid reservations. Track the scaled old and new targets and apply round(new_target * factor) - round(old_target * factor) rather than rounding the unscaled delta.
There was a problem hiding this comment.
Fixed in 789e554. Tracker/DList now carry absolute previous and current Group targets and scale both endpoints before subtraction. The same transition is used for reserve, release, policy updates, and waiter rollback. Added a 1.5 loading_resource_factor regression covering 0 -> 1 -> 2 -> 0.
| auto name_it = name_to_handle.find(update->group); | ||
| if (name_it != name_to_handle.end()) { | ||
| auto& state = dimension_group_state_.at(name_it->second); | ||
| if (!state.typed) { |
There was a problem hiding this comment.
[P1] Allow initial policy publication after group registration
registerDimensionGroup() creates every unknown group as a legacy state with typed == false, even when the registration carries the new max_runtime_unit metadata. If a CacheSlot registers before the owner publishes the group's first policy, this check rejects every subsequent update with kIncompatiblePolicy while those slots remain, so the group cannot adopt calculator-backed behavior. Preserve the intended group kind during registration and allow the first policy to initialize or promote a typed placeholder, or atomically declare typed groups before registrations are accepted.
There was a problem hiding this comment.
Resolved through the explicit initialization contract in 789e554: the authoritative owner publishes the initial typed Policy before any CacheSlot registers membership. Arbitrary legacy-to-typed promotion remains rejected because live legacy reservations lack runtime-unit tracking and promotion could under-reserve them.
|
|
||
| if (decrease.AnyGTZero()) { | ||
| auto scaled_decrease = decrease * eviction_config_.loading_resource_factor; | ||
| total_loading_size_ -= scaled_decrease; |
There was a problem hiding this comment.
[P2] Reconcile loading-byte gauges during policy updates
Policy updates change total_loading_size_ without adjusting cache_loading_bytes, but CacheSlot increments that gauge using the tracker delta at admission and decrements it using the potentially different delta at release. For example, if active group overhead changes from a target of 50 to 80, the update adds 30 only to DList; the eventual releases decrement 80 from a gauge that was incremented by 50, driving it negative. Shrinks leave the inverse positive leak. Account for policy deltas in the gauges with preserved attribution, or decouple per-load metrics from mutable tracker deltas.
There was a problem hiding this comment.
Fixed in 789e554 by making cache_loading_bytes an active request-estimate gauge rather than a view of mutable DList Group targets. Each CacheSlot now decrements exactly the stable value it incremented, so both shrink and expansion policy updates cannot leak or drive the gauge negative. Added a concurrent active-load update test.
| if (lhs == 0 || rhs == 0) { | ||
| return 0; | ||
| } | ||
| if (lhs > std::numeric_limits<int64_t>::max() / rhs) { |
There was a problem hiding this comment.
[P2] Validate signed inputs before saturating multiplication
SaturatingMultiply() assumes non-negative operands, but worker counts and max_runtime_unit accept unchecked int64_t values. Initial policy publication can accept a negative configuration because it evaluates an empty group with a zero runtime unit; a later reservation can then return INT64_MAX incorrectly or execute signed-overflow undefined behavior, such as -2 * INT64_MAX. Reject negative calculator and runtime-unit inputs before publication or registration, and make the multiplication explicitly safe for the supported domain.
There was a problem hiding this comment.
Fixed in 789e554. Calculator constructors reject negative capacity, inflight, and worker inputs; registration rejects negative upper bounds and max_runtime_unit values before mutating tracker state; ExecutorBoundCalculator also uses defensive saturating multiplication. Added negative-input and overflow regression tests.
| } | ||
|
|
||
| const auto bound = std::max(capacity_bytes_, input.max_runtime_unit_bytes); | ||
| return std::max(observed_inflight_bytes_, std::min(input.sum_active_overhead_bytes, bound)); |
There was a problem hiding this comment.
[P1] Reject policy-sized waiters that can never fit
BudgetBoundCalculator may return observed_inflight_bytes_ even when it exceeds this request's loaded + overhead, and the calculator contract permits targets above active overhead. Queued tracker requests nevertheless record only (loaded + overhead) * loading_resource_factor as their required size. If the policy-derived target exceeds the DList capacity while that smaller estimate does not, every retry fails but the permanent-impossibility check never rejects the waiter; it then blocks requests ordered behind it until timeout, or indefinitely for a negative timeout. Store or recompute the policy-derived requirement for queued requests and fail requests whose actual target cannot fit.
There was a problem hiding this comment.
Fixed in f13e1ea. Tracker-aware waiters now retain and recompute the checked policy-derived scaled requirement. Initial admission and retry both fail immediately when that requirement exceeds DList capacity. Covered by PolicySizedWaiterThatExceedsCapacityFailsImmediately and PolicyUpdateRejectsWaiterThatBecomesPermanentlyImpossible.
| [[nodiscard]] int64_t | ||
| ComputeReservationTarget(const LoadingOverheadCalculatorInput& input) const noexcept override { | ||
| const auto bound = SaturatingMultiply(effective_concurrency_, input.max_runtime_unit_bytes); | ||
| return std::min(input.sum_active_overhead_bytes, bound); |
There was a problem hiding this comment.
[P2] Do not use zero bytes as the reserve failure sentinel
ExecutorBoundCalculator(0, 0) validly returns a zero target for positive active overhead. When a load also has zero estimated final usage, DList successfully reserves zero bytes and returns ResourceUsage{}, which CacheSlot::RunLoad() interprets as failure. The release guard is then skipped, leaving the Tracker's active-overhead and runtime-unit entries behind. Return an explicit success flag alongside the reserved size, or otherwise distinguish zero-byte success from failure so the tracker mutation is released normally.
There was a problem hiding this comment.
Fixed in f13e1ea. Tracker-aware reservation now returns an explicit {success, reserved} result, so a successful zero-byte reservation is distinct from failure and CacheSlot still performs the matching Release. Covered by ZeroByteReservationIsSuccessful.
|
|
||
| result = | ||
| loading_overhead_tracker_->UpdatePolicies(update, [this](const LoadingOverheadReservationChange& change) { | ||
| const auto signed_delta = change.current_group_target * eviction_config_.loading_resource_factor - |
There was a problem hiding this comment.
[P2] Check calculator target scaling for overflow
Calculator targets may span the full non-negative int64_t range, but this transition scales them through ResourceUsage::operator*(double), which rounds and casts back to int64_t. INT64_MAX already rounds to 2^63 as a double, and a loading factor above 1 makes more targets unrepresentable. The out-of-range conversion can turn an expansion into a negative or zero delta, allowing the policy to commit without reserving the required DList capacity. Use checked scaling and reject targets or factors whose scaled endpoints cannot be represented.
There was a problem hiding this comment.
Fixed in f13e1ea. DList now uses checked scaling and checked addition for target endpoints and request transitions, rejecting non-finite/invalid factors and unrepresentable results before conversion or policy commit. Covered by RejectsUnrepresentableScaledPolicyTarget.
| } | ||
|
|
||
| const auto target = computeTarget(state); | ||
| const auto delta = std::max(target - state.overhead_reserved, int64_t{0}); |
There was a problem hiding this comment.
[P2] Require monotonic targets across demand changes
The public calculator contract permits any deterministic non-negative absolute target, but reserveDimension() silently ignores target decreases while releaseDimension() ignores target increases. A valid custom calculator that lowers its target when another request becomes active therefore leaves the old larger reservation booked, potentially rejecting unrelated loads even though the authoritative policy requested less capacity. Either document and enforce monotonicity with respect to active demand, or propagate signed endpoint transitions for both reserve and release.
There was a problem hiding this comment.
Fixed in f13e1ea. The Calculator contract now requires monotonic non-decreasing targets for a fixed policy snapshot. Tracker diagnoses violations and retains the current target instead of silently changing accounting in the wrong direction. Reserve and Release violations both have regression tests.
| auto target = std::min(std::max(state.sum_of_overhead, int64_t{0}), state.upper_bound); | ||
| auto delta = std::max(target - state.overhead_reserved, int64_t{0}); | ||
| if (state.typed) { | ||
| state.runtime_unit_ref_counts[runtime_unit]++; |
There was a problem hiding this comment.
[P3] Remove per-load allocation from the tracker critical section
Each typed-group admission inserts into a std::map while both the DList and Tracker mutexes are held, and the final release erases the entry. Even a group with one fixed runtime unit therefore allocates and frees a tree node on every idle-to-active-to-idle load cycle; absent max_runtime_unit, distinct estimates also add O(log N) work and allocator contention. Preserve reusable entries or add an allocation-free fast path for the common single-runtime-unit case.
There was a problem hiding this comment.
Leaving this as a measured performance follow-up. Retaining erased runtime-unit entries can grow metadata without bound when max_runtime_unit is absent and estimates vary. A safe allocation-free fast path needs additional representation/state complexity, so the design doc records profiling first; we can add the fast path if allocation contention is measurable.
| if (prepared.next_target < 0) { | ||
| return LoadingOverheadUpdateResult::kInvalidArgument; | ||
| } | ||
| prepared.deferred = prepared.state != nullptr && prepared.next_target < previous_target; |
There was a problem hiding this comment.
[P1] Keep deferred policies covered when calculator curves cross
Deferral is decided only against the target at publication, but while pending all Reserve and Release paths keep using only the effective calculator. Two monotone built-ins can cross: with active (sum=1000, max=100), ExecutorBoundCalculator(2) targets 200 and a new BudgetBoundCalculator(100) targets 100, so the update is deferred; after demand falls to (sum=80, max=10), the effective target is 20 while the accepted pending target is 80, and Release drops physical reservation to 20. The runtime owner has already installed the new limit, so DList can under-account by 60. Keep the reservation at max(effective, pending) until promotion, or only defer replacements proven pointwise no larger than the effective policy.
There was a problem hiding this comment.
Fixed in af68766. While a policy is pending, the tracker now reserves max(effective_target, pending_target); promotion still occurs only when the targets converge. Added DeferredPolicyKeepsMaximumAcrossCrossingTargets.
|
|
||
| If the actual requirement exceeds DList capacity, the waiter fails instead of indefinitely blocking requests behind it. This check is repeated when policy state changes and the waiter is retried. | ||
|
|
||
| Waiters are reprocessed after: |
There was a problem hiding this comment.
[P1] Reprocess waiters after cleanup and capacity changes
This guarantee is not wired to UpdateMaxLimit or watermark updates, nor to the timeout and cancellation callbacks; those paths only mutate state or erase the map entry and never call handleWaitingRequests(). If a large head waiter blocks a smaller indefinite waiter and then times out or is cancelled, or capacity is raised enough for the smaller request, it can remain queued forever until an unrelated release or eviction occurs. Re-run queue processing after these events and destroy retired requests outside list_mtx_.
There was a problem hiding this comment.
Fixed in af68766. Limit and watermark updates, timeout cleanup, and cancellation cleanup now rerun handleWaitingRequests(), and retired requests are destroyed after releasing list_mtx_. Added UpdateMaxLimitIncreaseReprocessesWaiters, TimeoutOfHeadWaiterReprocessesFollowingRequest, and CancellationOfHeadWaiterReprocessesFollowingRequest.
There was a problem hiding this comment.
[P2] Make active-demand mutation transactional and overflow-safe
reserveDimension() performs unchecked sum_of_overhead += overhead before the typed-group map insertion. A capped Group can accumulate estimates beyond INT64_MAX, and inserting a first-seen runtime unit can throw std::bad_alloc after the sum changed; either failure occurs before DList has a transition to roll back, while CacheSlot catches the exception and leaves phantom or corrupt active demand. Check the prospective sum and make the sum/multiset mutation transactional with an internal rollback guard.
There was a problem hiding this comment.
Fixed in af68766. Reserve prechecks checked sums for both dimensions before mutation, so overflow cannot leave partial active demand. Runtime-unit membership state is now updated only by Register/Unregister with a cached maximum; Reserve/Release only mutate sum_of_overhead. Added ReserveOverflowDoesNotMutateActiveDemand and MultiDimensionReserveOverflowIsAtomic.
| const auto signed_delta = | ||
| CheckedSignedTransition(change.previous_group_target, change.current_group_target, | ||
| eviction_config_.loading_resource_factor); | ||
| if (!signed_delta.has_value()) { |
There was a problem hiding this comment.
[P2] Distinguish invalid scaling from capacity rejection
CheckedSignedTransition() returns no value for an unrepresentable scaled target, but the callback's false result is collapsed to kReservationRejected. A target near INT64_MAX with a loading factor above one is permanently invalid, yet the documented owner will treat it as transient capacity pressure and retry while keeping the runtime expansion blocked. Propagate an invalid-transition result separately and return kInvalidArgument; reserve kReservationRejected for actual capacity or eviction failure.
There was a problem hiding this comment.
Fixed in af68766. Invalid checked-scaling transitions are now tracked separately and return kInvalidArgument; kReservationRejected remains reserved for capacity or eviction failure. Updated RejectsUnrepresentableScaledPolicyTarget.
| monitor::cache_cell_count(cell_data_type_, storage_type_).Increment(translator_->num_cells()); | ||
| // Register after all potentially-throwing operations, so that if the constructor | ||
| // fails, we don't leak a ref_count (destructor won't run for incomplete objects). | ||
| // Attach loading-overhead group membership after all potentially-throwing |
There was a problem hiding this comment.
[P2] Roll back metrics when membership registration fails
RegisterLoadingOverhead() now has expected validation failure paths, but the slot and cell gauges are incremented before registration. If registration throws, construction aborts and ~CacheSlot() never runs, permanently inflating both gauges. Register before incrementing the metrics or protect the increments with a constructor rollback guard.
| auto rollback = [overhead_handle, overhead, tracker]() { | ||
| if (overhead_handle != LoadingOverheadTracker::kInvalidHandle && tracker != nullptr) { | ||
| tracker->Release(overhead_handle, overhead); | ||
| tracker->ReleaseWithTargetChange(overhead_handle, overhead); |
There was a problem hiding this comment.
[P2] Reconcile the physical target during failed-admission rollback
The rollback discards the transition returned by ReleaseWithTargetChange(). An idle membership supplying the maximum runtime-unit bound can unregister between the tentative Reserve and this rollback because registration changes do not take list_mtx_; the rollback can then lower the Tracker target without subtracting the corresponding physical reservation. Subsequent transitions start from the lowered logical target and leave total_loading_size_ permanently inflated. Serialize membership changes with the admission transaction or apply the rollback endpoint transition to DList accounting.
| std::optional<ResourceUsage> attempted_requirement; | ||
| if (request_ptr_ref->use_resource_promise) { | ||
| auto [ok, unscaled] = | ||
| auto attempt = |
There was a problem hiding this comment.
[P2] Fail overflowing waiters without aborting queue processing
A tracker-aware waiter can pass validation when enqueued but overflow sum_of_overhead when retried after other requests join its Group. ReserveWithTargetChange() then throws through handleWaitingRequests(), aborting release, configuration-update, or timeout/cancellation processing while leaving this waiter and later requests stranded. Catch reservation-validation exceptions per waiter, fail and remove that request, and continue processing the queue.
| result = LoadingOverheadUpdateResult::kInvalidArgument; | ||
| } | ||
|
|
||
| if (result == LoadingOverheadUpdateResult::kApplied) { |
There was a problem hiding this comment.
[P2] Retry waiters when a rejected policy update evicts cells
Waiters are processed only when the policy update returns kApplied. The attempted reservation can evict cells and still return kReservationRejected if physical-memory pressure remains, so logical capacity may have been freed for an existing smaller or disk-only waiter even though the update failed. Track whether the failed attempt evicted capacity and reprocess the queue on that rejection path as well.
| // Track the active request estimate rather than the mutable Group reservation. | ||
| // Policy updates may change DList bookkeeping while this request is in flight, | ||
| // but the metric must decrement the same value that it incremented. | ||
| metric_loading_resource = loaded_resource + loading_overhead; |
There was a problem hiding this comment.
[P2] Check the active-request metric sum for overflow
metric_loading_resource is formed with unchecked signed addition. Group capping can make the DList transition representable and admit the request even when the uncapped loaded_resource + loading_overhead estimate exceeds INT64_MAX, causing undefined behavior or a corrupted gauge. Use checked addition and preserve the validated value for the paired decrement.
| return result; | ||
| } | ||
|
|
||
| if (change.previous_group_target != change.current_group_target && !try_apply_reservation_change(change)) { |
There was a problem hiding this comment.
[P2] Validate the loading factor on zero-delta policy updates
The reservation callback is skipped when the previous and current targets are equal, but that callback is the only policy-update path that validates loading_resource_factor. Initial publication for an idle Group is normally 0 -> 0, so a non-finite or non-positive factor can still return kApplied, after which every real reservation fails. Validate the factor before preparing the update or invoke checked transition validation even for zero deltas.
| When all contracts above are satisfied: | ||
|
|
||
| - **No stale per-CacheSlot policy overwrite.** Policy is Group-level state published by one serialized owner. | ||
| - **No idle-membership pinning.** Idle registrations do not contribute active demand or maximum runtime unit. |
There was a problem hiding this comment.
[P3] Correct the idle-membership guarantee
This guarantee says idle registrations do not contribute the maximum runtime unit, but Tracker caches the maximum across all registered memberships and uses it whenever any request in the Group is active. The same document later acknowledges that an idle large membership makes another active membership reserve more conservatively. Rewrite the guarantee so downstream owners do not rely on false per-membership isolation.
7426569 to
77c5c15
Compare
Signed-off-by: Shawn Wang <shawn.wang@zilliz.com>
77c5c15 to
771f102
Compare
Summary
This supersedes #108.
Related Milvus integration: milvus-io/milvus#51403
Contract
loading_overheadconservatively covers actual transient usage from successful Reserve through paired ReleaseCompatibility
BudgetBoundCalculatornow accepts only configured capacityExecutorBoundCalculatornow accepts only configured workersTest Plan
cmake --build build --target cachinglayer_test all_tests -j1source build/conanrun.sh && ctest --test-dir build --output-on-failure(2/2 passed)git diff --check