perf: Avoid heap copy of status/integer single-line replies#2860
Open
iliaal wants to merge 1 commit into
Open
Conversation
redis_boolean_response and redis_long_response read the reply with redis_sock_read, which estrndup's the +OK / :N line onto the heap, then inspect only the first byte (or parse the integer) and efree it. Every SET, INCR, EXPIRE, SADD, DEL, EXISTS and similar command paid a wasted allocation. Extract the shared dispatch into redis_sock_read_into, which writes a single-line reply into a caller-provided buffer (borrowed, no heap) and only heap-allocates for bulk bodies. redis_sock_read becomes a thin wrapper that estrndup's the borrowed line so its existing callers are unaffected. The two handlers now read into a stack buffer and only free when the reply was an (unexpected) bulk, which the shared reader still drains so the stream stays in sync. Verified on PHP 8.4: status/integer responses (incr/decr/zero/negative, near INT64_MAX, +OK booleans, :1/:0, error replies via the '-' path), bulk path unchanged through the wrapper, no leaks under report_memleaks, and testSetGet/testIncr/testExpire/testExists/testRename/testDel/ testHashes/testKeys.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
redis_boolean_responseandredis_long_responseread replies withredis_sock_read, whichestrndups the+OK/:Nline onto the heap only to inspect one byte (or parse the int) and free it. That wasted an allocation on everySET/INCR/EXPIRE/SADD/DEL/EXISTS. This extracts the dispatch intoredis_sock_read_into, which borrows the single-line reply into a caller buffer (no heap) and only allocates for bulk bodies.redis_sock_readstays a thin wrapper, so its other callers are unaffected, and the handlers still drain an unexpected bulk to keep the stream in sync.Verified on PHP 8.4 across integer, boolean, and error responses, with the bulk path unchanged through the wrapper: no leaks, 8 project suite methods pass. The ownership contract was independently reviewed.