server: set multiple concurrentReadTx instances share one txReadBuffer

[wilsonwang371]

Currently, the concurrentReadTx does a copy of the txReadBuffer inside backend readTx. However, this buffer is never modified by concurrentReadTx. Therefore, we can just use a single copy of the txReadBuffer as long as there are no new changes committed to backend DB.

Therefore, we use a cached txReadBuffer to avoid excessive deep copy operations for the new read transaction.

More performance testing results will be uploaded later.

@gyuho @ptabor

Guys, please take a look at this and let me know if there is any concern or issue.

[codecov-commenter]

Codecov Report

Merging #12933 (acaf5ae) into master (aeb9b5f) will decrease coverage by 0.46%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #12933      +/-   ##
==========================================
- Coverage   73.31%   72.84%   -0.47%     
==========================================
  Files         430      430              
  Lines       34182    34212      +30     
==========================================
- Hits        25060    24922     -138     
- Misses       7194     7350     +156     
- Partials     1928     1940      +12     

Flag	Coverage Δ
all	72.84% <100.00%> (-0.47%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/mvcc/backend/backend.go	82.33% <100.00%> (+1.93%)	⬆️
server/mvcc/backend/tx_buffer.go	92.30% <100.00%> (+0.20%)	⬆️
server/etcdserver/api/rafthttp/peer.go	77.41% <0.00%> (-18.07%)	⬇️
server/etcdserver/api/rafthttp/peer_status.go	89.28% <0.00%> (-10.72%)	⬇️
pkg/netutil/routes_linux.go	40.56% <0.00%> (-9.44%)	⬇️
client/pkg/v3/tlsutil/tlsutil.go	83.33% <0.00%> (-8.34%)	⬇️
server/mvcc/watchable_store.go	82.73% <0.00%> (-6.03%)	⬇️
server/proxy/grpcproxy/watch.go	92.02% <0.00%> (-5.53%)	⬇️
client/v3/leasing/cache.go	87.50% <0.00%> (-4.61%)	⬇️
raft/rafttest/node.go	95.52% <0.00%> (-4.48%)	⬇️
... and 26 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aeb9b5f...acaf5ae. Read the comment docs.

[wilsonwang371]

This is a patch inspired by #12529

[wilsonwang371]

Another different approach patch is #12935

[wilsonwang371]

Here are the performance graphs comparing the latest master branch and the concurrent read tx optimization code.

Data file:

concurrentReadTx-optimized.csv

[gyuho]

Test results look great. The master in your graph is after we merge your #10523, right? I will take another look tomorrow.

[wilsonwang371]

Test results look great. The master in your graph is after we merge your #10523, right? I will take another look tomorrow.

Sorry, it should be #12896. I will monitor the tests and fix possible bugs.

[wilsonwang371]

i have to mention that the integration test with 4 cpus failed once and this time it passed. I want to make sure it is not a bug with my code which can fail the integration test.

[wilsonwang371]

@gyuho @ptabor

Guys,

I have two questions here. First, do we still need to add a feature flag for this? Second, if the patch failed the 4cpus integration test once but later no further failures observed, is it safe?

[wilsonwang371]

@jingyih please take a look at this patch if you have time and give some suggestions.

[Jeffwan]

Can someone help add rel/3.5-nice-to-have and area/performance label to help track the progress? It would be great to add this PR to scope of etcd-v3.5

[gyuho]

Can we explain why serving from potentially stale read buffer is safe?

[wilsonwang371]

Can we explain why serving from potentially stale read buffer is safe?

Thank you Gyuho for the review comments. I updated the code as per your comments. I like the new code logic you posted in the review. I am following it. But please check my updated logic and decide whether the last if-condition is correct. 🙏

[gyuho]

After all the changes, we can see that the performance does not have a significant change.

Ah, I see. Do you have an idea why previous tests were much better?

Either way, this was a great exercise. Once the script is contributed, we can look into some ways to automate.

[wilsonwang371]

After all the changes, we can see that the performance does not have a significant change.

Ah, I see. Do you have an idea why previous tests were much better?

Either way, this was a great exercise. Once the script is contributed, we can look into some ways to automate.

I was comparing my first patch with the latest updated patch so ideally it should be 1.0 because we didn't change the real logic much with all the addressed issue fixed. The difference should be within the acceptable variance.

I am going to post a comparison between main branch and latest shared_txReadBuffer branch soon.

The file I compared were concurrentReadTx-optimized2.csv and concurrentReadTx-optimized.csv. That plot is a little bit misleading. I am going to post a new update soon comparing latest patch vs. new main branch.

[wilsonwang371]

@gyuho @ptabor

Hi guys, this is the graph of the performance. The first graph is the latest main branch performance. The second graph is the performance of this patch. The third one is the comparison between the two. It looks like we still can get similar performance improvement.

main-baseline.csv

concurrentReadTx-optimized2.csv

[wilsonwang371]

@wilsonwang371 thank you.

I think @gyuho though about comparison of 'main' vs 'latest'. From the last charts someone can have false impression that the change does not have meaningful feedback.

Can you, please, commit the benchmark script into .e.g. etcd/tools/benchmark-heatmaps/... (e.g. in separate PR).

I will file another PR to check in the benchmark script.

[wilsonwang371]

@ptabor
Piotr, are you familiar with this failure?
Linux ARM64 Graviton2 / test (pull_request)

There are no new changes except merging with the latest code.

[ptabor]

@ptabor
Piotr, are you familiar with this failure?
Linux ARM64 Graviton2 / test (pull_request)

There are no new changes except merging with the latest code.

You were hit by known flake: #12983

[gyuho]

@wilsonwang371 The latest graph above is way better than #12933 (comment). Why there's so much delta when the logic was the same?

[wilsonwang371]

@wilsonwang371 The latest graph above is way better than #12933 (comment). Why there's so much delta when the logic was the same?

Oh, they are different comparisons.

A. Main branch ----> B. Initial Pull Patch Diff ----> C. Latest Pull Patch Diff

B vs C: Graph in #12933 (comment)
A vs C: Graph in #12933 (comment)

[gyuho]

A. Main branch

This PR is rebased from the latest main branch, right?

A vs C

So, this is based on the current main branch we have today, correct?

[wilsonwang371]

A. Main branch

This PR is rebased from the latest main branch, right?

A vs C

So, this is based on the current main branch we have today, correct?

Main branch result is based on :

commit 932d42b
Merge: 1675101 3f7a038
Author: Piotr Tabor ptab@google.com
Date: Mon May 17 14:07:20 2021 +0200

Merge pull request #12971 from ptabor/20210514-split-etcdctl

Split etcdctl into etcdctl (public API access) & etcdutl (direct surgery on files)

The result of this patch is rebased on:

commit aeb9b5f
Merge: 6decbe1 f1123d6
Author: Piotr Tabor ptab@google.com
Date: Sat May 8 09:34:31 2021 +0200

Merge pull request #12855 from ptabor/20210409-backend-hooks

(no)StoreV2 (Part 4): Backend hooks:  precommit updates consistency_index

Shall I rebase it on the latest main branch and do the comparison again? It seems like recent new changes do not have some observable difference in performance.

[gyuho]

Shall I rebase it on the latest main branch and do the comparison again? It seems like recent new changes do not have some observable difference in performance.

As long as it has your PR #12896, shouldn't affect.

That said, 46b49a6 is the recent commit around storage. If we can rebase on the latest commit rather than the one in Sat May 8 and do one more tests, it would be great. Thanks!

[wilsonwang371]

Shall I rebase it on the latest main branch and do the comparison again? It seems like recent new changes do not have some observable difference in performance.

As long as it has your PR #12896, shouldn't affect.

That said, 46b49a6 is the recent commit around storage. If we can rebase on the latest commit rather than the one in Sat May 8 and do one more tests, it would be great. Thanks!

This is already done a few hours ago. I did this through the github UI. The checking result is generated after that merge.

But, in order to generate the performance graphs using python done above, it will take around 24 ~48 hours.

[wilsonwang371]

@gyuho

Hi Gyuho,

This is the performance result of the pull request based on 88d26c1

The performance is the same as the code that used in previous comments.

concurrentReadTx-optimized3.csv

[gyuho]

@wilsonwang371 Really solid testing data. Greatly appreciate it. Give me some time for me to go through this again, and will make some suggestions for comments.

[wilsonwang371]

@wilsonwang371 Really solid testing data. Greatly appreciate it. Give me some time for me to go through this again, and will make some suggestions for comments.

Hi Gyuho and Piotr,

Is it still possible to have this patch merged, tested and get shipped in 3.5 release? What is our next step before the final 3.5 release available?

@gyuho @ptabor

[gyuho]

@wilsonwang371 I will get back to you by this weekend.

[gyuho]

Can you add the requested comments to explain the logic inline?

[gyuho]

@wilsonwang371 Can we also remove merge commit, and squash/rebase from latest main branch? Thanks.

[gyuho]

lgtm. Thanks for the awesome work @wilsonwang371.

Can we also backport this to release-3.5 branch?

[ptabor]

Thank you, Wilson.

+1 to backporting (I think there are 3 chained PRs to cherry pick), and the performance impact of this change is very significant.

According to: https://www.kubernetes.dev/resources/release/, the k8s code freeze is Thursday, July 8th: Week 11 - Code Freeze), so we should have some headroom.