Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: optimizing memory overhead of copy operation in ConcurrentReadTxn #16508

Merged
merged 1 commit into from
Sep 9, 2023

Conversation

new-dream
Copy link
Contributor

…dTxn

Signed-off-by: new-dream <111836360+new-dream@users.noreply.github.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Could you also run the benchmark test [e.g. benchmark ] to help us understand the rough performance gain (including also memory usage) with this change?

@new-dream
Copy link
Contributor Author

OK, I'm happy to do performance tests, but I'm almost new here, so do you have any suggestions for performance tests?

  1. Which use cases should I test,
  2. Which tools to display the memory status after the performance test
  3. Is there any guidance to help me complete this

@ahrtr
Copy link
Member

ahrtr commented Aug 31, 2023

Please refer to #14394 (comment), please also pay attention to the memory usage during the test, and also the metrics such as process_resident_memory_bytes.

FYI, there is also a on-going discussion #16467

@fuweid
Copy link
Contributor

fuweid commented Aug 31, 2023

@new-dream I think you can use https://github.com/etcd-io/etcd/tree/main/tools/rw-heatmaps tools here. With patch, it can reduce memory overhead for read txn when there are multiple updates for same key. The benchmark txn-mixed can help us to simulate the read and write txn to verify this change. hope it can help

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@new-dream
Copy link
Contributor Author

thanks @ahrtr @fuweid , I'll do it these days.

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM - Thanks @new-dream.

One minor question below because I haven't looked at this piece of code much before.

server/storage/backend/tx_buffer.go Show resolved Hide resolved
@new-dream
Copy link
Contributor Author

Performance comparison

Environment: WSL2

MemTotal: 15896MB
12CPU, and each with Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz

Command:

#!/bin/bash

set -e

TEST_NAME=$1

ETCD_LOG=./logs/etcd-$TEST_NAME.log
BENCHMARK_LOG=./logs/benchmark-$TEST_NAME.log
MEMORY_LOG=./logs/memory-$TEST_NAME.log


mkdir -p logs
rm -rf default.etcd/
rm -rf $ETCD_LOG $BENCHMARK_LOG $MEMORY_LOG

echo "starting etcd..."
./bin/etcd --quota-backend-bytes=21474836480 --log-level error --listen-client-urls http://0.0.0.0:23790 --advertise-client-urls http://127.0.0.1:23790 &>$ETCD_LOG &
sleep 5s

echo "starting benchmark..."
./bin/tools/benchmark txn-mixed '' --conns=32 --clients=32 --total=200000 --endpoints http://127.0.0.1:23790 --rw-ratio 4 --key-space-size=4000000000 --key-size 64 --val-size 1024 &>$BENCHMARK_LOG &

while :
do
        if ! pgrep -x "benchmark" > /dev/null
        then
                echo "benchmark stopped"
                break
        fi
        curl -s  http://127.0.0.1:23790/metrics | grep -E "^process_resident_memory_bytes" >> $MEMORY_LOG
        sleep 1s
done

echo "kill etcd"
kill `pgrep etcd`


echo "Memory Usage:"
cat $MEMORY_LOG | awk '{printf("%f\n",$2)}' | awk '{sum+=$1} END {print sum/NR}' | awk '{printf("Average = %f\n",$0)}'
cat $MEMORY_LOG | awk '{printf("%f\n",$2)}' | awk 'BEGIN {max=0} {if ($1>max) max=$1} END {print "Max =",max}'

Result on one-server cluster

I tried 10 times in every branch

Benchmark result(the last of 10 times) on main branch, commit 506e9fdde6b65b0396f35e22186b07f1783fb853

Total Read Ops: 160101
Details:
Summary:
  Total:        249.9205 secs.
  Slowest:      0.1536 secs.
  Fastest:      0.0005 secs.
  Average:      0.0462 secs.
  Stddev:       0.0174 secs.
  Requests/sec: 640.6078

Response time histogram:
  0.0005 [1]    |
  0.0158 [2588] |∎
  0.0311 [26018]        |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0464 [62068]        |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0617 [42058]        |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0770 [18356]        |∎∎∎∎∎∎∎∎∎∎∎
  0.0924 [6531] |∎∎∎∎
  0.1077 [1890] |∎
  0.1230 [457]  |
  0.1383 [114]  |
  0.1536 [20]   |

