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()
}
Implement cache eviction and testing
vince created
Change summary
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(-)
Detailed changes
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)
}
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
}
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)
-}
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)
}