-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Conversation
…dTxn Signed-off-by: new-dream <[email protected]>
eec6b33
to
b05d75a
Compare
There was a problem hiding this 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?
OK, I'm happy to do performance tests, but I'm almost new here, so do you have any suggestions for performance tests?
|
Please refer to #14394 (comment), please also pay attention to the memory usage during the test, and also the metrics such as FYI, there is also a on-going discussion #16467 |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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.
Performance comparisonEnvironment: WSL2
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 clusterI tried 10 times in every branch Benchmark result(the last of 10 times) on main branch, commit
|
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. |
There was a problem hiding this 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.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.
rework #14835