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: set multiple concurrentReadTx instances share one txReadBuffer #12933

Merged
merged 1 commit into from
May 24, 2021

Conversation

wilsonwang371
Copy link
Contributor

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.

@wilsonwang371 wilsonwang371 force-pushed the shared_txReadBuffer branch 2 times, most recently from a4bda87 to 34793bf Compare May 9, 2021 23:41
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2021

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor Author

This is a patch inspired by #12529

@wilsonwang371
Copy link
Contributor Author

Another different approach patch is #12935

@gyuho gyuho self-assigned this May 10, 2021
@wilsonwang371
Copy link
Contributor Author

wilsonwang371 commented May 11, 2021

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

image

image

image

Data file:

concurrentReadTx-optimized.csv

@wilsonwang371 wilsonwang371 force-pushed the shared_txReadBuffer branch 4 times, most recently from 79ee062 to 0cbc33f Compare May 11, 2021 05:56
server/mvcc/backend/backend.go Outdated Show resolved Hide resolved
server/mvcc/backend/backend.go Outdated Show resolved Hide resolved
server/mvcc/backend/backend.go Outdated Show resolved Hide resolved
@gyuho
Copy link
Contributor

gyuho commented May 11, 2021

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

@wilsonwang371
Copy link
Contributor Author

wilsonwang371 commented May 11, 2021

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 wilsonwang371 force-pushed the shared_txReadBuffer branch 2 times, most recently from 1484273 to bcc8aae Compare May 11, 2021 06:50
@wilsonwang371
Copy link
Contributor Author

wilsonwang371 commented May 11, 2021

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@Jeffwan
Copy link

Jeffwan commented May 12, 2021

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

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

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

server/mvcc/backend/backend.go Outdated Show resolved Hide resolved
server/mvcc/backend/backend.go Outdated Show resolved Hide resolved
server/mvcc/backend/backend.go Outdated Show resolved Hide resolved
server/mvcc/backend/backend.go Outdated Show resolved Hide resolved
server/mvcc/backend/tx_buffer.go Outdated Show resolved Hide resolved
server/mvcc/backend/backend.go Show resolved Hide resolved
server/mvcc/backend/backend.go Outdated Show resolved Hide resolved
@wilsonwang371
Copy link
Contributor Author

wilsonwang371 commented May 13, 2021

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
Copy link
Contributor

gyuho commented May 17, 2021

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
Copy link
Contributor Author

wilsonwang371 commented May 17, 2021

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
Copy link
Contributor Author

wilsonwang371 commented May 18, 2021

@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.

image

image

image

main-baseline.csv

concurrentReadTx-optimized2.csv

@wilsonwang371
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Contributor

ptabor commented May 18, 2021

@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
Copy link
Contributor

gyuho commented May 18, 2021

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

@wilsonwang371
Copy link
Contributor Author

@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
Copy link
Contributor

gyuho commented May 18, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

gyuho commented May 18, 2021

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
Copy link
Contributor Author

wilsonwang371 commented May 18, 2021

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
Copy link
Contributor Author

@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.

image

image

image

concurrentReadTx-optimized3.csv

@gyuho
Copy link
Contributor

gyuho commented May 19, 2021

@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
Copy link
Contributor Author

@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
Copy link
Contributor

gyuho commented May 21, 2021

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

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

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

server/mvcc/backend/backend.go Show resolved Hide resolved
server/mvcc/backend/backend.go Show resolved Hide resolved
server/mvcc/backend/backend.go Show resolved Hide resolved
@gyuho
Copy link
Contributor

gyuho commented May 24, 2021

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

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks for the awesome work @wilsonwang371.

Can we also backport this to release-3.5 branch?

@ptabor
Copy link
Contributor

ptabor commented May 25, 2021

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.

@wilsonwang371 wilsonwang371 deleted the shared_txReadBuffer branch May 27, 2021 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants