Reduce AofAddress struct copy overhead on hot paths#1759
Merged
Conversation
…path Add scalar GetTailAddress/GetBeginAddress methods to GarnetLog and use GetTailAddress in ProcessPrimaryStream instead of copying full struct. Co-authored-by: Copilot <[email protected]>
Avoids copying 513-byte struct on every call to Equals, SetValueIf, MinExchange, AnyLesser, AnyGreater, Diff, AggregateDiff, EqualsAll, and IsOutOfRange. Co-authored-by: Copilot <[email protected]>
…ider Co-authored-by: Copilot <[email protected]>
GetPreviousAddress(int) and GetStartAddress(int) avoid reconstructing the full 513-byte AofAddress struct when only one sublog value is needed. Updated SafeTruncateAof hot path to use the scalar accessor. Co-authored-by: Copilot <[email protected]>
Callers that only need a single sublog offset now use the scalar accessor instead of copying the 513-byte AofAddress struct. Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce performance overhead from frequent by-value copying of the large AofAddress struct on AOF/replication hot paths by switching to in parameters and adding scalar sublog accessors.
Changes:
- Updates multiple APIs to pass
AofAddressbyinto avoid by-value copies (e.g., cluster truncate and replication/AOF helpers). - Adds scalar accessors (e.g., tail/begin/replication offsets per sublog) and updates hot-path replication callers to use them.
- Refactors some AOF sync/replication code to read/write per-sublog values without materializing full
AofAddressinstances.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/server/Cluster/IClusterProvider.cs | Updates SafeTruncateAOF signature to take in AofAddress. |
| libs/server/AOF/GarnetLog.cs | Adds GetTailAddress(int) / GetBeginAddress(int) scalar accessors for sublogs. |
| libs/server/AOF/AofAddress.cs | Switches many methods to in parameters; refactors comparisons/diffs; modifies max sublog constant. |
| libs/cluster/Server/Replication/ReplicationManager.cs | Adds GetReplicationOffset(int) scalar accessor to avoid returning full AofAddress. |
| libs/cluster/Server/Replication/ReplicaOps/AOFReplay/ReplicaReplaySession.cs | Updates divergence checks to use scalar tail address accessor. |
| libs/cluster/Server/Replication/ReplicaOps/AOFReplay/ReplicaReplayDriver.cs | Updates offset logging and throttle logic to use scalar accessors. |
| libs/cluster/Server/Replication/PrimaryOps/AofOperations/AofSyncDriverStore.cs | Uses scalar previous address accessor; updates APIs to accept in AofAddress. |
| libs/cluster/Server/Replication/PrimaryOps/AofOperations/AofSyncDriver.cs | Adds scalar sublog accessors (GetPreviousAddress, GetStartAddress). |
| libs/cluster/Server/Gossip/GarnetServerNode.cs | Uses scalar replication offset for CLUSTER NODES reporting. |
| libs/cluster/Server/ClusterProvider.cs | Updates SafeTruncateAOF implementation to match in signature. |
Comments suppressed due to low confidence (1)
libs/server/AOF/AofAddress.cs:241
- Same as above:
SetValueIf(in AofAddress value, ...)readsvalue[i]in a loop. If the indexer is notreadonly, this can cause defensive copies / negate the benefit of usingin. Prefer direct fixed-buffer reads (or a readonly accessor) forinparameters in hot loops.
public void SetValueIf(in AofAddress value, long comparand)
{
for (var i = 0; i < Length; i++)
{
if (addresses[i] == comparand)
addresses[i] = value[i];
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Make indexer getter readonly to prevent defensive copies on 'in' params - Reference AofAddress.MaxSublogCount in Options.cs validation attribute - Fix XML docs: 'allocating' -> 'copying' for scalar accessors Co-authored-by: Copilot <[email protected]>
badrishc
approved these changes
May 1, 2026
chenhao-ye
pushed a commit
to chenhao-ye/garnet
that referenced
this pull request
May 13, 2026
* Avoid copying 513-byte AofAddress struct in ReplicaReplaySession hot path Add scalar GetTailAddress/GetBeginAddress methods to GarnetLog and use GetTailAddress in ProcessPrimaryStream instead of copying full struct. Co-authored-by: Copilot <[email protected]> * Use 'in' parameter modifier for AofAddress instance methods Avoids copying 513-byte struct on every call to Equals, SetValueIf, MinExchange, AnyLesser, AnyGreater, Diff, AggregateDiff, EqualsAll, and IsOutOfRange. Co-authored-by: Copilot <[email protected]> * Use 'in' for AofAddress params in AofSyncDriverStore and IClusterProvider Co-authored-by: Copilot <[email protected]> * Add scalar accessors to AofSyncDriver for per-sublog access GetPreviousAddress(int) and GetStartAddress(int) avoid reconstructing the full 513-byte AofAddress struct when only one sublog value is needed. Updated SafeTruncateAof hot path to use the scalar accessor. Co-authored-by: Copilot <[email protected]> * Add scalar GetReplicationOffset(int) to avoid full struct copies Callers that only need a single sublog offset now use the scalar accessor instead of copying the 513-byte AofAddress struct. Co-authored-by: Copilot <[email protected]> * reduce default maximume sublog count * Address PR review comments - Make indexer getter readonly to prevent defensive copies on 'in' params - Reference AofAddress.MaxSublogCount in Options.cs validation attribute - Fix XML docs: 'allocating' -> 'copying' for scalar accessors Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Copilot <[email protected]>
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.
AofAddress is a 513-byte struct (1 byte + fixed long[64]) that was frequently copied by value on performance-critical paths.
Changes:
inmodifier to all AofAddress instance method parameters (Equals, AnyLesser, AnyGreater, Diff, AggregateDiff, etc.)inmodifier to AofSyncDriverStore and IClusterProvider method parametersGetTailAddress(int),GetBeginAddress(int),GetPreviousAddress(int),GetReplicationOffset(int)) to avoid full struct copies when only one sublog value is needed