Created specific SMEMBERS command logic which avoids sinterGenericCommand, and minimizes processing and memory overhead #13499

fcostaoliveira · 2024-08-28T15:04:12Z

This PR introduces a dedicated implementation for the SMEMBERS command that avoids using the more generalized sinterGenericCommand function.
By tailoring the logic specifically for SMEMBERS, we reduce unnecessary processing and memory overheads that were previously incurred by handling more complex cases like set intersections.

To test it we can simply focus on the set.tcl

tclsh tests/test_helper.tcl --single unit/type/set

Manual check of performance impact

redis-benchmarks-spec-client-runner --tests-regexp "memtier_benchmark-1key-set-1K-elements-smembers|memtier_benchmark-1key-set-10-elements-smembers|memtier_benchmark-1key-set-100-elements-smembers|memtier_benchmark-1key-set-10-elements-smembers-pipeline-10" --flushall_on_every_test_end  --flushall_on_every_test_start --override-memtier-test-time 60 --cpuset_start_pos 1

Impact in throughput (ALL STATS.Totals."Ops/sec")

Test Name	unstable (`3b1b1d1`)	optimize.smembers branch (`e2c449f`)	% change
memtier_benchmark-1key-set-100-elements-smembers	62250	79444	27.6%
memtier_benchmark-1key-set-1K-elements-smembers	11387	14251	25.2%
memtier_benchmark-1key-set-10-elements-smembers	112160	135476	20.8%
memtier_benchmark-1key-set-10-elements-smembers-pipeline-10	400503	578594	44.5%

Impact in p50 latency (ALL STATS.Totals."Percentile Latencies"."p50.00")

Test Name	unstable (`3b1b1d1`)	optimize.smembers branch (`e2c449f`)	% change
memtier_benchmark-1key-set-100-elements-smembers	3.087	2.399	22.3%
memtier_benchmark-1key-set-1K-elements-smembers	17.663	14.015	20.7%
memtier_benchmark-1key-set-10-elements-smembers	1.719	1.455	15.4%
memtier_benchmark-1key-set-10-elements-smembers-pipeline-10	4.831	3.039	37.1%

…mand logic, which minimizes processing and memory overheads

src/t_set.c

Co-authored-by: debing.sun <[email protected]>

fcostaoliveira · 2024-08-29T02:21:59Z

CE Performance Automation : step 1 of 2 (build) DONE.

This comment was automatically generated given a benchmark was triggered.
Started building at 2024-09-05 05:40:34.430889 and took 87 seconds.
You can check each build/benchmark progress in grafana:

git hash: 979be36
git branch: filipecosta90:optimize.smembers
commit date and time: n/a
commit summary: n/a
test filters:
- command priority lower limit: 0
- command priority upper limit: 10000
- test name regex: .*
- command group regex: .*

You can check a comparison in detail via the grafana link

fcostaoliveira · 2024-08-29T02:23:25Z

CE Performance Automation : step 2 of 2 (benchmark) FINISHED.

This comment was automatically generated given a benchmark was triggered.

