perf: Frame prefixed keys directly instead of via a temporary copy - #2862
Open
iliaal wants to merge 1 commit into
Open
perf: Frame prefixed keys directly instead of via a temporary copy#2862iliaal wants to merge 1 commit into
iliaal wants to merge 1 commit into
Conversation
When OPT_PREFIX is set, redis_cmd_append_sstr_key built the prefixed key with redis_key_prefix (emalloc + two memcpy), used it, appended it into the command smart_string (a third copy), then freed it. Outside cluster mode the contiguous prefixed key is only needed for that append, so frame the prefix and key as a single RESP bulk argument directly into the command buffer via a new two-segment helper, dropping the temporary allocation and one full-key copy per key. This runs for every key in GET/SET/H*/MGET/INCR/etc. whenever a prefix is configured. The cluster path (slot != NULL) is unchanged: it still builds the contiguous key because cluster_hash_key must see prefix and key together (a hash tag can span the boundary). The spprintf 'k' case, which duplicated this logic, now delegates to redis_cmd_append_sstr_key and so picks up the same fast path; 'K' already delegated. Verified on PHP 8.4: on-wire key equals prefix+key across SET/GET, HSET, RPUSH, SADD, ZADD, MSET/MGET, EXISTS/DEL multi-key, EXPIRE, RENAME (both keys), binary and empty keys, serializer+prefix, and the no-prefix regression path; no leaks under report_memleaks; testPrefix/testSetGet/ testKeys/testHashes/testMSetNX/testRename/testMulti pass.
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
1 participant
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.
When
OPT_PREFIXis set,redis_cmd_append_sstr_keybuilt a temporary prefixed key (emalloc+ 2memcpy), appended it into the command buffer (a third copy), then freed it. Outside cluster mode, this frames the prefix and key as one RESP bulk argument directly into the buffer instead, dropping the temporary allocation and a copy for every key. The cluster path (slot != NULL) keeps the contiguous key, sincecluster_hash_keymust see prefix and key together (a hash tag can straddle the boundary). The spprintf'k'case now delegates to the same helper.Verified on PHP 8.4: on-wire key equals
prefix+keyacross string/binary/empty keys, multi-key commands, RENAME, serializer+prefix, and the no-prefix regression path;testPrefixand 6 other suite methods pass; no leaks. Byte-equivalence and cluster-path safety also reviewed independently.