Latency distribution:
  10% in 0.0270 secs.
  25% in 0.0341 secs.
  50% in 0.0437 secs.
  75% in 0.0560 secs.
  90% in 0.0694 secs.
  95% in 0.0784 secs.
  99% in 0.0971 secs.
  99.9% in 0.1214 secs.

Total Write Ops: 39899
Details:
Summary:
  Total:        249.9206 secs.
  Slowest:      0.0913 secs.
  Fastest:      0.0015 secs.
  Average:      0.0149 secs.
  Stddev:       0.0099 secs.
  Requests/sec: 159.6467

Response time histogram:
  0.0015 [1]    |
  0.0105 [16371]        |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0195 [12930]        |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0285 [6669] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0374 [2675] |∎∎∎∎∎∎
  0.0464 [868]  |∎∎
  0.0554 [265]  |
  0.0644 [87]   |
  0.0734 [28]   |
  0.0823 [4]    |
  0.0913 [1]    |

Latency distribution:
  10% in 0.0046 secs.
  25% in 0.0073 secs.
  50% in 0.0126 secs.
  75% in 0.0201 secs.
  90% in 0.0284 secs.
  95% in 0.0338 secs.
  99% in 0.0462 secs.
  99.9% in 0.0632 secs.

Benchmark result(the last of 10 times) on main-concurrentreadtxn branch, commit b05d75ab0b2a1cefed04f0590e97b52360439ee8

Total Read Ops: 159878
Details:
Summary:
  Total:        249.6188 secs.
  Slowest:      0.1514 secs.
  Fastest:      0.0006 secs.
  Average:      0.0463 secs.
  Stddev:       0.0173 secs.
  Requests/sec: 640.4886

Response time histogram:
  0.0006 [1]    |
  0.0157 [2733] |∎
  0.0307 [23988]        |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0458 [60898]        |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0609 [43338]        |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0760 [19166]        |∎∎∎∎∎∎∎∎∎∎∎∎
  0.0910 [7099] |∎∎∎∎
  0.1061 [2007] |∎
  0.1212 [522]  |
  0.1363 [105]  |
  0.1514 [21]   |

Latency distribution:
  10% in 0.0270 secs.
  25% in 0.0343 secs.
  50% in 0.0438 secs.
  75% in 0.0559 secs.
  90% in 0.0694 secs.
  95% in 0.0784 secs.
  99% in 0.0966 secs.
  99.9% in 0.1193 secs.

Total Write Ops: 40122
Details:
Summary:
  Total:        249.6191 secs.
  Slowest:      0.0918 secs.
  Fastest:      0.0015 secs.
  Average:      0.0146 secs.
  Stddev:       0.0097 secs.
  Requests/sec: 160.7329

Response time histogram:
  0.0015 [1]    |
  0.0105 [16944]        |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0196 [12842]        |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0286 [6624] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
  0.0376 [2561] |∎∎∎∎∎∎
  0.0467 [822]  |∎
  0.0557 [253]  |
  0.0647 [61]   |
  0.0738 [8]    |
  0.0828 [3]    |
  0.0918 [3]    |

Latency distribution:
  10% in 0.0046 secs.
  25% in 0.0072 secs.
  50% in 0.0123 secs.
  75% in 0.0199 secs.
  90% in 0.0279 secs.
  95% in 0.0335 secs.
  99% in 0.0453 secs.
  99.9% in 0.0583 secs.

Memory usage result(10 times) on main branch, commit 506e9fdde6b65b0396f35e22186b07f1783fb853

Average = 177018000.000000
Max = 270290944.000000

Memory usage result(10 times) on main-concurrentreadtxn branch, commit b05d75ab0b2a1cefed04f0590e97b52360439ee8

Average = 175521000.000000
Max = 262029312.000000

Summary

The benchmark is almost the same, and the memory usage is reduced by (177018000 - 175521000) / 177018000 * 100 = 0.85%.

@new-dream
Copy link
Contributor Author

@ahrtr @fuweid Please look at the test result. The memory is not reduced much.

@ahrtr
Copy link
Member

ahrtr commented Sep 8, 2023

thx @new-dream for the feedback. I am supportive on this PR given it's a simple change and the reduction on memory usage although not too much.

Pls anyone let's know if you have any comments.

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.85% might not be much, but I like doing low hanging fruits first instead of redesigning whole storage transaction cache. The change is simple and understandable enough. I'm ok with it as we don't sacrifice any readability.

@ahrtr ahrtr merged commit a13fc7e into etcd-io:main Sep 9, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants