From 51b8d68a7fd910c3f58e065d7f9e05649195d9b6 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Mon, 2 Apr 2018 14:58:34 -0700 Subject: [PATCH 1/4] lease: godoc "LeaseWithTime", change field name to "id" No need to have "lease" in field name. Signed-off-by: Gyuho Lee --- lease/lease_queue.go | 3 ++- lease/lessor.go | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lease/lease_queue.go b/lease/lease_queue.go index a7c3cf553..261df9509 100644 --- a/lease/lease_queue.go +++ b/lease/lease_queue.go @@ -14,8 +14,9 @@ package lease +// LeaseWithTime contains lease object with expire information. type LeaseWithTime struct { - leaseId LeaseID + id LeaseID expiration int64 index int } diff --git a/lease/lessor.go b/lease/lessor.go index 8609e47b6..85d2f90b6 100644 --- a/lease/lessor.go +++ b/lease/lessor.go @@ -235,7 +235,7 @@ func (le *lessor) Grant(id LeaseID, ttl int64) (*Lease, error) { } le.leaseMap[id] = l - item := &LeaseWithTime{leaseId: l.ID, expiration: l.expiry.UnixNano()} + item := &LeaseWithTime{id: l.ID, expiration: l.expiry.UnixNano()} heap.Push(&le.leaseHeap, item) l.persistTo(le.b) @@ -319,7 +319,7 @@ func (le *lessor) Renew(id LeaseID) (int64, error) { } l.refresh(0) - item := &LeaseWithTime{leaseId: l.ID, expiration: l.expiry.UnixNano()} + item := &LeaseWithTime{id: l.ID, expiration: l.expiry.UnixNano()} heap.Push(&le.leaseHeap, item) return l.ttl, nil } @@ -355,7 +355,7 @@ func (le *lessor) Promote(extend time.Duration) { // refresh the expiries of all leases. for _, l := range le.leaseMap { l.refresh(extend) - item := &LeaseWithTime{leaseId: l.ID, expiration: l.expiry.UnixNano()} + item := &LeaseWithTime{id: l.ID, expiration: l.expiry.UnixNano()} heap.Push(&le.leaseHeap, item) } @@ -392,7 +392,7 @@ func (le *lessor) Promote(extend time.Duration) { delay := time.Duration(rateDelay) nextWindow = baseWindow + delay l.refresh(delay + extend) - item := &LeaseWithTime{leaseId: l.ID, expiration: l.expiry.UnixNano()} + item := &LeaseWithTime{id: l.ID, expiration: l.expiry.UnixNano()} heap.Push(&le.leaseHeap, item) } } @@ -532,7 +532,7 @@ func (le *lessor) findExpiredLeases(limit int) []*Lease { } item := heap.Pop(&le.leaseHeap).(*LeaseWithTime) - l := le.leaseMap[item.leaseId] + l := le.leaseMap[item.id] if l == nil { // lease has expired or been revoked, continue continue From f9b7a012b5c25df2ce95cb36f4ef5fe501472314 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Mon, 2 Apr 2018 14:59:19 -0700 Subject: [PATCH 2/4] lease: add "TestLeaseQueue" Signed-off-by: Gyuho Lee --- lease/lease_queue_test.go | 44 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 lease/lease_queue_test.go diff --git a/lease/lease_queue_test.go b/lease/lease_queue_test.go new file mode 100644 index 000000000..575fbc169 --- /dev/null +++ b/lease/lease_queue_test.go @@ -0,0 +1,44 @@ +// Copyright 2018 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package lease + +import ( + "container/heap" + "testing" + "time" +) + +func TestLeaseQueue(t *testing.T) { + le := &lessor{ + leaseHeap: make(LeaseQueue, 0), + leaseMap: make(map[LeaseID]*Lease), + } + heap.Init(&le.leaseHeap) + + // insert in reverse order of expiration time + for i := 50; i >= 1; i-- { + exp := time.Now().Add(time.Hour).UnixNano() + if i == 1 { + exp = time.Now().UnixNano() + } + le.leaseMap[LeaseID(i)] = &Lease{ID: LeaseID(i)} + heap.Push(&le.leaseHeap, &LeaseWithTime{id: LeaseID(i), expiration: exp}) + } + + // first element must be front + if le.leaseHeap[0].id != LeaseID(1) { + t.Fatalf("first item expected lease ID %d, got %d", LeaseID(1), le.leaseHeap[0].id) + } +} From a6984c53deb5c9beb262dcf260a09e01d59339b7 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Mon, 2 Apr 2018 16:27:16 -0700 Subject: [PATCH 3/4] lease: add "expireExists" Signed-off-by: Gyuho Lee --- lease/lease_queue_test.go | 11 ++++++++++ lease/lessor.go | 44 +++++++++++++++++++++++++++++---------- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/lease/lease_queue_test.go b/lease/lease_queue_test.go index 575fbc169..72346a1b8 100644 --- a/lease/lease_queue_test.go +++ b/lease/lease_queue_test.go @@ -41,4 +41,15 @@ func TestLeaseQueue(t *testing.T) { if le.leaseHeap[0].id != LeaseID(1) { t.Fatalf("first item expected lease ID %d, got %d", LeaseID(1), le.leaseHeap[0].id) } + + l, ok, more := le.expireExists() + if l.ID != 1 { + t.Fatalf("first item expected lease ID %d, got %d", 1, l.ID) + } + if !ok { + t.Fatal("expect expiry lease exists") + } + if more { + t.Fatal("expect no more expiry lease") + } } diff --git a/lease/lessor.go b/lease/lessor.go index 85d2f90b6..21569394c 100644 --- a/lease/lessor.go +++ b/lease/lessor.go @@ -521,28 +521,50 @@ func (le *lessor) runLoop() { } } +// expireExists returns true if expiry items exist. +// It pops "first" expired item from heap. +// If it's not expired yet, add it back. +// "next" is true, to indicate that it may exist in next attempt. +func (le *lessor) expireExists() (l *Lease, ok bool, next bool) { + if le.leaseHeap.Len() == 0 { + return nil, false, false + } + + item := heap.Pop(&le.leaseHeap).(*LeaseWithTime) // O(log N) + l = le.leaseMap[item.id] + if l == nil { + // lease has expired or been revoked + // no need to revoke (nothing is expiry) + return nil, false, true + } + + if time.Now().UnixNano() < item.expiration { + // Candidate expirations are caught up, reinsert this item + // and no need to revoke (nothing is expiry) + heap.Push(&le.leaseHeap, item) // O(log N) + return l, false, false + } + // if the lease is actually expired, add to the removal list. If it is not expired, we can ignore it because another entry will have been inserted into the heap + + return l, true, false +} + // findExpiredLeases loops leases in the leaseMap until reaching expired limit // and returns the expired leases that needed to be revoked. func (le *lessor) findExpiredLeases(limit int) []*Lease { leases := make([]*Lease, 0, 16) for { - if le.leaseHeap.Len() == 0 { + l, ok, next := le.expireExists() + if !ok && !next { break } - - item := heap.Pop(&le.leaseHeap).(*LeaseWithTime) - l := le.leaseMap[item.id] - if l == nil { - // lease has expired or been revoked, continue + if !ok { continue } - if time.Now().UnixNano() < item.expiration { - // Candidate expirations are caught up, reinsert this item - heap.Push(&le.leaseHeap, item) - break + if next { + continue } - // if the lease is actually expired, add to the removal list. If it is not expired, we can ignore it because another entry will have been inserted into the heap if l.expired() { leases = append(leases, l) From 9c62d7b2d112baae8af4745f6a66dc44196ed1b0 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Mon, 2 Apr 2018 16:31:58 -0700 Subject: [PATCH 4/4] leaes: remove unnecessary O(log N) operation when nothing is expiry Since heap is already sorted, we can just check first element to see if anything is expiry, rather than popping and pushing it back. If nothing is expiry, pop operation is unnecessary. Signed-off-by: Gyuho Lee --- lease/lease_queue_test.go | 4 ++++ lease/lessor.go | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lease/lease_queue_test.go b/lease/lease_queue_test.go index 72346a1b8..201911fd0 100644 --- a/lease/lease_queue_test.go +++ b/lease/lease_queue_test.go @@ -52,4 +52,8 @@ func TestLeaseQueue(t *testing.T) { if more { t.Fatal("expect no more expiry lease") } + + if le.leaseHeap.Len() != 49 { + t.Fatalf("expected lease heap pop, got %d", le.leaseHeap.Len()) + } } diff --git a/lease/lessor.go b/lease/lessor.go index 21569394c..90c568f9d 100644 --- a/lease/lessor.go +++ b/lease/lessor.go @@ -522,30 +522,30 @@ func (le *lessor) runLoop() { } // expireExists returns true if expiry items exist. -// It pops "first" expired item from heap. -// If it's not expired yet, add it back. +// It pops only when expiry item exists. // "next" is true, to indicate that it may exist in next attempt. func (le *lessor) expireExists() (l *Lease, ok bool, next bool) { if le.leaseHeap.Len() == 0 { return nil, false, false } - item := heap.Pop(&le.leaseHeap).(*LeaseWithTime) // O(log N) + item := le.leaseHeap[0] l = le.leaseMap[item.id] if l == nil { // lease has expired or been revoked // no need to revoke (nothing is expiry) + heap.Pop(&le.leaseHeap) // O(log N) return nil, false, true } if time.Now().UnixNano() < item.expiration { // Candidate expirations are caught up, reinsert this item // and no need to revoke (nothing is expiry) - heap.Push(&le.leaseHeap, item) // O(log N) return l, false, false } // if the lease is actually expired, add to the removal list. If it is not expired, we can ignore it because another entry will have been inserted into the heap + heap.Pop(&le.leaseHeap) // O(log N) return l, true, false }