Skip to content

Conversation

@fcostaoliveira
Copy link
Collaborator

@fcostaoliveira fcostaoliveira commented Jan 3, 2025

This update refactors prepareClientToWrite by introducing _prepareClientToWrite for inline checks within network.c file, and separates replica and non-replica handling for pending replies and writes (_clientHasPendingRepliesSlave/NonSlave and _writeToClientSlave/NonSlave).

prepareClientToWrites improvement

Function / Call Stack CPU Time: Difference CPU Time: unstable (04f63d4) CPU Time: This PR (0584dfe)
prepareClientToWrite 2.40% 3.55% 1.15%

splitting replica and non-replica handling improvement

TBD

To test it we can simply:

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

Confirmation from daily:

benchmark results

Achievable ops/sec impact:

test name unstable (04f63d4) This PR first commit (22b75ff) % change This PR last commit (145de69) % change
memtier_benchmark-1key-list-1K-elements-lrange-all-elements 14746 15253 3.4% 15607 5.8%
memtier_benchmark-1key-list-100-elements-lrange-all-elements 82602 80676 -2.3% 85011 2.9%
memtier_benchmark-1key-list-100-elements-int-7bit-uint-lrange-all-elements-pipeline-10 136793 139121 1.7% 148916 8.9%
memtier_benchmark-1key-list-100-elements-lrange-all-elements-pipeline-10 137138 135291 -1.3% 141980 3.5%
memtier_benchmark-1key-list-1K-elements-lrange-all-elements-pipeline-10 13975 14256 2.0% 14798 5.9%
memtier_benchmark-1key-list-2K-elements-quicklist-lrange-all-elements-longs 5996 6239 4.1% 6330 5.6%
memtier_benchmark-1key-list-10-elements-lrange-all-elements-pipeline-10 518595 522612 0.8% 536865 3.5%
memtier_benchmark-1key-list-100-elements-int-lrange-all-elements-pipeline-10 109166 110532 1.3% 113514 4.0%
memtier_benchmark-1key-list-10-elements-lrange-all-elements 145262 143343 -1.3% 144856 -0.3%
memtier_benchmark-1key-hash-1K-fields-hgetall 7407.93 7821.4 5.6% 8025.5 8.3%
memtier_benchmark-1key-hash-1K-fields-hgetall-pipeline-10 7156.6 7409.02 3.5% 7650.24 6.9%

To reproduce:

taskset -c 0 /root/redis/src/redis-server  --unixsocket /tmp/1.socket --save '' --enable-debug-command local
pip3 install redis-benchmarks-specification
redis-benchmarks-spec-client-runner --tests-regexp ".*lrange.*|memtier_benchmark-1key-hash-1K-fields-hgetall.*" --flushall_on_every_test_start --flushall_on_every_test_end  --cpuset_start_pos 2 --override-memtier-test-time 60

@fcostaoliveira fcostaoliveira requested a review from sundb January 3, 2025 18:20
@fcostaoliveira fcostaoliveira added action:run-benchmark Triggers the benchmark suite for this Pull Request and removed action:run-benchmark Triggers the benchmark suite for this Pull Request labels Jan 3, 2025
@fcostaoliveira fcostaoliveira force-pushed the optimize.prepareClientToWrite branch from e9cdef9 to c6fd636 Compare January 5, 2025 10:25
@fcostaoliveira fcostaoliveira force-pushed the optimize.prepareClientToWrite branch from c6fd636 to de49159 Compare January 5, 2025 10:26
…erbose the usage of client slave and monitor use-cases.
@fcostaoliveira fcostaoliveira requested a review from sundb January 7, 2025 12:10
@fcostaoliveira fcostaoliveira requested a review from sundb January 8, 2025 11:30
@fcostaoliveira fcostaoliveira requested a review from sundb January 8, 2025 11:58
sundb
sundb previously approved these changes Jan 8, 2025
@sundb
Copy link
Collaborator

sundb commented Jan 8, 2025

@fcostaoliveira
Copy link
Collaborator Author

fcostaoliveira commented Jan 10, 2025

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

This comment was automatically generated given a benchmark was triggered.

Started benchmark suite at 2025-01-10 19:31:08.441028 and took 8152.022405 seconds to finish.
Status: [################################################################################] 100.0% completed.

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

@fcostaoliveira
Copy link
Collaborator Author

fcostaoliveira commented Jan 10, 2025

Automated performance analysis summary

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

Using platform named: intel64-ubuntu22.04-redis-clx1 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 filipecosta90:optimize.prepareClientToWrite.

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

Improvements Table

Test Case Baseline redis/redis unstable (median obs. +- std.dev) Comparison redis/redis filipecosta90:optimize.prepareClientToWrite (median obs. +- std.dev) % change (higher-better) Note
memtier_benchmark-10Kkeys-load-hash-50-fields-with-10000B-values 1084 1823 68.2% IMPROVEMENT
memtier_benchmark-1Mkeys-generic-scan-pipeline-10 249738 322217 29.0% IMPROVEMENT
memtier_benchmark-1Mkeys-load-stream-5-fields-with-100B-values-pipeline-10 95311 139362 46.2% IMPROVEMENT
memtier_benchmark-2keys-zset-300-elements-skiplist-encoded-zunion 1950 2506 28.5% IMPROVEMENT

Improvements test regexp names: memtier_benchmark-10Kkeys-load-hash-50-fields-with-10000B-values|memtier_benchmark-1Mkeys-generic-scan-pipeline-10|memtier_benchmark-1Mkeys-load-stream-5-fields-with-100B-values-pipeline-10|memtier_benchmark-2keys-zset-300-elements-skiplist-encoded-zunion

Full Results table:
Test Case Baseline redis/redis unstable (median obs. +- std.dev) Comparison redis/redis filipecosta90:optimize.prepareClientToWrite (median obs. +- std.dev) % change (higher-better) Note
memtier_benchmark-10Kkeys-load-hash-50-fields-with-10000B-values 1084 1823 68.2% IMPROVEMENT
memtier_benchmark-1Mkeys-generic-scan-pipeline-10 249738 322217 29.0% IMPROVEMENT
memtier_benchmark-1Mkeys-load-stream-5-fields-with-100B-values-pipeline-10 95311 139362 46.2% IMPROVEMENT
memtier_benchmark-2keys-zset-300-elements-skiplist-encoded-zunion 1950 2506 28.5% IMPROVEMENT

@ShooterIT
Copy link
Member

Hi @fcostaoliveira and do you know if __attribute__((always_inline)) can work when we add it at the beginning of a normal function?

Co-authored-by: Yuan Wang <[email protected]>
@ShooterIT
Copy link
Member

ShooterIT commented Jan 13, 2025

I tried __attribute__((always_inline)) int prepareClientToWrite(client *c), gcc in ubuntu reports a warning networking.c:286:36: warning: 'always_inline' function might not be inlinable [-Wattributes] , but clang in mac didn't.
it seems _attribute__((always_inline)) is recommended to be used together with inline. I initially want to just use __attribute__((always_inline)) int prepareClientToWrite(client *c) to avoid changing much code and dirtying blame log, but it doesn't work, so we can leave as it is

@sundb sundb merged commit dc0ee51 into redis:unstable Jan 13, 2025
17 checks passed
@sundb sundb added this to Redis 8.0 Aug 15, 2025
@sundb sundb moved this from Todo to Done in Redis 8.0 Aug 15, 2025
funny-dog pushed a commit to funny-dog/redis that referenced this pull request Sep 17, 2025
This update refactors prepareClientToWrite by introducing
_prepareClientToWrite for inline checks within network.c file, and
separates replica and non-replica handling for pending replies and
writes (_clientHasPendingRepliesSlave/NonSlave and
_writeToClientSlave/NonSlave).

---------

Co-authored-by: debing.sun <[email protected]>
Co-authored-by: Yuan Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants