From 6efada43e73c40e0c76c441f84cf02cc00d3eb1b Mon Sep 17 00:00:00 2001 From: vince Date: Sun, 26 Jul 2020 15:52:29 +0800 Subject: [PATCH 1/6] Implement the LRU Cache --- bug/bug.go | 19 +++++++++ cache/lru_id_cache.go | 64 +++++++++++++++++++++++++++++ cache/repo_cache.go | 20 ++++++++- cache/repo_cache_bug.go | 89 +++++++++++++++++++++++++++++----------- cache/repo_cache_test.go | 76 ++++++++++++++++++++++++++++++++-- go.mod | 2 +- go.sum | 1 + 7 files changed, 241 insertions(+), 30 deletions(-) create mode 100644 cache/lru_id_cache.go diff --git a/bug/bug.go b/bug/bug.go index 2ee890310814ab8e62e47ef7fab77a437eb3a0c6..3a770881b6bcaa3c8a3f39618373300b08716372 100644 --- a/bug/bug.go +++ b/bug/bug.go @@ -749,3 +749,22 @@ func (bug *Bug) Compile() Snapshot { return snap } + +// EquivalentBug returns true if two bugs are equal +func EquivalentBug(expected, actual *Bug) bool { + if len(expected.packs) != len(actual.packs) { + return false + } + + for i := range expected.packs { + for j := range expected.packs[i].Operations { + actual.packs[i].Operations[j].base().id = expected.packs[i].Operations[j].base().id + } + } + + if expected != actual { + return false + } + + return true +} diff --git a/cache/lru_id_cache.go b/cache/lru_id_cache.go new file mode 100644 index 0000000000000000000000000000000000000000..e31f2843f4b00110b647ca3f551475f1529ef57d --- /dev/null +++ b/cache/lru_id_cache.go @@ -0,0 +1,64 @@ +package cache + +import ( + lru "github.com/hashicorp/golang-lru" + + "github.com/MichaelMure/git-bug/entity" +) + +type LRUIdCache struct { + parentCache *lru.Cache + maxSize int + onEvict func(id entity.Id) +} + +func NewLRUIdCache(size int, onEvicted func(id entity.Id)) (*LRUIdCache, error) { + cache, err := lru.New(size) + if err != nil { + return nil, err + } + + return &LRUIdCache{ + cache, + size, + onEvicted, + }, nil +} + +func (c *LRUIdCache) Add(id entity.Id) bool { + return c.parentCache.Add(id, nil) +} + +func (c *LRUIdCache) Contains(id entity.Id) bool { + return c.parentCache.Contains(id) +} + +func (c *LRUIdCache) Get(id entity.Id) bool { + _, present := c.parentCache.Get(id) + return present +} + +func (c *LRUIdCache) GetOldest() (entity.Id, bool) { + id, _, present := c.parentCache.GetOldest() + return id.(entity.Id), present +} + +func (c *LRUIdCache) GetAll() (ids []entity.Id) { + interfaceKeys := c.parentCache.Keys() + for _, id := range interfaceKeys { + ids = append(ids, id.(entity.Id)) + } + return +} + +func (c *LRUIdCache) Len() int { + return c.parentCache.Len() +} + +func (c *LRUIdCache) Remove(id entity.Id) bool { + return c.parentCache.Remove(id) +} + +func (c *LRUIdCache) Resize(size int) int { + return c.parentCache.Resize(size) +} diff --git a/cache/repo_cache.go b/cache/repo_cache.go index 8977245588bf0a0ddfe2e826cb2ea85f59fa8ca7..5fe1b7981eb3db6f663f7b11ce7c0688a92e17fa 100644 --- a/cache/repo_cache.go +++ b/cache/repo_cache.go @@ -21,6 +21,8 @@ import ( // 2: added cache for identities with a reference in the bug cache const formatVersion = 2 +const lruCacheSize = 100 + var _ repository.RepoCommon = &RepoCache{} // RepoCache is a cache for a Repository. This cache has multiple functions: @@ -50,6 +52,9 @@ type RepoCache struct { // bug loaded in memory bugs map[entity.Id]*BugCache + // presentBugs is an LRU cache that records which bugs the cache has loaded in + presentBugs *LRUIdCache + muIdentity sync.RWMutex // excerpt of identities data for all identities identitiesExcerpts map[entity.Id]*IdentityExcerpt @@ -144,7 +149,7 @@ func (c *RepoCache) Close() error { c.identities = make(map[entity.Id]*IdentityCache) c.identitiesExcerpts = nil - c.bugs = make(map[entity.Id]*BugCache) + c.bugs = nil c.bugExcerpts = nil lockPath := repoLockFilePath(c.repo) @@ -175,11 +180,22 @@ func (c *RepoCache) buildCache() error { _, _ = fmt.Fprintf(os.Stderr, "Building bug cache... ") + presentBugs, err := NewLRUIdCache(lruCacheSize, c.onEvict) + if err != nil { + return err + } + c.presentBugs = presentBugs + c.bugExcerpts = make(map[entity.Id]*BugExcerpt) allBugs := bug.ReadAllLocalBugs(c.repo) - for b := range allBugs { + for i := 0; i < lruCacheSize; i++ { + if len(allBugs) == 0 { + break + } + + b := <-allBugs if b.Err != nil { return b.Err } diff --git a/cache/repo_cache_bug.go b/cache/repo_cache_bug.go index bcbfcea3b542216cbc1561a902d0f4339ec3c1ce..573973619f8f7fbb90db42f95e08a69b0b5f23cd 100644 --- a/cache/repo_cache_bug.go +++ b/cache/repo_cache_bug.go @@ -24,14 +24,13 @@ func bugCacheFilePath(repo repository.Repo) string { // bugUpdated is a callback to trigger when the excerpt of a bug changed, // that is each time a bug is updated func (c *RepoCache) bugUpdated(id entity.Id) error { - c.muBug.Lock() - - b, ok := c.bugs[id] - if !ok { - c.muBug.Unlock() - panic("missing bug in the cache") + err := c.ensureBugLoaded(id) + if err != nil { + return err } + c.muBug.Lock() + b, _ := c.bugs[id] c.bugExcerpts[id] = NewBugExcerpt(b.bug, b.Snapshot()) c.muBug.Unlock() @@ -106,38 +105,59 @@ func (c *RepoCache) writeBugCache() error { // ResolveBugExcerpt retrieve a BugExcerpt matching the exact given id func (c *RepoCache) ResolveBugExcerpt(id entity.Id) (*BugExcerpt, error) { - c.muBug.RLock() - defer c.muBug.RUnlock() - - e, ok := c.bugExcerpts[id] - if !ok { - return nil, bug.ErrBugNotExist + err := c.ensureBugLoaded(id) + if err != nil { + return nil, err } - return e, nil + c.muBug.RLock() + defer c.muBug.RUnlock() + return c.bugExcerpts[id], nil } // ResolveBug retrieve a bug matching the exact given id func (c *RepoCache) ResolveBug(id entity.Id) (*BugCache, error) { + err := c.ensureBugLoaded(id) + if err != nil { + return nil, err + } + c.muBug.RLock() - cached, ok := c.bugs[id] - c.muBug.RUnlock() - if ok { - return cached, nil + defer c.muBug.RUnlock() + return c.bugs[id], nil +} + +func (c *RepoCache) ensureBugLoaded(id entity.Id) error { + if c.presentBugs.Get(id) { + return nil } b, err := bug.ReadLocalBug(c.repo, id) if err != nil { - return nil, err + return err } - cached = NewBugCache(c, b) + bugCache := NewBugCache(c, b) c.muBug.Lock() - c.bugs[id] = cached - c.muBug.Unlock() + if c.presentBugs.Len() == c.presentBugs.maxSize { + for _, id := range c.presentBugs.GetAll() { + if b := c.bugs[id]; !b.NeedCommit() { + b.mu.Lock() + c.presentBugs.Remove(id) + delete(c.bugExcerpts, id) + delete(c.bugs, id) + } + } + } - return cached, nil + c.presentBugs.Add(id) + c.bugs[id] = bugCache + excerpt := NewBugExcerpt(b, bugCache.Snapshot()) // TODO: Is this needed? + c.bugExcerpts[id] = excerpt + + c.muBug.Unlock() + return nil } // ResolveBugExcerptPrefix retrieve a BugExcerpt matching an id prefix. It fails if multiple @@ -363,15 +383,38 @@ func (c *RepoCache) NewBugRaw(author *IdentityCache, unixTime int64, title strin // RemoveBug removes a bug from the cache and repo given a bug id prefix func (c *RepoCache) RemoveBug(prefix string) error { b, err := c.ResolveBugPrefix(prefix) + if err != nil { + return err + } + err = c.ensureBugLoaded(b.Id()) if err != nil { return err } + c.muBug.Lock() + b.mu.Lock() + fmt.Println("got lock") err = bug.RemoveBug(c.repo, b.Id()) - + if err != nil { + c.muBug.Unlock() + b.mu.Unlock() + return err + } + fmt.Println("noerr") + c.presentBugs.Remove(b.Id()) + fmt.Println("removing1") delete(c.bugs, b.Id()) + fmt.Println("removed2") delete(c.bugExcerpts, b.Id()) + fmt.Println("unlocking") + c.muBug.Unlock() return c.writeBugCache() } + +// onEvict will update the bugs and bugExcerpts when a bug is evicted from the cache +func (c *RepoCache) onEvict(id entity.Id) { // TODO: Do we need this? + delete(c.bugs, id) + delete(c.bugExcerpts, id) +} diff --git a/cache/repo_cache_test.go b/cache/repo_cache_test.go index 0deb155ed506a996a64688ac53bd2eaaa3649c64..ba59a7df15add74321b668e58958f97cb690be15 100644 --- a/cache/repo_cache_test.go +++ b/cache/repo_cache_test.go @@ -1,9 +1,7 @@ package cache import ( - "fmt" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -181,13 +179,16 @@ func TestRemove(t *testing.T) { rene, err := repoCache.NewIdentity("René Descartes", "rene@descartes.fr") require.NoError(t, err) + err = repoCache.SetUserIdentity(rene) + require.NoError(t, err) + for i := 0; i < 100; i++ { - _, _, err := repoCache.NewBugRaw(rene, time.Now().Unix(), "title", fmt.Sprintf("message%v", i), nil, nil) + _, _, err := repoCache.NewBug("title", "message") require.NoError(t, err) } // and one more for testing - b1, _, err := repoCache.NewBugRaw(rene, time.Now().Unix(), "title", "message", nil, nil) + b1, _, err := repoCache.NewBug("title", "message") require.NoError(t, err) _, err = repoCache.Push("remoteA") @@ -210,3 +211,70 @@ func TestRemove(t *testing.T) { _, err = repoCache.ResolveBug(b1.Id()) assert.Error(t, bug.ErrBugNotExist, err) } + +func TestCacheEviction(t *testing.T) { + repo := repository.CreateTestRepo(false) + repoCache, err := NewRepoCache(repo) + require.NoError(t, err) + repoCache.presentBugs.Resize(2) + + require.Equal(t, 0, repoCache.presentBugs.Len()) + require.Equal(t, 0, len(repoCache.bugs)) + require.Equal(t, 0, len(repoCache.bugExcerpts)) + + // Generating some bugs + rene, err := repoCache.NewIdentity("René Descartes", "rene@descartes.fr") + require.NoError(t, err) + err = repoCache.SetUserIdentity(rene) + require.NoError(t, err) + + bug1, _, err := repoCache.NewBug("title", "message") + require.NoError(t, err) + + checkBugPresence(t, repoCache, bug1, true) + require.Equal(t, 1, repoCache.presentBugs.Len()) + require.Equal(t, 1, len(repoCache.bugs)) + require.Equal(t, 1, len(repoCache.bugExcerpts)) + + bug2, _, err := repoCache.NewBug("title", "message") + require.NoError(t, err) + + checkBugPresence(t, repoCache, bug1, true) + checkBugPresence(t, repoCache, bug2, true) + require.Equal(t, 2, repoCache.presentBugs.Len()) + require.Equal(t, 2, len(repoCache.bugs)) + require.Equal(t, 2, len(repoCache.bugExcerpts)) + + // Number of bugs should not exceed max size of lruCache, oldest one should be evicted + bug3, _, err := repoCache.NewBug("title", "message") + require.NoError(t, err) + + checkBugPresence(t, repoCache, bug1, false) + checkBugPresence(t, repoCache, bug2, true) + checkBugPresence(t, repoCache, bug3, true) + require.Equal(t, 2, repoCache.presentBugs.Len()) + require.Equal(t, 2, len(repoCache.bugs)) + require.Equal(t, 2, len(repoCache.bugExcerpts)) + + // Accessing bug should update position in lruCache and therefore it should not be evicted + repoCache.presentBugs.Get(bug2.Id()) + oldestId, _ := repoCache.presentBugs.GetOldest() + require.Equal(t, bug3.Id(), oldestId) + + checkBugPresence(t, repoCache, bug1, false) + checkBugPresence(t, repoCache, bug2, true) + checkBugPresence(t, repoCache, bug3, true) + require.Equal(t, 2, repoCache.presentBugs.Len()) + require.Equal(t, 2, len(repoCache.bugs)) + require.Equal(t, 2, len(repoCache.bugExcerpts)) +} + +func checkBugPresence(t *testing.T, cache *RepoCache, bug *BugCache, presence bool) { + id := bug.Id() + require.Equal(t, presence, cache.presentBugs.Contains(id)) + b, ok := cache.bugs[id] + require.Equal(t, presence, ok) + require.Equal(t, bug, b) + _, ok = cache.bugExcerpts[id] + require.Equal(t, presence, ok) +} diff --git a/go.mod b/go.mod index 48c1b25d79d945a3b75a4eb43e4117bd250c2c12..b257917e701e9e02db021b016e1eb264415e88a5 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/dustin/go-humanize v1.0.0 github.com/fatih/color v1.9.0 github.com/gorilla/mux v1.7.4 - github.com/hashicorp/golang-lru v0.5.4 // indirect + github.com/hashicorp/golang-lru v0.5.4 github.com/icrowley/fake v0.0.0-20180203215853-4178557ae428 github.com/mattn/go-isatty v0.0.12 github.com/phayes/freeport v0.0.0-20171002181615-b8543db493a5 diff --git a/go.sum b/go.sum index 08d8c9ed6bdc4d8d52f5734c0a05442d69e017a3..5e27863a7b393ace0d47f0808ee68490e560fa2b 100644 --- a/go.sum +++ b/go.sum @@ -109,6 +109,7 @@ github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgf github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY= github.com/hashicorp/go-cleanhttp v0.5.1 h1:dH3aiDG9Jvb5r5+bYHsikaOUIpcM0xvgMXVoDkXMzJM= github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= +github.com/hashicorp/go-hclog v0.9.2 h1:CG6TE5H9/JXsFWJCfoIVpKFIkFe6ysEuHirp4DxCsHI= github.com/hashicorp/go-hclog v0.9.2/go.mod h1:5CU+agLiy3J7N7QjHK5d05KxGsuXiQLrjA0H7acj2lQ= github.com/hashicorp/go-retryablehttp v0.6.4 h1:BbgctKO892xEyOXnGiaAwIoSq1QZ/SS4AhjoAh9DnfY= github.com/hashicorp/go-retryablehttp v0.6.4/go.mod h1:vAew36LZh98gCBJNLH42IQ1ER/9wtLZZ8meHqQvEYWY= From 4b065029af63c16ffd754ac28190ba4b3125c494 Mon Sep 17 00:00:00 2001 From: vince Date: Tue, 25 Aug 2020 10:43:42 +0800 Subject: [PATCH 2/6] Implement cache eviction and testing --- cache/bug_cache.go | 2 - cache/lru_id_cache.go | 13 +++--- cache/repo_cache.go | 16 ++----- cache/repo_cache_bug.go | 92 ++++++++++++++++++++++------------------ cache/repo_cache_test.go | 25 +++++------ 5 files changed, 73 insertions(+), 75 deletions(-) diff --git a/cache/bug_cache.go b/cache/bug_cache.go index 8365b3f9a9b5e63e175bb8d0494d1106b93897d3..ca526f7b316f49f477c38a8d1573b74e81f47fbb 100644 --- a/cache/bug_cache.go +++ b/cache/bug_cache.go @@ -37,8 +37,6 @@ func (c *BugCache) Snapshot() *bug.Snapshot { } func (c *BugCache) Id() entity.Id { - c.mu.RLock() - defer c.mu.RUnlock() return c.bug.Id() } diff --git a/cache/lru_id_cache.go b/cache/lru_id_cache.go index e31f2843f4b00110b647ca3f551475f1529ef57d..a387cb75a28dc23d0a5fc0fff6e4f2b36a6eefef 100644 --- a/cache/lru_id_cache.go +++ b/cache/lru_id_cache.go @@ -9,20 +9,16 @@ import ( type LRUIdCache struct { parentCache *lru.Cache maxSize int - onEvict func(id entity.Id) } -func NewLRUIdCache(size int, onEvicted func(id entity.Id)) (*LRUIdCache, error) { - cache, err := lru.New(size) - if err != nil { - return nil, err - } +func NewLRUIdCache(size int) *LRUIdCache { + // Ignore error here + cache, _ := lru.New(size) return &LRUIdCache{ cache, size, - onEvicted, - }, nil + } } func (c *LRUIdCache) Add(id entity.Id) bool { @@ -60,5 +56,6 @@ func (c *LRUIdCache) Remove(id entity.Id) bool { } func (c *LRUIdCache) Resize(size int) int { + c.maxSize = size return c.parentCache.Resize(size) } diff --git a/cache/repo_cache.go b/cache/repo_cache.go index 5fe1b7981eb3db6f663f7b11ce7c0688a92e17fa..f6ff1abe45636dbfbd01446b197df02f35fd0861 100644 --- a/cache/repo_cache.go +++ b/cache/repo_cache.go @@ -82,6 +82,9 @@ func NewNamedRepoCache(r repository.ClockedRepo, name string) (*RepoCache, error return &RepoCache{}, err } + presentBugs := NewLRUIdCache(lruCacheSize) + c.presentBugs = presentBugs + err = c.load() if err == nil { return c, nil @@ -180,22 +183,11 @@ func (c *RepoCache) buildCache() error { _, _ = fmt.Fprintf(os.Stderr, "Building bug cache... ") - presentBugs, err := NewLRUIdCache(lruCacheSize, c.onEvict) - if err != nil { - return err - } - c.presentBugs = presentBugs - c.bugExcerpts = make(map[entity.Id]*BugExcerpt) allBugs := bug.ReadAllLocalBugs(c.repo) - for i := 0; i < lruCacheSize; i++ { - if len(allBugs) == 0 { - break - } - - b := <-allBugs + for b := range allBugs { if b.Err != nil { return b.Err } diff --git a/cache/repo_cache_bug.go b/cache/repo_cache_bug.go index 573973619f8f7fbb90db42f95e08a69b0b5f23cd..40081e6e617151dfe6e4c2b1af4095d8a4556740 100644 --- a/cache/repo_cache_bug.go +++ b/cache/repo_cache_bug.go @@ -3,6 +3,7 @@ package cache import ( "bytes" "encoding/gob" + "errors" "fmt" "os" "path" @@ -17,6 +18,9 @@ import ( const bugCacheFile = "bug-cache" +var errBugNotInCache = errors.New("bug missing from cache") +var errCantEvictBug = errors.New("unable to evict a bug from the cache") + func bugCacheFilePath(repo repository.Repo) string { return path.Join(repo.GetPath(), "git-bug", bugCacheFile) } @@ -24,13 +28,13 @@ func bugCacheFilePath(repo repository.Repo) string { // bugUpdated is a callback to trigger when the excerpt of a bug changed, // that is each time a bug is updated func (c *RepoCache) bugUpdated(id entity.Id) error { - err := c.ensureBugLoaded(id) - if err != nil { - return err - } - c.muBug.Lock() - b, _ := c.bugs[id] + b, ok := c.bugs[id] + if !ok { + c.muBug.Unlock() + return errBugNotInCache + } + c.presentBugs.Get(id) c.bugExcerpts[id] = NewBugExcerpt(b.bug, b.Snapshot()) c.muBug.Unlock() @@ -105,14 +109,14 @@ func (c *RepoCache) writeBugCache() error { // ResolveBugExcerpt retrieve a BugExcerpt matching the exact given id func (c *RepoCache) ResolveBugExcerpt(id entity.Id) (*BugExcerpt, error) { - err := c.ensureBugLoaded(id) - if err != nil { - return nil, err - } - c.muBug.RLock() defer c.muBug.RUnlock() - return c.bugExcerpts[id], nil + excerpt, ok := c.bugExcerpts[id] + if !ok { + return nil, errBugNotInCache + } + c.presentBugs.Get(id) + return excerpt, nil } // ResolveBug retrieve a bug matching the exact given id @@ -127,6 +131,8 @@ func (c *RepoCache) ResolveBug(id entity.Id) (*BugCache, error) { return c.bugs[id], nil } +// ensureBugLoaded ensures a bug (if it exists) is loaded into the cache +// it will automatically evict a bug if needed func (c *RepoCache) ensureBugLoaded(id entity.Id) error { if c.presentBugs.Get(id) { return nil @@ -139,27 +145,40 @@ func (c *RepoCache) ensureBugLoaded(id entity.Id) error { bugCache := NewBugCache(c, b) - c.muBug.Lock() - if c.presentBugs.Len() == c.presentBugs.maxSize { - for _, id := range c.presentBugs.GetAll() { - if b := c.bugs[id]; !b.NeedCommit() { - b.mu.Lock() - c.presentBugs.Remove(id) - delete(c.bugExcerpts, id) - delete(c.bugs, id) - } - } + err = c.evictIfNeeded() + if err != nil { + return err } + c.muBug.Lock() + defer c.muBug.Unlock() c.presentBugs.Add(id) c.bugs[id] = bugCache - excerpt := NewBugExcerpt(b, bugCache.Snapshot()) // TODO: Is this needed? - c.bugExcerpts[id] = excerpt - - c.muBug.Unlock() return nil } +// evictIfNeeded will evict a bug from the cache if needed +// it also removes references of the bug from the bugs and bugExcerpts +func (c *RepoCache) evictIfNeeded() error { + c.muBug.Lock() + defer c.muBug.Unlock() + if c.presentBugs.Len() < c.presentBugs.maxSize { + return nil + } + + for _, id := range c.presentBugs.GetAll() { + if b := c.bugs[id]; !b.NeedCommit() { + b.mu.Lock() + c.presentBugs.Remove(id) + delete(c.bugExcerpts, id) + delete(c.bugs, id) + return nil + } + } + + return errCantEvictBug +} + // ResolveBugExcerptPrefix retrieve a BugExcerpt matching an id prefix. It fails if multiple // bugs match. func (c *RepoCache) ResolveBugExcerptPrefix(prefix string) (*BugExcerpt, error) { @@ -361,13 +380,15 @@ func (c *RepoCache) NewBugRaw(author *IdentityCache, unixTime int64, title strin return nil, nil, err } - c.muBug.Lock() - if _, has := c.bugs[b.Id()]; has { - c.muBug.Unlock() - return nil, nil, fmt.Errorf("bug %s already exist in the cache", b.Id()) + if other, err := c.ResolveBug(b.Id()); other != nil { + if err != nil { + return nil, nil, err + } } cached := NewBugCache(c, b) + + c.muBug.Lock() c.bugs[b.Id()] = cached c.muBug.Unlock() @@ -394,27 +415,16 @@ func (c *RepoCache) RemoveBug(prefix string) error { c.muBug.Lock() b.mu.Lock() - fmt.Println("got lock") err = bug.RemoveBug(c.repo, b.Id()) if err != nil { c.muBug.Unlock() b.mu.Unlock() return err } - fmt.Println("noerr") c.presentBugs.Remove(b.Id()) - fmt.Println("removing1") delete(c.bugs, b.Id()) - fmt.Println("removed2") delete(c.bugExcerpts, b.Id()) - fmt.Println("unlocking") c.muBug.Unlock() return c.writeBugCache() } - -// onEvict will update the bugs and bugExcerpts when a bug is evicted from the cache -func (c *RepoCache) onEvict(id entity.Id) { // TODO: Do we need this? - delete(c.bugs, id) - delete(c.bugExcerpts, id) -} diff --git a/cache/repo_cache_test.go b/cache/repo_cache_test.go index ba59a7df15add74321b668e58958f97cb690be15..426279e132371bf13d7477a0dae50f799fcdcd60 100644 --- a/cache/repo_cache_test.go +++ b/cache/repo_cache_test.go @@ -83,7 +83,8 @@ func TestCache(t *testing.T) { require.Empty(t, cache.identitiesExcerpts) // Reload, only excerpt are loaded - require.NoError(t, cache.load()) + cache, err = NewRepoCache(repo) + require.NoError(t, err) require.Empty(t, cache.bugs) require.Empty(t, cache.identities) require.Len(t, cache.bugExcerpts, 2) @@ -175,17 +176,14 @@ func TestRemove(t *testing.T) { repoCache, err := NewRepoCache(repo) require.NoError(t, err) - // generate a bunch of bugs rene, err := repoCache.NewIdentity("René Descartes", "rene@descartes.fr") require.NoError(t, err) err = repoCache.SetUserIdentity(rene) require.NoError(t, err) - for i := 0; i < 100; i++ { - _, _, err := repoCache.NewBug("title", "message") - require.NoError(t, err) - } + _, _, err = repoCache.NewBug("title", "message") + require.NoError(t, err) // and one more for testing b1, _, err := repoCache.NewBug("title", "message") @@ -205,8 +203,8 @@ func TestRemove(t *testing.T) { err = repoCache.RemoveBug(b1.Id().String()) require.NoError(t, err) - assert.Equal(t, 100, len(repoCache.bugs)) - assert.Equal(t, 100, len(repoCache.bugExcerpts)) + assert.Equal(t, 1, len(repoCache.bugs)) + assert.Equal(t, 1, len(repoCache.bugExcerpts)) _, err = repoCache.ResolveBug(b1.Id()) assert.Error(t, bug.ErrBugNotExist, err) @@ -218,6 +216,7 @@ func TestCacheEviction(t *testing.T) { require.NoError(t, err) repoCache.presentBugs.Resize(2) + require.Equal(t, 2, repoCache.presentBugs.maxSize) require.Equal(t, 0, repoCache.presentBugs.Len()) require.Equal(t, 0, len(repoCache.bugs)) require.Equal(t, 0, len(repoCache.bugExcerpts)) @@ -249,12 +248,12 @@ func TestCacheEviction(t *testing.T) { bug3, _, err := repoCache.NewBug("title", "message") require.NoError(t, err) - checkBugPresence(t, repoCache, bug1, false) - checkBugPresence(t, repoCache, bug2, true) - checkBugPresence(t, repoCache, bug3, true) require.Equal(t, 2, repoCache.presentBugs.Len()) require.Equal(t, 2, len(repoCache.bugs)) require.Equal(t, 2, len(repoCache.bugExcerpts)) + checkBugPresence(t, repoCache, bug1, false) + checkBugPresence(t, repoCache, bug2, true) + checkBugPresence(t, repoCache, bug3, true) // Accessing bug should update position in lruCache and therefore it should not be evicted repoCache.presentBugs.Get(bug2.Id()) @@ -274,7 +273,9 @@ func checkBugPresence(t *testing.T, cache *RepoCache, bug *BugCache, presence bo require.Equal(t, presence, cache.presentBugs.Contains(id)) b, ok := cache.bugs[id] require.Equal(t, presence, ok) - require.Equal(t, bug, b) + if ok { + require.Equal(t, bug, b) + } _, ok = cache.bugExcerpts[id] require.Equal(t, presence, ok) } From 4d678f3e057aea30d1396240d88e622c833a177f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Mur=C3=A9?= Date: Tue, 25 Aug 2020 15:26:23 +0200 Subject: [PATCH 3/6] cache: simplify cache eviction --- cache/lru_id_cache.go | 15 ++--- cache/repo_cache.go | 30 ++++++---- cache/repo_cache_bug.go | 115 +++++++++++++++++++-------------------- cache/repo_cache_test.go | 27 ++++----- 4 files changed, 89 insertions(+), 98 deletions(-) diff --git a/cache/lru_id_cache.go b/cache/lru_id_cache.go index a387cb75a28dc23d0a5fc0fff6e4f2b36a6eefef..f775404f05e5a627ee4c9591626a6cd4fa9094bb 100644 --- a/cache/lru_id_cache.go +++ b/cache/lru_id_cache.go @@ -1,6 +1,8 @@ package cache import ( + "math" + lru "github.com/hashicorp/golang-lru" "github.com/MichaelMure/git-bug/entity" @@ -8,16 +10,14 @@ import ( type LRUIdCache struct { parentCache *lru.Cache - maxSize int } -func NewLRUIdCache(size int) *LRUIdCache { +func NewLRUIdCache() *LRUIdCache { // Ignore error here - cache, _ := lru.New(size) + cache, _ := lru.New(math.MaxInt32) return &LRUIdCache{ cache, - size, } } @@ -39,7 +39,7 @@ func (c *LRUIdCache) GetOldest() (entity.Id, bool) { return id.(entity.Id), present } -func (c *LRUIdCache) GetAll() (ids []entity.Id) { +func (c *LRUIdCache) GetOldestToNewest() (ids []entity.Id) { interfaceKeys := c.parentCache.Keys() for _, id := range interfaceKeys { ids = append(ids, id.(entity.Id)) @@ -54,8 +54,3 @@ func (c *LRUIdCache) Len() int { func (c *LRUIdCache) Remove(id entity.Id) bool { return c.parentCache.Remove(id) } - -func (c *LRUIdCache) Resize(size int) int { - c.maxSize = size - return c.parentCache.Resize(size) -} diff --git a/cache/repo_cache.go b/cache/repo_cache.go index f6ff1abe45636dbfbd01446b197df02f35fd0861..d13ce65c0cb9e2031f16a45762f38afa86845777 100644 --- a/cache/repo_cache.go +++ b/cache/repo_cache.go @@ -21,7 +21,8 @@ import ( // 2: added cache for identities with a reference in the bug cache const formatVersion = 2 -const lruCacheSize = 100 +// The maximum number of bugs loaded in memory. After that, eviction will be done. +const defaultMaxLoadedBugs = 100 var _ repository.RepoCommon = &RepoCache{} @@ -46,14 +47,16 @@ type RepoCache struct { // the name of the repository, as defined in the MultiRepoCache name string + // maximum number of loaded bugs + maxLoadedBugs int + muBug sync.RWMutex // excerpt of bugs data for all bugs bugExcerpts map[entity.Id]*BugExcerpt // bug loaded in memory bugs map[entity.Id]*BugCache - - // presentBugs is an LRU cache that records which bugs the cache has loaded in - presentBugs *LRUIdCache + // loadedBugs is an LRU cache that records which bugs the cache has loaded in + loadedBugs *LRUIdCache muIdentity sync.RWMutex // excerpt of identities data for all identities @@ -71,10 +74,12 @@ func NewRepoCache(r repository.ClockedRepo) (*RepoCache, error) { func NewNamedRepoCache(r repository.ClockedRepo, name string) (*RepoCache, error) { c := &RepoCache{ - repo: r, - name: name, - bugs: make(map[entity.Id]*BugCache), - identities: make(map[entity.Id]*IdentityCache), + repo: r, + name: name, + maxLoadedBugs: defaultMaxLoadedBugs, + bugs: make(map[entity.Id]*BugCache), + loadedBugs: NewLRUIdCache(), + identities: make(map[entity.Id]*IdentityCache), } err := c.lock() @@ -82,9 +87,6 @@ func NewNamedRepoCache(r repository.ClockedRepo, name string) (*RepoCache, error return &RepoCache{}, err } - presentBugs := NewLRUIdCache(lruCacheSize) - c.presentBugs = presentBugs - err = c.load() if err == nil { return c, nil @@ -99,6 +101,12 @@ func NewNamedRepoCache(r repository.ClockedRepo, name string) (*RepoCache, error return c, c.write() } +// setCacheSize change the maximum number of loaded bugs +func (c *RepoCache) setCacheSize(size int) { + c.maxLoadedBugs = size + c.evictIfNeeded() +} + // load will try to read from the disk all the cache files func (c *RepoCache) load() error { err := c.loadBugCache() diff --git a/cache/repo_cache_bug.go b/cache/repo_cache_bug.go index 40081e6e617151dfe6e4c2b1af4095d8a4556740..cd60eac176f30330100370144a2f1cff41eaf3a3 100644 --- a/cache/repo_cache_bug.go +++ b/cache/repo_cache_bug.go @@ -19,7 +19,6 @@ import ( const bugCacheFile = "bug-cache" var errBugNotInCache = errors.New("bug missing from cache") -var errCantEvictBug = errors.New("unable to evict a bug from the cache") func bugCacheFilePath(repo repository.Repo) string { return path.Join(repo.GetPath(), "git-bug", bugCacheFile) @@ -32,9 +31,15 @@ func (c *RepoCache) bugUpdated(id entity.Id) error { b, ok := c.bugs[id] if !ok { c.muBug.Unlock() + + // if the bug is not loaded at this point, it means it was loaded before + // but got evicted. Which means we potentially have multiple copies in + // memory and thus concurrent write. + // Failing immediately here is the simple and safe solution to avoid + // complicated data loss. return errBugNotInCache } - c.presentBugs.Get(id) + c.loadedBugs.Get(id) c.bugExcerpts[id] = NewBugExcerpt(b.bug, b.Snapshot()) c.muBug.Unlock() @@ -111,72 +116,66 @@ func (c *RepoCache) writeBugCache() error { func (c *RepoCache) ResolveBugExcerpt(id entity.Id) (*BugExcerpt, error) { c.muBug.RLock() defer c.muBug.RUnlock() + excerpt, ok := c.bugExcerpts[id] if !ok { - return nil, errBugNotInCache + panic("missing bug in the cache") } - c.presentBugs.Get(id) + return excerpt, nil } // ResolveBug retrieve a bug matching the exact given id func (c *RepoCache) ResolveBug(id entity.Id) (*BugCache, error) { - err := c.ensureBugLoaded(id) - if err != nil { - return nil, err - } - c.muBug.RLock() - defer c.muBug.RUnlock() - return c.bugs[id], nil -} - -// ensureBugLoaded ensures a bug (if it exists) is loaded into the cache -// it will automatically evict a bug if needed -func (c *RepoCache) ensureBugLoaded(id entity.Id) error { - if c.presentBugs.Get(id) { - return nil + cached, ok := c.bugs[id] + if ok { + c.loadedBugs.Get(id) + c.muBug.RUnlock() + return cached, nil } + c.muBug.RUnlock() b, err := bug.ReadLocalBug(c.repo, id) if err != nil { - return err + return nil, err } - bugCache := NewBugCache(c, b) - - err = c.evictIfNeeded() - if err != nil { - return err - } + cached = NewBugCache(c, b) c.muBug.Lock() - defer c.muBug.Unlock() - c.presentBugs.Add(id) - c.bugs[id] = bugCache - return nil + c.bugs[id] = cached + c.loadedBugs.Add(id) + c.muBug.Unlock() + + c.evictIfNeeded() + + return cached, nil } // evictIfNeeded will evict a bug from the cache if needed -// it also removes references of the bug from the bugs and bugExcerpts -func (c *RepoCache) evictIfNeeded() error { +// it also removes references of the bug from the bugs +func (c *RepoCache) evictIfNeeded() { c.muBug.Lock() defer c.muBug.Unlock() - if c.presentBugs.Len() < c.presentBugs.maxSize { - return nil + if c.loadedBugs.Len() <= c.maxLoadedBugs { + return } - for _, id := range c.presentBugs.GetAll() { - if b := c.bugs[id]; !b.NeedCommit() { - b.mu.Lock() - c.presentBugs.Remove(id) - delete(c.bugExcerpts, id) - delete(c.bugs, id) - return nil + for _, id := range c.loadedBugs.GetOldestToNewest() { + b := c.bugs[id] + if b.NeedCommit() { + continue } - } - return errCantEvictBug + b.mu.Lock() + c.loadedBugs.Remove(id) + delete(c.bugs, id) + + if c.loadedBugs.Len() <= c.maxLoadedBugs { + return + } + } } // ResolveBugExcerptPrefix retrieve a BugExcerpt matching an id prefix. It fails if multiple @@ -380,18 +379,19 @@ func (c *RepoCache) NewBugRaw(author *IdentityCache, unixTime int64, title strin return nil, nil, err } - if other, err := c.ResolveBug(b.Id()); other != nil { - if err != nil { - return nil, nil, err - } + c.muBug.Lock() + if _, has := c.bugs[b.Id()]; has { + c.muBug.Unlock() + return nil, nil, fmt.Errorf("bug %s already exist in the cache", b.Id()) } cached := NewBugCache(c, b) - - c.muBug.Lock() c.bugs[b.Id()] = cached + c.loadedBugs.Add(b.Id()) c.muBug.Unlock() + c.evictIfNeeded() + // force the write of the excerpt err = c.bugUpdated(b.Id()) if err != nil { @@ -403,28 +403,23 @@ func (c *RepoCache) NewBugRaw(author *IdentityCache, unixTime int64, title strin // RemoveBug removes a bug from the cache and repo given a bug id prefix func (c *RepoCache) RemoveBug(prefix string) error { - b, err := c.ResolveBugPrefix(prefix) - if err != nil { - return err - } + c.muBug.RLock() - err = c.ensureBugLoaded(b.Id()) + b, err := c.ResolveBugPrefix(prefix) if err != nil { + c.muBug.RUnlock() return err } + c.muBug.RUnlock() c.muBug.Lock() - b.mu.Lock() err = bug.RemoveBug(c.repo, b.Id()) - if err != nil { - c.muBug.Unlock() - b.mu.Unlock() - return err - } - c.presentBugs.Remove(b.Id()) + delete(c.bugs, b.Id()) delete(c.bugExcerpts, b.Id()) + c.loadedBugs.Remove(b.Id()) c.muBug.Unlock() + return c.writeBugCache() } diff --git a/cache/repo_cache_test.go b/cache/repo_cache_test.go index 426279e132371bf13d7477a0dae50f799fcdcd60..c0f7f18996eb292541f9d8c86b7e403e76ae6a76 100644 --- a/cache/repo_cache_test.go +++ b/cache/repo_cache_test.go @@ -214,12 +214,11 @@ func TestCacheEviction(t *testing.T) { repo := repository.CreateTestRepo(false) repoCache, err := NewRepoCache(repo) require.NoError(t, err) - repoCache.presentBugs.Resize(2) + repoCache.setCacheSize(2) - require.Equal(t, 2, repoCache.presentBugs.maxSize) - require.Equal(t, 0, repoCache.presentBugs.Len()) + require.Equal(t, 2, repoCache.maxLoadedBugs) + require.Equal(t, 0, repoCache.loadedBugs.Len()) require.Equal(t, 0, len(repoCache.bugs)) - require.Equal(t, 0, len(repoCache.bugExcerpts)) // Generating some bugs rene, err := repoCache.NewIdentity("René Descartes", "rene@descartes.fr") @@ -231,51 +230,45 @@ func TestCacheEviction(t *testing.T) { require.NoError(t, err) checkBugPresence(t, repoCache, bug1, true) - require.Equal(t, 1, repoCache.presentBugs.Len()) + require.Equal(t, 1, repoCache.loadedBugs.Len()) require.Equal(t, 1, len(repoCache.bugs)) - require.Equal(t, 1, len(repoCache.bugExcerpts)) bug2, _, err := repoCache.NewBug("title", "message") require.NoError(t, err) checkBugPresence(t, repoCache, bug1, true) checkBugPresence(t, repoCache, bug2, true) - require.Equal(t, 2, repoCache.presentBugs.Len()) + require.Equal(t, 2, repoCache.loadedBugs.Len()) require.Equal(t, 2, len(repoCache.bugs)) - require.Equal(t, 2, len(repoCache.bugExcerpts)) // Number of bugs should not exceed max size of lruCache, oldest one should be evicted bug3, _, err := repoCache.NewBug("title", "message") require.NoError(t, err) - require.Equal(t, 2, repoCache.presentBugs.Len()) + require.Equal(t, 2, repoCache.loadedBugs.Len()) require.Equal(t, 2, len(repoCache.bugs)) - require.Equal(t, 2, len(repoCache.bugExcerpts)) checkBugPresence(t, repoCache, bug1, false) checkBugPresence(t, repoCache, bug2, true) checkBugPresence(t, repoCache, bug3, true) // Accessing bug should update position in lruCache and therefore it should not be evicted - repoCache.presentBugs.Get(bug2.Id()) - oldestId, _ := repoCache.presentBugs.GetOldest() + repoCache.loadedBugs.Get(bug2.Id()) + oldestId, _ := repoCache.loadedBugs.GetOldest() require.Equal(t, bug3.Id(), oldestId) checkBugPresence(t, repoCache, bug1, false) checkBugPresence(t, repoCache, bug2, true) checkBugPresence(t, repoCache, bug3, true) - require.Equal(t, 2, repoCache.presentBugs.Len()) + require.Equal(t, 2, repoCache.loadedBugs.Len()) require.Equal(t, 2, len(repoCache.bugs)) - require.Equal(t, 2, len(repoCache.bugExcerpts)) } func checkBugPresence(t *testing.T, cache *RepoCache, bug *BugCache, presence bool) { id := bug.Id() - require.Equal(t, presence, cache.presentBugs.Contains(id)) + require.Equal(t, presence, cache.loadedBugs.Contains(id)) b, ok := cache.bugs[id] require.Equal(t, presence, ok) if ok { require.Equal(t, bug, b) } - _, ok = cache.bugExcerpts[id] - require.Equal(t, presence, ok) } From 2440a208f6e0d18f67f4e022b1c28996ee226af0 Mon Sep 17 00:00:00 2001 From: vince Date: Wed, 26 Aug 2020 09:04:40 +0800 Subject: [PATCH 4/6] Fix bugs and cleanup code --- cache/repo_cache.go | 4 ++-- cache/repo_cache_bug.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cache/repo_cache.go b/cache/repo_cache.go index d13ce65c0cb9e2031f16a45762f38afa86845777..563fac6b5059ddc0608ff870852ae4d0b2f1fc8e 100644 --- a/cache/repo_cache.go +++ b/cache/repo_cache.go @@ -22,7 +22,7 @@ import ( const formatVersion = 2 // The maximum number of bugs loaded in memory. After that, eviction will be done. -const defaultMaxLoadedBugs = 100 +const defaultMaxLoadedBugs = 1000 var _ repository.RepoCommon = &RepoCache{} @@ -160,7 +160,7 @@ func (c *RepoCache) Close() error { c.identities = make(map[entity.Id]*IdentityCache) c.identitiesExcerpts = nil - c.bugs = nil + c.bugs = make(map[entity.Id]*BugCache) c.bugExcerpts = nil lockPath := repoLockFilePath(c.repo) diff --git a/cache/repo_cache_bug.go b/cache/repo_cache_bug.go index cd60eac176f30330100370144a2f1cff41eaf3a3..37b91c548b8c2fc9dbe1dc4b1b5085c336ec7ccc 100644 --- a/cache/repo_cache_bug.go +++ b/cache/repo_cache_bug.go @@ -119,7 +119,7 @@ func (c *RepoCache) ResolveBugExcerpt(id entity.Id) (*BugExcerpt, error) { excerpt, ok := c.bugExcerpts[id] if !ok { - panic("missing bug in the cache") + return nil, bug.ErrBugNotExist } return excerpt, nil From 8a4e4a1290bc49ea9ac196f96ff5140ddb871eba Mon Sep 17 00:00:00 2001 From: Vincent Tiu <46623413+Invincibot@users.noreply.github.com> Date: Thu, 27 Aug 2020 20:10:10 +0800 Subject: [PATCH 5/6] Update cache/lru_id_cache.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Michael Muré --- cache/lru_id_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cache/lru_id_cache.go b/cache/lru_id_cache.go index f775404f05e5a627ee4c9591626a6cd4fa9094bb..fda12ca6a54b8f86b712ae1eb5e5fa2902f7430d 100644 --- a/cache/lru_id_cache.go +++ b/cache/lru_id_cache.go @@ -13,7 +13,7 @@ type LRUIdCache struct { } func NewLRUIdCache() *LRUIdCache { - // Ignore error here + // we can ignore the error here as it would only fail if the size is negative. cache, _ := lru.New(math.MaxInt32) return &LRUIdCache{ From 98a1d831f0672f118ccded5411027e4aad4cae94 Mon Sep 17 00:00:00 2001 From: vince Date: Thu, 27 Aug 2020 20:09:55 +0800 Subject: [PATCH 6/6] Delete EquivalentBug function --- bug/bug.go | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/bug/bug.go b/bug/bug.go index 3a770881b6bcaa3c8a3f39618373300b08716372..2ee890310814ab8e62e47ef7fab77a437eb3a0c6 100644 --- a/bug/bug.go +++ b/bug/bug.go @@ -749,22 +749,3 @@ func (bug *Bug) Compile() Snapshot { return snap } - -// EquivalentBug returns true if two bugs are equal -func EquivalentBug(expected, actual *Bug) bool { - if len(expected.packs) != len(actual.packs) { - return false - } - - for i := range expected.packs { - for j := range expected.packs[i].Operations { - actual.packs[i].Operations[j].base().id = expected.packs[i].Operations[j].base().id - } - } - - if expected != actual { - return false - } - - return true -}