Eliminate synthetic RMW for RI.SET/RI.DEL AOF logging#1725
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the synthetic no-op Tsavorite RMW path previously used to AOF-log RI.SET/RI.DEL, and replaces it with a direct AOF append at the RangeIndex command call sites, reducing overhead while keeping the AOF entry format compatible with existing replay logic.
Changes:
- Added a
TsavoriteLog.Enqueue<THeader,TInput>(THeader, ReadOnlySpan<byte>, ref TInput, out long)overload that uses the log’s internal epoch (no external epoch accessor required). - Switched
RI.SET/RI.DELAOF logging from synthetic RMW to directappendOnlyFile.Enqueue(...)calls via updatedRangeIndexManager.ReplicateRangeIndexSet/Del. - Removed now-dead RISET/RIDEL branches from RMW sizing/update logic and removed special-case AOF filtering previously needed for the synthetic path; also skips AOF for
RIPROMOTE/RIRESTORE.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/storage/Tsavorite/cs/src/core/TsavoriteLog/TsavoriteLog.cs | Adds a non-epoch Enqueue overload to support direct AOF appends without requiring a caller-provided epoch accessor. |
| libs/server/Storage/Session/MainStore/RangeIndexOps.cs | Routes RI.SET/RI.DEL to new direct AOF logging helpers instead of injecting synthetic RMWs. |
| libs/server/Resp/RangeIndex/RangeIndexManager.cs | Updates replication helpers to directly append StoreRMW AOF entries for RI.SET/RI.DEL. |
| libs/server/Storage/Functions/MainStore/VarLenInputMethods.cs | Removes RISET/RIDEL sizing paths that were only needed for synthetic RMW logging. |
| libs/server/Storage/Functions/MainStore/RMWMethods.cs | Removes RISET/RIDEL update paths and suppresses AOF marking for internal RIPROMOTE/RIRESTORE. |
| libs/server/Storage/Functions/MainStore/PrivateMethods.cs | Removes RISET/RIDEL AOF filtering that was specific to the synthetic logging approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
badrishc
force-pushed
the
badrishc/bftree-patch-1
branch
from
8026d37 to
99e24cf
Compare
Replace the synthetic no-op RMW path (RangeIndexManager.ReplicateRangeIndexSet/Del) with direct appendOnlyFile.Enqueue at the call site in RangeIndexOps. This avoids the overhead of a full Tsavorite RMW (hash lookup, epoch acquire, IPU/CopyUpdater, PostRMW) just to create an AOF entry. The AOF entry format is unchanged (AofHeader + key + StringInput with RISET/RIDEL command), so replay via AofProcessor.HandleRangeIndexSetReplay/DelReplay works without modification. Added a non-epoch Enqueue<THeader, TInput> overload to TsavoriteLog that uses the log's internal epoch (same as other non-epoch Enqueue overloads). Removed: - ReplicateRangeIndexSet/Del methods from RangeIndexManager - RISETAppendLogArg/RIDELAppendLogArg sentinel constants - RISET/RIDEL cases from NeedInitialUpdate, NeedCopyUpdate, InPlaceUpdater, CopyUpdater, VarLenInputMethods (all dead code now) - RISET/RIDEL exclusion from WriteLogRMW (no longer goes through RMW) Also skip AOF for RIPROMOTE/RIRESTORE (internal store-maintenance ops). Co-authored-by: Copilot <[email protected]>
badrishc
force-pushed
the
badrishc/bftree-patch-1
branch
from
99e24cf to
f395483
Compare
BGSAVE returns immediately and Thread.Sleep(500) is insufficient on slow CI runners. If checkpoint doesn't complete before post-checkpoint writes, the version ordering breaks: post-checkpoint AOF entries get the same version as the checkpoint and are skipped during replay by IsOldVersionRecord, causing data loss (e.g. key-C not found). Use synchronous SAVE which blocks until checkpoint completes, ensuring clean version boundaries between checkpoint and post-checkpoint data. Co-authored-by: Copilot <[email protected]>
tiagonapoli
approved these changes
Apr 22, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Replace the synthetic no-op RMW path (RangeIndexManager.ReplicateRangeIndexSet/Del) with direct appendOnlyFile.Enqueue at the call site in RangeIndexOps. This avoids the overhead of a full Tsavorite RMW (hash lookup, epoch acquire, IPU/CopyUpdater, PostRMW) just to create an AOF entry.
The AOF entry format is unchanged (AofHeader + key + StringInput with RISET/RIDEL command), so replay via AofProcessor.HandleRangeIndexSetReplay/DelReplay works without modification.
Added a non-epoch Enqueue<THeader, TInput> overload to TsavoriteLog that uses the log's internal epoch (same as other non-epoch Enqueue overloads).
Removed:
Also skip AOF for RIPROMOTE/RIRESTORE (internal store-maintenance ops).