Started benchmark suite at 2024-10-20 17:50:25.340919 and took 5251.823101 seconds to finish.
Status: [################################################################################] 100.0% completed.

In total will run 135 benchmarks.
- 0 pending.
- 135 completed:
- 0 successful.
- 135 failed.
You can check a the status in detail via the grafana link

fcostaoliveira · 2024-08-30T16:44:12Z

Automated performance analysis summary

This comment was automatically generated given there is performance data available.

Using platform named: intel64-ubuntu22.04-redis-icx1 to do the comparison.

In summary:

Detected a total of 4 improvements above the improvement water line.

You can check a comparison in detail via the grafana link

Comparison between unstable and optimize.smembers.

Time Period from 5 months ago. (environment used: oss-standalone)

Improvements Table

Test Case	Baseline unstable (median obs. +- std.dev)	Comparison optimize.smembers (median obs. +- std.dev)	% change (higher-better)	Note
memtier_benchmark-1key-set-10-elements-smembers	140572 +- 1.9% (6 datapoints)	173955	23.7%	IMPROVEMENT
memtier_benchmark-1key-set-10-elements-smembers-pipeline-10	459961 +- 1.8% (7 datapoints)	654388	42.3%	IMPROVEMENT
memtier_benchmark-1key-set-100-elements-smembers	82277 +- 8.5% (7 datapoints)	96814	17.7%	waterline=8.5%. IMPROVEMENT
memtier_benchmark-1key-set-1K-elements-smembers	13542 +- 1.7% (7 datapoints)	15312	13.1%	IMPROVEMENT

Improvements test regexp names: memtier_benchmark-1key-set-10-elements-smembers|memtier_benchmark-1key-set-10-elements-smembers-pipeline-10|memtier_benchmark-1key-set-100-elements-smembers|memtier_benchmark-1key-set-1K-elements-smembers

Full Results table:

Test Case	Baseline unstable (median obs. +- std.dev)	Comparison optimize.smembers (median obs. +- std.dev)	% change (higher-better)	Note
memtier_benchmark-1key-set-10-elements-smembers	140572 +- 1.9% (6 datapoints)	173955	23.7%	IMPROVEMENT
memtier_benchmark-1key-set-10-elements-smembers-pipeline-10	459961 +- 1.8% (7 datapoints)	654388	42.3%	IMPROVEMENT
memtier_benchmark-1key-set-100-elements-smembers	82277 +- 8.5% (7 datapoints)	96814	17.7%	waterline=8.5%. IMPROVEMENT
memtier_benchmark-1key-set-1K-elements-smembers	13542 +- 1.7% (7 datapoints)	15312	13.1%	IMPROVEMENT

sundb · 2024-09-04T01:28:20Z

src/t_set.c

+    }
+
+    /* Prepare the response. */
+    addReplySetLen(c,setTypeSize(setobj));


If the data is corrupted, setTypeSize(setobj) may not equal the number of replies in the following loop.
we can choose one of the following:

use addReplyDeferredLen().

add a counter and add a assertion in the end.

fail CI: https://github.com/redis/redis/actions/runs/10692581371
@fcostaoliveira WDYT

Fixed in #13515

the second one is faster than the first.

After #13499, If the length set by `addReplySetLen()` does not match the actual number of elements in the reply, it will cause protocol broken and result in the client hanging.

### New Features in binary distributions - 7 new data structures: JSON, Time series, Bloom filter, Cuckoo filter, Count-min sketch, Top-k, t-digest - Redis scalable query engine (including vector search) ### Potentially breaking changes - #12272 `GETRANGE` returns an empty bulk when the negative end index is out of range - #12395 Optimize `SCAN` command when matching data type ### Bug fixes - #13510 Fix `RM_RdbLoad` to enable AOF after RDB loading is completed - #13489 `ACL CAT` - return module commands - #13476 Fix a race condition in the `cache_memory` of `functionsLibCtx` - #13473 Fix incorrect lag due to trimming stream via `XTRIM` command - #13338 Fix incorrect lag field in `XINFO` when tombstone is after the `last_id` of the consume group - #13470 On `HDEL` of last field - update the global hash field expiration data structure - #13465 Cluster: Pass extensions to node if extension processing is handled by it - #13443 Cluster: Ensure validity of myself when loading cluster config - #13422 Cluster: Fix `CLUSTER SHARDS` command returns empty array ### Modules API - #13509 New API calls: `RM_DefragAllocRaw`, `RM_DefragFreeRaw`, and `RM_RegisterDefragCallbacks` - defrag API to allocate and free raw memory ### Performance and resource utilization improvements - #13503 Avoid overhead of comparison function pointer calls in listpack `lpFind` - #13505 Optimize `STRING` datatype write commands - #13499 Optimize `SMEMBERS` command - #13494 Optimize `GEO*` commands reply - #13490 Optimize `HELLO` command - #13488 Optimize client query buffer - #12395 Optimize `SCAN` command when matching data type - #13529 Optimize `LREM`, `LPOS`, `LINSERT`, and `LINDEX` commands - #13516 Optimize `LRANGE` and other commands that perform several writes to client buffers per call - #13431 Avoid `used_memory` contention when updating from multiple threads ### Other general improvements - #13495 Reply `-LOADING` on replica while flushing the db ### CLI tools - #13411 redis-cli: Fix wrong `dbnum` showed after the client reconnected ### Notes - No backward compatibility for replication or persistence. - Additional distributions, upgrade paths, features, and improvements will be introduced in upcoming pre-releases. - With the GA release of 8.0 we will deprecate Redis Stack.

