Embed keys and hash fields as SDS type 5 by zuiderkwast · Pull Request #1613 · valkey-io/valkey

zuiderkwast · 2025-01-24T19:02:35Z

Until now, these embedded strings were SDS type 8, because they were copied as-is from EMBSTR encoded string objects, which always use SDS type 8. This change allows them to be embedded as SDS type 5, saving two bytes.

The implementation of embeddeding SDS strings in other structures is refactored. sdswrite() is a new function to create an SDS representation into a buffer provided by the caller. sdscopytobuffer(), which just copied the layout of an existing sds, is deleted.

The DEBUG SDSLEN command output is changed slightly, to account for correct allocation sizes of embedded strings and not report the same memory twice.

Fixes #1567

Until now, these embedded strings were SDS type 8, becuase they were copied as-is from EMBSTR encoded string objects, which always use SDS type 8. This change allows them to be embedded as SDS type 5, saving two bytes. The implementation of embeddeding SDS strings in other structures is refactored. sdswrite() is a new function to create an SDS representation into a buffer provided by the caller, instead of allocating it. sdscopytobuffer() which just copied the layout of an existing sds is deleted. The DEBUG SDSLEN command output is changed slightly, to account for correct allocation sizes for embedded strings and not report the same memory twice. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

codecov · 2025-01-27T18:47:33Z

Codecov Report

Attention: Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.03%. Comparing base (e9b8970) to head (4d33538).
Report is 18 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/sds.c	94.73%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1613      +/-   ##
============================================
- Coverage     71.04%   71.03%   -0.02%     
============================================
  Files           121      121              
  Lines         65174    65243      +69     
============================================
+ Hits          46304    46343      +39     
- Misses        18870    18900      +30

Files with missing lines	Coverage Δ
src/debug.c	`54.14% <100.00%> (+1.73%)`	⬆️
src/object.c	`82.17% <100.00%> (+0.13%)`	⬆️
src/sds.h	`80.30% <100.00%> (+1.27%)`	⬆️
src/server.c	`87.70% <ø> (+0.08%)`	⬆️
src/t_hash.c	`96.30% <100.00%> (+0.01%)`	⬆️
src/sds.c	`86.67% <94.73%> (-0.18%)`	⬇️

... and 27 files with indirect coverage changes

ranshid

(Sorry it took me time to get to it, was very sick for a long period of time)

Overall LGTM!

