From a6e171ddf7ab5bf7fdbba8e88d7f80df39c2a1c0 Mon Sep 17 00:00:00 2001 From: Anant Prakash Date: Mon, 14 May 2018 23:38:20 +0530 Subject: [PATCH] Use cycling double map instead, improve code logic, remove unneeded test Signed-off-by: Anant Prakash --- .../common/transactions/transactions.go | 72 +++++++++---------- .../common/transactions/transactions_test.go | 33 ++------- 2 files changed, 39 insertions(+), 66 deletions(-) diff --git a/src/github.com/matrix-org/dendrite/common/transactions/transactions.go b/src/github.com/matrix-org/dendrite/common/transactions/transactions.go index 349292a4b..5c5944e8f 100644 --- a/src/github.com/matrix-org/dendrite/common/transactions/transactions.go +++ b/src/github.com/matrix-org/dendrite/common/transactions/transactions.go @@ -13,75 +13,73 @@ package transactions import ( - "errors" "sync" "time" "github.com/matrix-org/util" ) -// CleanupPeriod represents time in nanoseconds after which cacheCleanService runs. -const CleanupPeriod time.Duration = 30 * time.Minute +// DefaultCleanupPeriod represents time in nanoseconds after which cacheCleanService runs. +const DefaultCleanupPeriod time.Duration = 30 * time.Minute -type entry struct { - value *util.JSONResponse - entryTime time.Time -} +type txnsMap map[string]*util.JSONResponse // Cache represents a temporary store for entries. -// Entries are evicted after a certain period, defined by CleanupPeriod. +// Entries are evicted after a certain period, defined by cleanupPeriod. type Cache struct { sync.RWMutex - txns map[string]entry + txnsMaps [2]txnsMap + cleanupPeriod time.Duration } // New creates a new Cache object, starts cacheCleanService. -// Returns a referance to newly created Cache. -func New() *Cache { - t := Cache{txns: make(map[string]entry)} +// Takes cleanupPeriod (in minutes) as argument, in case of 0 DefaultCleanupPeriod is used instead. +// Returns a reference to newly created Cache. +func New(cleanupPeriodInMins int) *Cache { + t := Cache{txnsMaps: [2]txnsMap{make(txnsMap), make(txnsMap)}} - // Start cleanup service as the Cache is created + cleanupPeriod := time.Duration(cleanupPeriodInMins) * time.Minute + if cleanupPeriod == 0 { + cleanupPeriod = DefaultCleanupPeriod + } + t.cleanupPeriod = cleanupPeriod + + // Start clean service as the Cache is created go cacheCleanService(&t) return &t } // FetchTransaction looks up an entry for txnID in Cache. -// Returns a JSON response if txnID is found, else returns error. -func (t *Cache) FetchTransaction(txnID string) (*util.JSONResponse, error) { +// Returns (JSON response, true) if txnID is found, else the returned bool is false. +func (t *Cache) FetchTransaction(txnID string) (*util.JSONResponse, bool) { t.RLock() - res, ok := t.txns[txnID] - t.RUnlock() - - if ok { - return res.value, nil + defer t.RUnlock() + for _, txns := range t.txnsMaps { + res, ok := txns[txnID] + if ok { + return res, true + } } - return nil, errors.New("TxnID not present") + return nil, false } -// AddTransaction adds a entry for txnID in Cache for later access. +// AddTransaction adds an entry for txnID in Cache for later access. func (t *Cache) AddTransaction(txnID string, res *util.JSONResponse) { t.Lock() defer t.Unlock() - t.txns[txnID] = entry{value: res, entryTime: time.Now()} + t.txnsMaps[0][txnID] = res } -// cacheCleanService is responsible for cleaning up transactions older than CleanupPeriod. -// It guarantees that a transaction will be present in cache for at least CleanupPeriod & at most 2 * CleanupPeriod. +// cacheCleanService is responsible for cleaning up entries after cleanupPeriod. +// It guarantees that an entry will be present in cache for at least cleanupPeriod & at most 2 * cleanupPeriod. +// This works by func cacheCleanService(t *Cache) { - for { - time.Sleep(CleanupPeriod) - go clean(t) - } -} - -func clean(t *Cache) { - expire := time.Now().Add(-CleanupPeriod) - for key := range t.txns { + ticker := time.Tick(t.cleanupPeriod) + for range ticker { t.Lock() - if t.txns[key].entryTime.Before(expire) { - delete(t.txns, key) - } + t.txnsMaps[1] = t.txnsMaps[0] + t.txnsMaps[0] = make(txnsMap) t.Unlock() } } diff --git a/src/github.com/matrix-org/dendrite/common/transactions/transactions_test.go b/src/github.com/matrix-org/dendrite/common/transactions/transactions_test.go index 459cc7617..b0713dacb 100644 --- a/src/github.com/matrix-org/dendrite/common/transactions/transactions_test.go +++ b/src/github.com/matrix-org/dendrite/common/transactions/transactions_test.go @@ -30,8 +30,7 @@ var ( // TestCache creates a New Cache and tests AddTransaction & FetchTransaction func TestCache(t *testing.T) { - - fakeTxnCache := New() + fakeTxnCache := New(0) fakeTxnCache.AddTransaction(fakeTxnID, fakeResponse) // Add entries for noise. @@ -42,34 +41,10 @@ func TestCache(t *testing.T) { ) } - testResponse, err := fakeTxnCache.FetchTransaction(fakeTxnID) - if err != nil { + testResponse, ok := fakeTxnCache.FetchTransaction(fakeTxnID) + if !ok { t.Error("Failed to retrieve entry for txnID: ", fakeTxnID) } else if testResponse.JSON != fakeResponse.JSON { - t.Error("Incorrect fetched response. Expected: ", fakeResponse.JSON, " got: ", testResponse.JSON) - } -} - -// TestConcurentAccess provides some guarantee against corruption. -func TestConcurentAccess(t *testing.T) { - fakeTxnCache := New() - // Signal that this test should run in parallel - t.Parallel() - // Add entries concurrently to test concurrent access. - for i := 1; i <= 1000; i++ { - go check(t, fakeTxnCache, i) - } -} - -// Adds an entry and checks it is retrieved. -func check(t *testing.T, fakeTxnCache *Cache, i int) { - fakeTxnCache.AddTransaction( - fakeTxnID+string(i), - &util.JSONResponse{Code: http.StatusOK, JSON: fakeType{ID: string(i)}}, - ) - _, err := fakeTxnCache.FetchTransaction(fakeTxnID + string(i)) - - if err != nil { - t.Error("Failed to retrieve entry for txnID: ", fakeTxnID+string(i)) + t.Error("Fetched response incorrect. Expected: ", fakeResponse.JSON, " got: ", testResponse.JSON) } }