fcostaoliveira added 2 commits August 28, 2024 16:03

Created specific SMEMBERS command logic which avoids sinterGenericCom…

6810dc7

…mand logic, which minimizes processing and memory overheads

Fixed key check and set existance order

e2c449f

fcostaoliveira changed the title ~~Created specific SMEMBERS command logic which avoids sinterGenericCommand, which minimizes processing and memory overheads~~ Created specific SMEMBERS command logic which avoids sinterGenericCommand, and minimizes processing and memory overhead Aug 28, 2024

sundb reviewed Aug 29, 2024

View reviewed changes

src/t_set.c Outdated Show resolved Hide resolved

src/t_set.c Outdated Show resolved Hide resolved

fcostaoliveira and others added 3 commits August 29, 2024 03:09

Update src/t_set.c

ce8e4b0

Co-authored-by: debing.sun <[email protected]>

Fixes per PR review

0def2d6

Removed unused variable on smembersCommand after suggestion

979be36

fcostaoliveira requested a review from sundb August 29, 2024 02:17

fcostaoliveira added the action:run-benchmark Triggers the benchmark suite for this Pull Request label Aug 29, 2024

sundb approved these changes Aug 29, 2024

View reviewed changes

fcostaoliveira requested a review from oranagra September 2, 2024 12:43

sundb approved these changes Sep 3, 2024

View reviewed changes

sundb merged commit 00a8e72 into redis:unstable Sep 3, 2024
15 checks passed

sundb reviewed Sep 4, 2024

View reviewed changes

sundb mentioned this pull request Sep 4, 2024

Fix set with invalid length causes smembers to hang #13515

Merged

YaacovHazan added the release-notes indication that this issue needs to be mentioned in the release notes label Sep 5, 2024

YaacovHazan mentioned this pull request Sep 11, 2024

Redis 8.0 M01 #13538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created specific SMEMBERS command logic which avoids sinterGenericCommand, and minimizes processing and memory overhead #13499

Created specific SMEMBERS command logic which avoids sinterGenericCommand, and minimizes processing and memory overhead #13499

fcostaoliveira commented Aug 28, 2024 •

edited

Loading

fcostaoliveira commented Aug 29, 2024 •

edited

Loading

fcostaoliveira commented Aug 29, 2024 •

edited

Loading

fcostaoliveira commented Aug 30, 2024

sundb Sep 4, 2024 •

edited

Loading

sundb Sep 4, 2024

sundb Sep 4, 2024

Created specific SMEMBERS command logic which avoids sinterGenericCommand, and minimizes processing and memory overhead #13499

Created specific SMEMBERS command logic which avoids sinterGenericCommand, and minimizes processing and memory overhead #13499

Conversation

fcostaoliveira commented Aug 28, 2024 • edited Loading

Manual check of performance impact

Impact in throughput (ALL STATS.Totals."Ops/sec")

Impact in p50 latency (ALL STATS.Totals."Percentile Latencies"."p50.00")

fcostaoliveira commented Aug 29, 2024 • edited Loading

CE Performance Automation : step 1 of 2 (build) DONE.

fcostaoliveira commented Aug 29, 2024 • edited Loading

CE Performance Automation : step 2 of 2 (benchmark) FINISHED.

fcostaoliveira commented Aug 30, 2024

Automated performance analysis summary

Comparison between unstable and optimize.smembers.

Improvements Table

sundb Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

sundb Sep 4, 2024

Choose a reason for hiding this comment

sundb Sep 4, 2024

Choose a reason for hiding this comment

fcostaoliveira commented Aug 28, 2024 •

edited

Loading

fcostaoliveira commented Aug 29, 2024 •

edited

Loading

fcostaoliveira commented Aug 29, 2024 •

edited

Loading

sundb Sep 4, 2024 •

edited

Loading