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

mvcc: revert change made by #10526 and #10699 #10772

Merged
merged 1 commit into from
May 30, 2019

Conversation

jingyih
Copy link
Contributor

@jingyih jingyih commented May 30, 2019

Revert #10526 and its followup #10699. To fix data race in the following tests:

  • TestLeasingDeleteRangeContendTxn
  • TestBarrierSingleNode
  • TestBarrierMultiNode

&keyIndex{} is not deep copied in tree.Clone().

etcd/mvcc/index.go

Lines 94 to 96 in e1ca3b4

ti.RLock()
clone := ti.tree.Clone()
ti.RUnlock()

@jingyih jingyih added the WIP label May 30, 2019
@xiang90
Copy link
Contributor

xiang90 commented May 30, 2019

lgtm. Is there a way to fix this problem? I think we still want a low cost way to do the traverse.

@jingyih
Copy link
Contributor Author

jingyih commented May 30, 2019

lgtm. Is there a way to fix this problem? I think we still want a low cost way to do the traverse.

I am thinking about adding finer granularity locking at keyIndex level.

@codecov-io
Copy link

Codecov Report

Merging #10772 into master will decrease coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10772      +/-   ##
==========================================
- Coverage    62.3%   62.16%   -0.14%     
==========================================
  Files         392      392              
  Lines       37158    37157       -1     
==========================================
- Hits        23152    23100      -52     
- Misses      12438    12492      +54     
+ Partials     1568     1565       -3
Impacted Files Coverage Δ
mvcc/index.go 89.92% <100%> (-0.08%) ⬇️
auth/options.go 67.5% <0%> (-25%) ⬇️
proxy/grpcproxy/register.go 69.44% <0%> (-11.12%) ⬇️
etcdserver/api/v3rpc/lease.go 64.77% <0%> (-10.23%) ⬇️
clientv3/leasing/util.go 88.33% <0%> (-10%) ⬇️
pkg/netutil/netutil.go 63.11% <0%> (-6.56%) ⬇️
pkg/adt/interval_tree.go 83.18% <0%> (-5.41%) ⬇️
auth/store.go 46.08% <0%> (-4.7%) ⬇️
auth/simple_token.go 63.41% <0%> (-4.07%) ⬇️
pkg/testutil/recorder.go 77.77% <0%> (-3.71%) ⬇️
... and 25 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 dc6885d...4345f74. Read the comment docs.

@jingyih
Copy link
Contributor Author

jingyih commented May 30, 2019

I tried the same benchmark setup as mentioned in #10523 (comment), which includes three types of workloads injected concurrently. With this PR which revert the earlier optimization in index tree traversing, the result is slightly worse but very comparable. Maybe this is because ranging revisions in index tree takes much shorter time compared to ranging keys in the bbolt (and read buffer).

BaseLine
This is without this PR (therefore has the data race bug). #10523 is included.
Result: #10772 (comment)

With this PR
#10523 plus this PR, which reverts the locking optimization in index tree traversing.
Result: #10772 (comment)

@jingyih
Copy link
Contributor Author

jingyih commented May 30, 2019

BaseLine

Long Read

Summary:
Total: 81.1964 secs.
Slowest: 2.2894 secs.
Fastest: 1.6143 secs.
Average: 1.8100 secs.
Stddev: 0.1274 secs.
Requests/sec: 0.9853

Response time histogram:
1.6143 [1] |∎
1.6818 [9] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
1.7493 [20] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
1.8168 [9] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
1.8843 [24] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
1.9518 [11] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
2.0194 [2] |∎∎∎
2.0869 [1] |∎
2.1544 [1] |∎
2.2219 [0] |
2.2894 [2] |∎∎∎

Latency distribution:
10% in 1.6775 secs.
25% in 1.7069 secs.
50% in 1.8212 secs.
75% in 1.8734 secs.
90% in 1.9436 secs.
95% in 2.0472 secs.

Short Read

Summary:
Total: 42.4995 secs.
Slowest: 0.2192 secs.
Fastest: 0.0003 secs.
Average: 0.0083 secs.
Stddev: 0.0085 secs.
Requests/sec: 1176.4849

Response time histogram:
0.0003 [1] |
0.0222 [48213] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
0.0441 [1436] |∎
0.0660 [40] |
0.0879 [234] |
0.1098 [66] |
0.1316 [0] |
0.1535 [0] |
0.1754 [0] |
0.1973 [0] |
0.2192 [10] |

