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

lease: Add a heap to optimize lease expiration checks #9418

Merged
merged 4 commits into from Apr 2, 2018

Conversation

@mgates
Copy link

commented Mar 9, 2018

This adds a heap acting as a priority queue to keep track of lease
exiprations. Previously the whole lease map had to be iterated through
each time.

The queue allows us to check only those leases which might be expired.
When the expiration changes, we add an additional entry. If we check an
entry that isn't expired, it means that the lease got extended.
If we find a entry in the heap that doesn't have a corresponding entry in
the map, we know that the lease has already been expired or revoked.

This is our first stab at writing real Go, and we're happy to make any changes if you all think this is a good way of doing things.

/cc @jcalvert

@xiang90

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2018

can you write a benchmark to compare the old/new approach?

@jcalvert

This comment has been minimized.

Copy link

commented Mar 12, 2018

Absolutely. Do you have an example benchmark you would like us to emulate?

@mgates

This comment has been minimized.

Copy link
Author

commented Mar 13, 2018

We're working on a reproducible, synthetic benchmark, but here is a chart of 3.2.11, 3.3.1 and 3.3.1 with this change under our production traffic to give you an idea of the problem we are trying to fix:

graphite_browser

This is our application observed latency for creating a lease and making a transaction (plus some other stuff responsible for the baseline 100ms. These match our other metrics, where average latency is normal, but p99 and higher get very, very large. We should have a reproducible benchmark available soon.

Follow up - that spikiness in the last section was from bad compaction settings - it's almost flat with correct settings.

@gyuho

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

@mgates Do you mean the tail latency with heap is much higher than the one with map?

@gyuho

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

Ideally, benchmark (in Go, func BenchmarkTESTNAME) should show faster expiry lease query, and the compiled etcd with heap show lower p99 latency.

@mgates

This comment has been minimized.

Copy link
Author

commented Mar 13, 2018

Great - we'll get those benchmarks set up.

@mgates

This comment has been minimized.

Copy link
Author

commented Mar 13, 2018

I think I'm misunderstanding - the tail latency with the heap is much much lower (and doesn't seem to grow unbounded with lease map length).

@gyuho

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

tail latency with the heap is much much lower

Then, sounds good!

Can you provide reproducible workloads that can be cross-checked in my side?

@mgates

This comment has been minimized.

Copy link
Author

commented Mar 15, 2018

Just to let you all know, we got side-tracked, but we're still planning on getting some benchmarks done and ready.

@@ -151,14 +152,52 @@ type lessor struct {
doneC chan struct{}
}

type LeaseWithTime struct {

This comment has been minimized.

Copy link
@xiang90

xiang90 Mar 17, 2018

Contributor

move this to a new file?

This comment has been minimized.

Copy link
@mgates

mgates Mar 18, 2018

Author

That's a good idea.

@gyuho

This comment has been minimized.

Copy link
Member

commented Mar 17, 2018

@mgates This is missing some methods (e.g. revoke).

Let me cherry-pick your commit with more extensive benchmarks, tomorrow.

@mgates

This comment has been minimized.

Copy link
Author

commented Mar 18, 2018

So, we don't actually think we need to revoke or update - the idea is that the heap is just candidates - we always check the map for the actual lease. If a lease is revoked, when we get to it in the heap, we find it's not in the map, and we ignore it. If a lease is renewed, an additional marker gets put in the heap, and we ignore the first one, because the lease isn't expired in the map yet.

@gyuho

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

So, we don't actually think we need to revoke or update - the idea is that the heap is just candidates - we always check the map for the actual lease.

Then how are we going to handle false positives in server-side lease revoke routine? If a bunch of manual revoke happened, then expire channel could return leases with stale secondary index, and server errors. Was there any performance bottleneck updating heap on revoke?

@mgates

This comment has been minimized.

Copy link
Author

commented Mar 19, 2018

I don't think that we'd get errors - if we get a stale lease ID, we just ignore it. My understanding is that it would be slow to iterate through the leap finding them by lease ID, but if you think it won't be we can try it and benchmark it.

@gyuho

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

Oh I see now that your code ignores it. I will take another look.

@gyuho gyuho self-assigned this Mar 19, 2018

@jcalvert

This comment has been minimized.

Copy link

commented Mar 27, 2018

@gyuho We've added a set of benchmark tests around lease operations. From the master branch we got results of:

BenchmarkLessorFindExpired1-16                  10000000               129 ns/op
BenchmarkLessorFindExpired10-16                   100000             12785 ns/op
BenchmarkLessorFindExpired100-16                   10000            137281 ns/op
BenchmarkLessorFindExpired1000-16                   1000           1392141 ns/op
BenchmarkLessorFindExpired10000-16                   100          14356232 ns/op
BenchmarkLessorFindExpired100000-16                   10         165592206 ns/op
BenchmarkLessorFindExpired1000000-16                   1        3157352162 ns/op
BenchmarkLessorGrant1-16                          500000              3499 ns/op
BenchmarkLessorGrant10-16                         500000              3372 ns/op
BenchmarkLessorGrant100-16                        500000              3328 ns/op
BenchmarkLessorGrant1000-16                       500000              3490 ns/op
BenchmarkLessorGrant10000-16                      500000              3475 ns/op
BenchmarkLessorGrant100000-16                     500000              3503 ns/op
BenchmarkLessorGrant1000000-16                    300000              3720 ns/op
BenchmarkLessorRenew1-16                        20000000                77.5 ns/op
BenchmarkLessorRenew10-16                       20000000                78.7 ns/op
BenchmarkLessorRenew100-16                      20000000                76.8 ns/op
BenchmarkLessorRenew1000-16                     20000000                77.5 ns/op
BenchmarkLessorRenew10000-16                    20000000                77.8 ns/op
BenchmarkLessorRenew100000-16                   20000000                77.4 ns/op
BenchmarkLessorRenew1000000-16                  20000000                78.2 ns/op
BenchmarkLessorRevoke1-16                        3000000               508 ns/op
BenchmarkLessorRevoke10-16                       3000000               488 ns/op
BenchmarkLessorRevoke100-16                      3000000               490 ns/op
BenchmarkLessorRevoke1000-16                     3000000               489 ns/op
BenchmarkLessorRevoke10000-16                    3000000               488 ns/op
BenchmarkLessorRevoke100000-16                   3000000               489 ns/op
BenchmarkLessorRevoke1000000-16                  3000000               503 ns/op

and with our patch:

BenchmarkLessorFindExpired1-16                  10000000               125 ns/op
BenchmarkLessorFindExpired10-16                  1000000              1537 ns/op
BenchmarkLessorFindExpired100-16                 1000000              1628 ns/op
BenchmarkLessorFindExpired1000-16                1000000              1730 ns/op
BenchmarkLessorFindExpired10000-16               1000000              1898 ns/op
BenchmarkLessorFindExpired100000-16              1000000              2029 ns/op
BenchmarkLessorFindExpired1000000-16              500000              2038 ns/op
BenchmarkLessorGrant1-16                          500000              3685 ns/op
BenchmarkLessorGrant10-16                         500000              3593 ns/op
BenchmarkLessorGrant100-16                        500000              3605 ns/op
BenchmarkLessorGrant1000-16                       500000              3818 ns/op
BenchmarkLessorGrant10000-16                      500000              3681 ns/op
BenchmarkLessorGrant100000-16                     500000              3644 ns/op
BenchmarkLessorGrant1000000-16                    500000              3664 ns/op
BenchmarkLessorRenew1-16                        20000000                79.8 ns/op
BenchmarkLessorRenew10-16                       20000000                79.7 ns/op
BenchmarkLessorRenew100-16                      20000000                79.7 ns/op
BenchmarkLessorRenew1000-16                     20000000                80.6 ns/op
BenchmarkLessorRenew10000-16                    20000000                80.8 ns/op
BenchmarkLessorRenew100000-16                   20000000                79.7 ns/op
BenchmarkLessorRenew1000000-16                  20000000                80.4 ns/op
BenchmarkLessorRevoke1-16                        3000000               487 ns/op
BenchmarkLessorRevoke10-16                       3000000               489 ns/op
BenchmarkLessorRevoke100-16                      3000000               488 ns/op
BenchmarkLessorRevoke1000-16                     3000000               487 ns/op
BenchmarkLessorRevoke10000-16                    3000000               485 ns/op
BenchmarkLessorRevoke100000-16                   3000000               489 ns/op
BenchmarkLessorRevoke1000000-16                  3000000               498 ns/op

We believe this accurately reflects the improvement in performance we've seen with the find expired leases function.

@gyuho

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

@mgates Thanks for updates!

accurately reflects the improvement in performance we've seen with the find expired leases function.

I would like to cross-check on this before we merging in. What kind of workloads did you ingest to verify this improvements?

I am busy till next week. So will try to merge this by following week latest.

@jcalvert

This comment has been minimized.

Copy link

commented Mar 27, 2018

@gyuho - We cloned our traffic in a production environment to a separate cluster, for about 500 GRPC operations per second (tx, get, put, lease grant etc) with a max of about 250 leases expiring per second and a total number of keys averaging around 5 million. This same workload rendered our unpatched cluster unusable within 30 minutes of operation.

@xiang90

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2018

The benchmark result makes sense to me.

if l == nil {
// lease has expired or been revoked, continue
continue
} else if time.Now().UnixNano() < item.expiration {

This comment has been minimized.

Copy link
@xiang90

xiang90 Mar 28, 2018

Contributor

no need the else statement here. just if time.Now ... is enough.

@xiang90

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2018

@jcalvert can you please clean this PR up? Then we can get it merged.

func NewLessor(b backend.Backend, minLeaseTTL int64) Lessor {
return newLessor(b, minLeaseTTL)
}

func newLessor(b backend.Backend, minLeaseTTL int64) *lessor {

This comment has been minimized.

Copy link
@gyuho

gyuho Mar 30, 2018

Member

remove this line?

// TODO: probably should change to <= 100-500 millisecond to
// make up committing latency.
for {

This comment has been minimized.

Copy link
@gyuho

gyuho Mar 30, 2018

Member

remove this line?

@gyuho

This comment has been minimized.

Copy link
Member

commented Mar 30, 2018

LGTM after addressing @xiang90's comments. We can merge first. I will do the benchmarks sometime later.

Thanks!

@cosgroveb

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

Hey @gyuho and @xiang90,

@jcalvert and I have pushed some cleanups to the PR and I think we addressed all outstanding feedback. Let us know.

@gyuho

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

@mgates @cosgroveb Could you squash commits and add license header? I have example branch https://github.com/gyuho/etcd/commits/tmp.

@jcalvert jcalvert force-pushed the mgates:use_heap_to_track_lease_expirations branch from 97b1c02 to cb5e822 Apr 2, 2018

@cosgroveb

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

@gyuho Added the license header to both new files and squashed our commits.

@gyuho

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

@cosgroveb One last nit, could you amend commit titles to lease: ... (CI will complain about this)? Then LGTM.

Thanks a lot!

braintreeps added 2 commits Mar 9, 2018
lease: Add a heap to optimize lease expiration checks
This adds a heap acting as a priority queue to keep track of lease
exiprations. Previously the whole lease map had to be iterated through
each time.

The queue allows us to check only those leases which might be expired.
When the expiration changes, we add an additional entry. If we check an
entry that isn't expired, it means that the lease got extended.
If we find a entry in the heap that doesn't have a corresponding entry in
the map, we know that the lease has already been expired or revoked.

@gyuho gyuho force-pushed the mgates:use_heap_to_track_lease_expirations branch from cb5e822 to 3f85ae7 Apr 2, 2018

@gyuho

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

@mgates I just rebased from master to trigger CIs. Will add release notes as well. Thanks.

CHANGELOG: update
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

@gyuho gyuho changed the title Add a heap to optimize lease expiration checks lease: Add a heap to optimize lease expiration checks Apr 2, 2018

lease: fix gofmt
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
@gyuho

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

Test failures aren't related. I also manually ran full test suites and no failures. Merging.

@gyuho gyuho merged commit 2aa3dec into etcd-io:master Apr 2, 2018

4 of 5 checks passed

semaphoreci The build is pending on Semaphore.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-cov Build finished.
Details
jenkins-ppc64le Build finished.
Details
jenkins-proxy-ci Build finished.
Details
cosgroveb added a commit to cosgroveb/etcd that referenced this pull request Apr 27, 2018
jcalvert pushed a commit to jcalvert/etcd that referenced this pull request Jul 2, 2018
Merge pull request etcd-io#9418 from mgates/use_heap_to_track_lease_e…
…xpirations

lease: Add a heap to optimize lease expiration checks
jcalvert pushed a commit to jcalvert/etcd that referenced this pull request Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.