Skip to content

Reply Copy Avoidance #14608

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

Jump to bottom
Open
sundb wants to merge 18 commits into
base: unstable
Choose a base branch
from
Open

Reply Copy Avoidance #14608

sundb wants to merge 18 commits into from

Conversation

@sundb
Copy link
Collaborator

@sundb sundb commented Dec 8, 2025 •
edited
Loading

This PR is based on valkey-io/valkey#2078

Reply Copy Avoidance Optimization

This PR introduces an optimization to avoid unnecessary memory copies when sending replies to clients in Redis.

Overview

Currently, Redis copies reply data into client output buffers before sending responses. This PR implements a mechanism to avoid these copies in certain scenarios, improving performance and reducing memory overhead.

Key Changes

  • Added capability to reply construction allowing to interleave regular
    replies with copy avoid replies in client reply buffers
  • Extended write-to-client handlers to support copy avoid replies
  • Added copy avoidance of string bulk replies when copy avoidance
    indicated by I/O threads
  • Copy avoidance is beneficial for performance despite object size only
    starting certain number of threads. So it will be enabled only starting
    certain number of threads. (TODO)

Note: When copy avoidance disabled content and handling of client
reply buffers remains as before this PR

Signed-off-by: Alexander Shabanov [email protected]
Signed-off-by: xbasel [email protected]
Signed-off-by: Madelyn Olson [email protected]
Co-authored-by: Alexander Shabanov [email protected]
Co-authored-by: xbasel [email protected]
Co-authored-by: Madelyn Olson [email protected]

@sundb sundb mentioned this pull request Dec 8, 2025
Closed
@sundb sundb force-pushed the reply-copy-avoid-1 branch 2 times, most recently from a4c387c to b5206dd Compare December 8, 2025 06:59
@sundb sundb added the release-notes indication that this issue needs to be mentioned in the release notes label Dec 8, 2025
@sundb sundb added this to Redis 8.6 Dec 8, 2025
@github-project-automation github-project-automation bot moved this to Todo in Redis 8.6 Dec 8, 2025
@sundb sundb requested a review from oranagra December 9, 2025 15:01
@sundb
Copy link
Collaborator Author

sundb commented Dec 10, 2025 •
edited
Loading

@oranagra follow #14544 (comment)
another way is to reduce refcount from 30bits to 16bits(unsigned short), i think 65535 is enough.

please take a look this commit: 852beaf

the only place where refcount overflow is the test in propagate, but the behavior is not reasonable, so i think we can ignore it.

@sundb sundb mentioned this pull request Dec 11, 2025
Open
@oranagra
Copy link
Member

oranagra commented Dec 14, 2025

@sundb which test in propagate? who holds all these references?
another area that worries me is modules (i can't really tell what they're using it for and how many references we may be facing)

@sundb
Copy link
Collaborator Author

sundb commented Dec 15, 2025

@sundb which test in propagate? who holds all these references? another area that worries me is modules (i can't really tell what they're using it for and how many references we may be facing)

module propagates from timer test.
this test was added cause that module kept calling RedisModule_ReplicateVerbatim, and every call will incr the refcount on c->argv.

@oranagra
Copy link
Member

oranagra commented Dec 15, 2025

ok. let's use 16 bits, and hope it'll be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra Awaiting requested review from oranagra

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

release-notes indication that this issue needs to be mentioned in the release notes

Projects

Redis 8.6
Status: Todo

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sundb @oranagra