Latency distribution:
10% in 0.0013 secs.
25% in 0.0034 secs.
50% in 0.0080 secs.
75% in 0.0091 secs.
90% in 0.0135 secs.
95% in 0.0202 secs.
99% in 0.0318 secs.
99.9% in 0.0907 secs.

Write

Summary:
Total: 39.2792 secs.
Slowest: 0.2047 secs.
Fastest: 0.0003 secs.
Average: 0.0043 secs.
Stddev: 0.0069 secs.
Requests/sec: 1527.5268

Response time histogram:
0.0003 [1] |
0.0207 [59147] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
0.0412 [522] |
0.0616 [30] |
0.0821 [227] |
0.1025 [63] |
0.1229 [0] |
0.1434 [0] |
0.1638 [0] |
0.1843 [0] |
0.2047 [10] |

Latency distribution:
10% in 0.0012 secs.
25% in 0.0020 secs.
50% in 0.0031 secs.
75% in 0.0043 secs.
90% in 0.0067 secs.
95% in 0.0136 secs.
99% in 0.0230 secs.
99.9% in 0.0833 secs.

@jingyih
Copy link
Contributor Author

jingyih commented May 30, 2019

With this PR

Long Read

Summary:
Total: 80.9574 secs.
Slowest: 2.1666 secs.
Fastest: 1.6296 secs.
Average: 1.8302 secs.
Stddev: 0.1258 secs.
Requests/sec: 0.9882

Response time histogram:
1.6296 [1] |∎∎
1.6833 [14] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
1.7370 [11] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
1.7907 [9] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
1.8444 [5] |∎∎∎∎∎∎∎∎∎∎∎∎∎
1.8981 [11] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
1.9518 [15] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
2.0055 [10] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
2.0592 [1] |∎∎
2.1129 [2] |∎∎∎∎∎
2.1666 [1] |∎∎

Latency distribution:
10% in 1.6644 secs.
25% in 1.7155 secs.
50% in 1.8466 secs.
75% in 1.9275 secs.
90% in 1.9754 secs.
95% in 2.0521 secs.

Short Read

Summary:
Total: 44.2209 secs.
Slowest: 0.1589 secs.
Fastest: 0.0003 secs.
Average: 0.0087 secs.
Stddev: 0.0089 secs.
Requests/sec: 1130.6870

Response time histogram:
0.0003 [1] |
0.0162 [45563] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
0.0321 [3822] |∎∎∎
0.0479 [185] |
0.0638 [29] |
0.0796 [182] |
0.0955 [182] |
0.1113 [25] |
0.1272 [1] |
0.1431 [0] |
0.1589 [10] |

Latency distribution:
10% in 0.0015 secs.
25% in 0.0041 secs.
50% in 0.0080 secs.
75% in 0.0092 secs.
90% in 0.0146 secs.
95% in 0.0213 secs.
99% in 0.0422 secs.
99.9% in 0.0910 secs.

Write

Summary:
Total: 40.7489 secs.
Slowest: 0.1464 secs.
Fastest: 0.0004 secs.
Average: 0.0046 secs.
Stddev: 0.0073 secs.
Requests/sec: 1472.4331

Response time histogram:
0.0004 [1] |
0.0150 [56957] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
0.0296 [2533] |∎
0.0442 [98] |
0.0588 [24] |
0.0734 [182] |
0.0880 [175] |
0.1026 [17] |
0.1172 [3] |
0.1318 [0] |
0.1464 [10] |

Latency distribution:
10% in 0.0013 secs.
25% in 0.0020 secs.
50% in 0.0032 secs.
75% in 0.0044 secs.
90% in 0.0074 secs.
95% in 0.0151 secs.
99% in 0.0262 secs.
99.9% in 0.0832 secs.

@jingyih
Copy link
Contributor Author

jingyih commented May 30, 2019

@xiang90 I think simply reverting the previous change is good enough.

@jingyih jingyih changed the title [WIP] mvcc: revert change made by #10526 mvcc: revert change made by #10526 and #10699 May 30, 2019
@jingyih jingyih removed the WIP label May 30, 2019
@xiang90
Copy link
Contributor

xiang90 commented May 30, 2019

lgtm

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.

4 participants