Some small comments and/or questions.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Fixes the following test case which fails in builds with allocators that don't have usable size: ``` *** [err]: Memory usage of embedded string value in tests/unit/type/string.tcl Expected '40' to be less than or equal to '32' (context: type eval line 5 cmd {assert_lessthan_equal [r memory usage quux] 32} proc ::test) ``` With allocators like libc malloc that doesn't have usable size, zmalloc stores an extra size field in the allocation. This makes the memory usage 8 bytes larger than we expected in this test case. We can just skip this check. It's not the main thing tested in this test case. The failure was introduced in #1613. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Until now, these embedded strings were SDS type 8, because they were copied as-is from EMBSTR encoded string objects, which always use SDS type 8. This change allows them to be embedded as SDS type 5, saving two bytes. The implementation of embeddeding SDS strings in other structures is refactored. `sdswrite()` is a new function to create an SDS representation into a buffer provided by the caller. `sdscopytobuffer()`, which just copied the layout of an existing sds, is deleted. The DEBUG SDSLEN command output is changed slightly, to account for correct allocation sizes of embedded strings and not report the same memory twice. Fixes valkey-io#1567 --------- Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Fixes the following test case which fails in builds with allocators that don't have usable size: ``` *** [err]: Memory usage of embedded string value in tests/unit/type/string.tcl Expected '40' to be less than or equal to '32' (context: type eval line 5 cmd {assert_lessthan_equal [r memory usage quux] 32} proc ::test) ``` With allocators like libc malloc that doesn't have usable size, zmalloc stores an extra size field in the allocation. This makes the memory usage 8 bytes larger than we expected in this test case. We can just skip this check. It's not the main thing tested in this test case. The failure was introduced in valkey-io#1613. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Until now, these embedded strings were SDS type 8, because they were copied as-is from EMBSTR encoded string objects, which always use SDS type 8. This change allows them to be embedded as SDS type 5, saving two bytes. The implementation of embeddeding SDS strings in other structures is refactored. `sdswrite()` is a new function to create an SDS representation into a buffer provided by the caller. `sdscopytobuffer()`, which just copied the layout of an existing sds, is deleted. The DEBUG SDSLEN command output is changed slightly, to account for correct allocation sizes of embedded strings and not report the same memory twice. Fixes valkey-io#1567 --------- Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Fixes the following test case which fails in builds with allocators that don't have usable size: ``` *** [err]: Memory usage of embedded string value in tests/unit/type/string.tcl Expected '40' to be less than or equal to '32' (context: type eval line 5 cmd {assert_lessthan_equal [r memory usage quux] 32} proc ::test) ``` With allocators like libc malloc that doesn't have usable size, zmalloc stores an extra size field in the allocation. This makes the memory usage 8 bytes larger than we expected in this test case. We can just skip this check. It's not the main thing tested in this test case. The failure was introduced in valkey-io#1613. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Until now, these embedded strings were SDS type 8, because they were copied as-is from EMBSTR encoded string objects, which always use SDS type 8. This change allows them to be embedded as SDS type 5, saving two bytes. The implementation of embeddeding SDS strings in other structures is refactored. `sdswrite()` is a new function to create an SDS representation into a buffer provided by the caller. `sdscopytobuffer()`, which just copied the layout of an existing sds, is deleted. The DEBUG SDSLEN command output is changed slightly, to account for correct allocation sizes of embedded strings and not report the same memory twice. Fixes valkey-io#1567 --------- Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Fixes the following test case which fails in builds with allocators that don't have usable size: ``` *** [err]: Memory usage of embedded string value in tests/unit/type/string.tcl Expected '40' to be less than or equal to '32' (context: type eval line 5 cmd {assert_lessthan_equal [r memory usage quux] 32} proc ::test) ``` With allocators like libc malloc that doesn't have usable size, zmalloc stores an extra size field in the allocation. This makes the memory usage 8 bytes larger than we expected in this test case. We can just skip this check. It's not the main thing tested in this test case. The failure was introduced in valkey-io#1613. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

zuiderkwast requested a review from ranshid January 24, 2025 19:02

zuiderkwast changed the title ~~Embed keys in serverObject and fields in hash entry as SDS type 5~~ Embed keys and hash fields as SDS type 5 Jan 24, 2025

zuiderkwast marked this pull request as draft January 24, 2025 19:04

zuiderkwast added 4 commits January 24, 2025 23:43

Fix something

4ba9334

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Fix SDS_NOINIT thing

3d1a674

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Adjust adjustTypeIfNeeded if needed

6ba6ea5

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Give up on storing unused allocation space in embedded SDS headers

8f2f234

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Merge remote-tracking branch 'valkey/unstable' into embed-sds5

1e6c565

zuiderkwast marked this pull request as ready for review January 27, 2025 21:24

ranshid reviewed Feb 5, 2025

View reviewed changes

Comment thread src/object.c Outdated

Comment thread src/object.c

Comment thread src/debug.c Outdated

Comment thread tests/unit/type/string.tcl Outdated

zuiderkwast added 2 commits February 5, 2025 11:25

Address review comments

e869598

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Missed some occurrences of has_embkey

4d33538

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

ranshid approved these changes Feb 5, 2025

View reviewed changes

zuiderkwast merged commit 8d8ce19 into valkey-io:unstable Feb 5, 2025

zuiderkwast deleted the embed-sds5 branch February 5, 2025 12:15

zuiderkwast mentioned this pull request Feb 5, 2025

Fix memory usage check in test for embedded objects #1675

Merged

zvi-code mentioned this pull request Dec 24, 2025

[BUG] Crash in createEmbeddedStringObjectWithKeyAndExpire (when calling sdswrite) - while running FT.SEARCH #2968

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Embed keys and hash fields as SDS type 5#1613

Embed keys and hash fields as SDS type 5#1613
zuiderkwast merged 8 commits into
valkey-io:unstablefrom
zuiderkwast:embed-sds5

zuiderkwast commented Jan 24, 2025 •

edited

Loading

Uh oh!

codecov Bot commented Jan 27, 2025 •

edited

Loading

Uh oh!

ranshid left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

zuiderkwast commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zuiderkwast commented Jan 24, 2025 •

edited

Loading

codecov Bot commented Jan 27, 2025 •

edited

Loading