-
Notifications
You must be signed in to change notification settings - Fork 970
Reply Copy Avoidance #2078
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.
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
Reply Copy Avoidance #2078
Conversation
Signed-off-by: Alexander Shabanov <[email protected]>
Signed-off-by: Alexander Shabanov <[email protected]>
Signed-off-by: Alexander Shabanov <[email protected]>
Signed-off-by: Alexander Shabanov <[email protected]>
Signed-off-by: Alexander Shabanov <[email protected]>
Signed-off-by: Alexander Shabanov <[email protected]>
Signed-off-by: Alexander Shabanov <[email protected]>
Signed-off-by: Alexander Shabanov <[email protected]>
Signed-off-by: Alexander Shabanov <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #2078 +/- ##
============================================
+ Coverage 71.29% 71.51% +0.21%
============================================
Files 122 122
Lines 66196 66410 +214
============================================
+ Hits 47197 47490 +293
+ Misses 18999 18920 -79
🚀 New features to boost your workflow:
|
16ded37 to
d40909f
Compare
Signed-off-by: xbasel <[email protected]>
|
Clone of #1457 + test fixes |
|
So Sasha is not going to work on #1457 anymore? We should close it then to avoid confusion. |
Signed-off-by: Madelyn Olson <[email protected]>
|
The schema validation seems to be failing consistently. It's not reporting which test, but it seems to be because its expecting an integer reply and getting nothing. |
|
Also checked to see if there were any test failures if you force everything to offload, and just the ones we expected: So for the schema, I think we just need to run it locally and see what test is actually failing (it'll be in the output file) and then we can check what is not working correctly. |
|
Signed-off-by: xbasel <[email protected]>
|
The undefined-sanitizer fails in Daily with an unaligned pointer address. I'm just guessing it can be related to this PR. Is it? https://github.com/valkey-io/valkey/actions/runs/15547432999/job/43771545681#step:10:301 |
Seems likely. @xbasel Can you take a look? |
### Overview This PR introduces the ability to avoid copying the content of string object into replies (i.e. bulk string replies) and to allow I/O threads refer directly to obj->ptr in writev iov. ### 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 * Minor changes in cluster slots stats in order to support `network-bytes-out` for copy avoid replies * 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. Internal configuration ``min-io-threads-copy-avoid`` introduced to manage this number of threads **Note**: When copy avoidance disabled content and handling of client reply buffers remains as before this PR ### Implementation Details #### ``client`` and ``clientReplyBlock`` structs: 1. ``buf_encoded`` flag has been added to ``clientReplyBlock`` struct and to ``client`` struct for static ``c->buf`` to indicate if reply buffer is in copy avoidance mode (i.e. include headers and payloads) or not (i.e. plain replies only). 2. ``io_last_written_buf``, ``io_last_written_bufpos``, ``io_last_written_data_len`` fields added ``client`` struct to to keep track of write state between ``writevToClient`` invocations #### Reply construction: 1. Original ```_addReplyToBuffer``` and ```_addReplyProtoToList``` have been renamed to ```_addReplyPayloadToBuffer``` and ```_addReplyPayloadToList``` and extended to support different types of payloads - regular replies and copy avoid replies. 3. New ```_addReplyToBuffer``` and ```_addReplyProtoToList``` calls now ```_addReplyPayloadToBuffer``` and ```_addReplyPayloadToList``` and used for adding **regular** replies to client reply buffers. 4. Newly introduced ```_addBulkOffloadToBuffer``` and ```_addBulkOffloadToList``` are used for adding **copy avoid** replies to client reply buffers. #### Write-to-client infrastructure: The ```writevToClient``` and ```_postWriteToClient``` has been significantly changed to support copy avoidance capability. #### Debug configuration: 1. ``min-io-threads-avoid-copy-reply`` - Minimum number of IO threads for copy avoidance 2. ``min-string-size-avoid-copy-reply`` - Minimum bulk string size for copy avoidance when IO threads disabled 3. ``min-string-size-avoid-copy-reply-threaded`` - Minimum bulk string size for copy avoidance when IO threads enabled ### Testing 1. Existing unit and integration tests passed. Copy avoidance enabled on tests with ``--io-threads`` flag 2. Added unit tests for copy avoidance functionality ### Performance Tests Note: pay attention `io-threads 1` config means only main thread with no additional io-threads, `io-threads 2` means main thread plus 1 I/O thread, `io-threads 9` means main thread plus 8 I/O threads. #### 512 byte object size Tests are conducted on memory optimized instances using: * 3,000,000 keys * 512 bytes object size * 1000 clients |io-threads (including main thread) |Plain Reply |Copy Avoidance | |--- |--- |--- | |7 |1,160,000 |1,160,000 | |8 |1,150,000 |1,280,000 | |9 |1,150,000 |1,330,000 | |10 |N/A |1,380,000 | |11 |N/A |1,420,000 | #### Various object size, small number of threads |iothreads |Data size |Keys |Clients |Instance type |Unstable branch |Copy Avoidance On | |--- |--- |--- |--- |--- |--- |--- | |1 |512 byte |3,000,000 |1,000 |memory optimized |195,000 |195,000 | |2 |512 byte |3,000,000 |1,000 |memory optimized |245,000 |245,000 | |3 |512 byte |3,000,000 |1,000 |memory optimized |455,000 |459,000 | |4 |512 byte |3,000,000 |1,000 |memory optimized |685,000 |685,000 | | | | | | | | | |1 |1K |3,000,000 |1,000 |memory optimized |185,000 |185,000 | |2 |1K |3,000,000 |1,000 |memory optimized |235,000 |235,000 | |3 |1K |3,000,000 |1,000 |memory optimized |450,000 |450,000 | | | | | | | | | |1 |4K |1,000,000 |1,000 |network optimized |182,000 |187,000 | |2 |4K |1,000,000 |1,000 |network optimized |240,000 |238,000 | | | | | | | | | |1 |16K |1,000,000 |500 |network optimized |100,000 |120,000 | |2 |16K |1,000,000 |500 |network optimized |140,000 |140,000 | |3 |16K |1,000,000 |500 |network optimized |275,000 |260,000 | | | | | | | | | |1 |32K |500,000 |500 |network optimized |57,000 |90,000 | |2 |32K |500,000 |500 |network optimized |110,000 |110,000 | |3 |32K |500,000 |500 |network optimized |215,000 |215,000 | | | | | | | | | |1 |64K |100,000 |500 |network optimized |30,000 |57,000 | |2 |64K |100,000 |500 |network optimized |69,000 |61,000 | |3 |64K |100,000 |500 |network optimized |120,000 |120,000 | |4 |64K |100,000 |500 |network optimized |115,000 - 175,000 |175,000 | |5 |64K |100,000 |500 |network optimized |115,000 - 165,000 |230,000 | --------- 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: Madelyn Olson <[email protected]> Signed-off-by: chzhoo <[email protected]>
### Overview This PR introduces the ability to avoid copying the content of string object into replies (i.e. bulk string replies) and to allow I/O threads refer directly to obj->ptr in writev iov. ### 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 * Minor changes in cluster slots stats in order to support `network-bytes-out` for copy avoid replies * 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. Internal configuration ``min-io-threads-copy-avoid`` introduced to manage this number of threads **Note**: When copy avoidance disabled content and handling of client reply buffers remains as before this PR ### Implementation Details #### ``client`` and ``clientReplyBlock`` structs: 1. ``buf_encoded`` flag has been added to ``clientReplyBlock`` struct and to ``client`` struct for static ``c->buf`` to indicate if reply buffer is in copy avoidance mode (i.e. include headers and payloads) or not (i.e. plain replies only). 2. ``io_last_written_buf``, ``io_last_written_bufpos``, ``io_last_written_data_len`` fields added ``client`` struct to to keep track of write state between ``writevToClient`` invocations #### Reply construction: 1. Original ```_addReplyToBuffer``` and ```_addReplyProtoToList``` have been renamed to ```_addReplyPayloadToBuffer``` and ```_addReplyPayloadToList``` and extended to support different types of payloads - regular replies and copy avoid replies. 3. New ```_addReplyToBuffer``` and ```_addReplyProtoToList``` calls now ```_addReplyPayloadToBuffer``` and ```_addReplyPayloadToList``` and used for adding **regular** replies to client reply buffers. 4. Newly introduced ```_addBulkOffloadToBuffer``` and ```_addBulkOffloadToList``` are used for adding **copy avoid** replies to client reply buffers. #### Write-to-client infrastructure: The ```writevToClient``` and ```_postWriteToClient``` has been significantly changed to support copy avoidance capability. #### Debug configuration: 1. ``min-io-threads-avoid-copy-reply`` - Minimum number of IO threads for copy avoidance 2. ``min-string-size-avoid-copy-reply`` - Minimum bulk string size for copy avoidance when IO threads disabled 3. ``min-string-size-avoid-copy-reply-threaded`` - Minimum bulk string size for copy avoidance when IO threads enabled ### Testing 1. Existing unit and integration tests passed. Copy avoidance enabled on tests with ``--io-threads`` flag 2. Added unit tests for copy avoidance functionality ### Performance Tests Note: pay attention `io-threads 1` config means only main thread with no additional io-threads, `io-threads 2` means main thread plus 1 I/O thread, `io-threads 9` means main thread plus 8 I/O threads. #### 512 byte object size Tests are conducted on memory optimized instances using: * 3,000,000 keys * 512 bytes object size * 1000 clients |io-threads (including main thread) |Plain Reply |Copy Avoidance | |--- |--- |--- | |7 |1,160,000 |1,160,000 | |8 |1,150,000 |1,280,000 | |9 |1,150,000 |1,330,000 | |10 |N/A |1,380,000 | |11 |N/A |1,420,000 | #### Various object size, small number of threads |iothreads |Data size |Keys |Clients |Instance type |Unstable branch |Copy Avoidance On | |--- |--- |--- |--- |--- |--- |--- | |1 |512 byte |3,000,000 |1,000 |memory optimized |195,000 |195,000 | |2 |512 byte |3,000,000 |1,000 |memory optimized |245,000 |245,000 | |3 |512 byte |3,000,000 |1,000 |memory optimized |455,000 |459,000 | |4 |512 byte |3,000,000 |1,000 |memory optimized |685,000 |685,000 | | | | | | | | | |1 |1K |3,000,000 |1,000 |memory optimized |185,000 |185,000 | |2 |1K |3,000,000 |1,000 |memory optimized |235,000 |235,000 | |3 |1K |3,000,000 |1,000 |memory optimized |450,000 |450,000 | | | | | | | | | |1 |4K |1,000,000 |1,000 |network optimized |182,000 |187,000 | |2 |4K |1,000,000 |1,000 |network optimized |240,000 |238,000 | | | | | | | | | |1 |16K |1,000,000 |500 |network optimized |100,000 |120,000 | |2 |16K |1,000,000 |500 |network optimized |140,000 |140,000 | |3 |16K |1,000,000 |500 |network optimized |275,000 |260,000 | | | | | | | | | |1 |32K |500,000 |500 |network optimized |57,000 |90,000 | |2 |32K |500,000 |500 |network optimized |110,000 |110,000 | |3 |32K |500,000 |500 |network optimized |215,000 |215,000 | | | | | | | | | |1 |64K |100,000 |500 |network optimized |30,000 |57,000 | |2 |64K |100,000 |500 |network optimized |69,000 |61,000 | |3 |64K |100,000 |500 |network optimized |120,000 |120,000 | |4 |64K |100,000 |500 |network optimized |115,000 - 175,000 |175,000 | |5 |64K |100,000 |500 |network optimized |115,000 - 165,000 |230,000 | --------- 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: Madelyn Olson <[email protected]> Signed-off-by: shanwan1 <[email protected]>
…addRepliesWithOffloadsToList (#2201) Replaced direct cast to robj** with memcpy to avoid undefined behavior on unaligned buffers, which caused test failure when built with UBSan. Follow-up of #2078 Signed-off-by: xbasel <[email protected]>
Because valkey optimizes the reuse of client buffers, so the test will break. see valkey-io/valkey#2078
…addRepliesWithOffloadsToList (valkey-io#2201) Replaced direct cast to robj** with memcpy to avoid undefined behavior on unaligned buffers, which caused test failure when built with UBSan. Follow-up of valkey-io#2078 Signed-off-by: xbasel <[email protected]>
In valkey-io#2078, we did not report large reply when copy avoidance is allowed. This results in replies larger than 16384 not being recorded in the commandlog large-reply. This 16384 is controlled by the hidden config min-string-size-avoid-copy-reply. Signed-off-by: Binbin <[email protected]>
In #2078, we did not report large reply when copy avoidance is allowed. This results in replies larger than 16384 not being recorded in the commandlog large-reply. This 16384 is controlled by the hidden config min-string-size-avoid-copy-reply. Signed-off-by: Binbin <[email protected]>
…#2652) In valkey-io#2078, we did not report large reply when copy avoidance is allowed. This results in replies larger than 16384 not being recorded in the commandlog large-reply. This 16384 is controlled by the hidden config min-string-size-avoid-copy-reply. Signed-off-by: Binbin <[email protected]>
In #2078, we did not report large reply when copy avoidance is allowed. This results in replies larger than 16384 not being recorded in the commandlog large-reply. This 16384 is controlled by the hidden config min-string-size-avoid-copy-reply. Signed-off-by: Binbin <[email protected]>
Overview
This PR introduces the ability to avoid copying whole content of string object into replies (i.e. bulk string replies) and to allow I/O threads refer directly to obj->ptr in writev iov as described at #1353.
Key Changes
network-bytes-outfor copy avoid repliesmin-io-threads-copy-avoidintroduced to manage this number of threadsNote: When copy avoidance disabled content and handling of client reply buffers remains as before this PR
Implementation Details
clientandclientReplyBlockstructs:buf_encodedflag has been added toclientReplyBlockstruct and toclientstruct for staticc->bufto indicate if reply buffer is in copy avoidance mode (i.e. include headers and payloads) or not (i.e. plain replies only).io_last_written_buf,io_last_written_bufpos,io_last_written_data_lenfields addedclientstruct to to keep track of write state betweenwritevToClientinvocationsReply construction:
_addReplyToBufferand_addReplyProtoToListhave been renamed to_addReplyPayloadToBufferand_addReplyPayloadToListand extended to support different types of payloads - regular replies and copy avoid replies._addReplyToBufferand_addReplyProtoToListcalls now_addReplyPayloadToBufferand_addReplyPayloadToListand used for adding regular replies to client reply buffers._addBulkOffloadToBufferand_addBulkOffloadToListare used for adding copy avoid replies to client reply buffers.Write-to-client infrastructure:
The
writevToClientand_postWriteToClienthas been significantly changed to support copy avoidance capability.Internal configuration:
min-io-threads-avoid-copy-reply- Minimum number of IO threads for copy avoidancemin-string-size-avoid-copy-reply- Minimum bulk string size for copy avoidance when IO threads disabledmin-string-size-avoid-copy-reply-threaded- Minimum bulk string size for copy avoidance when IO threads enabledTesting
--io-threadsflagPerformance Tests
Note: pay attention
io-threads 1config means only main thread with no additional io-threads,io-threads 2means main thread plus 1 I/O thread,io-threads 9means main thread plus 8 I/O threads.512 byte object size
Tests are conducted on memory optimized instances using:
Various object size, small number of threads