From ca3d62d6aa3dd01dc075b4eeb2e52b8f8c0e29a6 Mon Sep 17 00:00:00 2001 From: Jordan Krage Date: Sat, 23 Sep 2017 10:10:07 -0500 Subject: [PATCH 1/2] gps: source cache: optimize and fix mem cache getAllVersions() via slice store-and-copy and proper locking --- internal/gps/source_cache.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/internal/gps/source_cache.go b/internal/gps/source_cache.go index 5724c613fa..cfc610839e 100644 --- a/internal/gps/source_cache.go +++ b/internal/gps/source_cache.go @@ -61,9 +61,10 @@ type singleSourceCache interface { } type singleSourceCacheMemory struct { - mut sync.RWMutex // protects all maps + mut sync.RWMutex // protects all fields infos map[ProjectAnalyzerInfo]map[Revision]projectInfo ptrees map[Revision]pkgtree.PackageTree + vList []PairedVersion // replaced, never modified vMap map[UnpairedVersion]Revision rMap map[Revision][]UnpairedVersion } @@ -136,13 +137,14 @@ func (c *singleSourceCacheMemory) getPackageTree(r Revision) (pkgtree.PackageTre func (c *singleSourceCacheMemory) setVersionMap(versionList []PairedVersion) { c.mut.Lock() + c.vList = versionList // TODO(sdboyer) how do we handle cache consistency here - revs that may // be out of date vis-a-vis the ptrees or infos maps? for r := range c.rMap { c.rMap[r] = nil } - c.vMap = make(map[UnpairedVersion]Revision) + c.vMap = make(map[UnpairedVersion]Revision, len(versionList)) for _, pv := range versionList { u, r := pv.Unpair(), pv.Revision() @@ -168,14 +170,16 @@ func (c *singleSourceCacheMemory) getVersionsFor(r Revision) ([]UnpairedVersion, } func (c *singleSourceCacheMemory) getAllVersions() []PairedVersion { - if len(c.vMap) == 0 { + c.mut.Lock() + vList := c.vList + c.mut.Unlock() + + if vList == nil { return nil } - vlist := make([]PairedVersion, 0, len(c.vMap)) - for v, r := range c.vMap { - vlist = append(vlist, v.Pair(r)) - } - return vlist + cp := make([]PairedVersion, len(vList)) + copy(cp, vList) + return cp } func (c *singleSourceCacheMemory) getRevisionFor(uv UnpairedVersion) (Revision, bool) { From 31ede573e7dde7552511331687533ac07314baf2 Mon Sep 17 00:00:00 2001 From: Jordan Krage Date: Sat, 23 Sep 2017 10:18:32 -0500 Subject: [PATCH 2/2] gps: source cache: add ok bool to singleSourceCache.getAllVersions() in order to avoid depending on len(0) vs nil distinction --- internal/gps/source.go | 6 ++++-- internal/gps/source_cache.go | 8 ++++---- internal/gps/source_cache_bolt.go | 8 ++++---- internal/gps/source_cache_bolt_test.go | 16 ++++++++-------- internal/gps/source_cache_multi.go | 16 ++++++++-------- internal/gps/source_cache_test.go | 8 ++++---- internal/gps/source_test.go | 4 ++-- 7 files changed, 34 insertions(+), 32 deletions(-) diff --git a/internal/gps/source.go b/internal/gps/source.go index 3ffab80217..737676adf4 100644 --- a/internal/gps/source.go +++ b/internal/gps/source.go @@ -494,8 +494,10 @@ func (sg *sourceGateway) listVersions(ctx context.Context) ([]PairedVersion, err if err != nil { return nil, err } - - return sg.cache.getAllVersions(), nil + if pvs, ok := sg.cache.getAllVersions(); ok { + return pvs, nil + } + return nil, nil } func (sg *sourceGateway) revisionPresentIn(ctx context.Context, r Revision) (bool, error) { diff --git a/internal/gps/source_cache.go b/internal/gps/source_cache.go index cfc610839e..0f285ec341 100644 --- a/internal/gps/source_cache.go +++ b/internal/gps/source_cache.go @@ -43,7 +43,7 @@ type singleSourceCache interface { getVersionsFor(Revision) ([]UnpairedVersion, bool) // Gets all the version pairs currently known to the cache. - getAllVersions() []PairedVersion + getAllVersions() ([]PairedVersion, bool) // Get the revision corresponding to the given unpaired version. getRevisionFor(UnpairedVersion) (Revision, bool) @@ -169,17 +169,17 @@ func (c *singleSourceCacheMemory) getVersionsFor(r Revision) ([]UnpairedVersion, return versionList, has } -func (c *singleSourceCacheMemory) getAllVersions() []PairedVersion { +func (c *singleSourceCacheMemory) getAllVersions() ([]PairedVersion, bool) { c.mut.Lock() vList := c.vList c.mut.Unlock() if vList == nil { - return nil + return nil, false } cp := make([]PairedVersion, len(vList)) copy(cp, vList) - return cp + return cp, true } func (c *singleSourceCacheMemory) getRevisionFor(uv UnpairedVersion) (Revision, bool) { diff --git a/internal/gps/source_cache_bolt.go b/internal/gps/source_cache_bolt.go index bf5c82d1dc..f2bae19738 100644 --- a/internal/gps/source_cache_bolt.go +++ b/internal/gps/source_cache_bolt.go @@ -351,8 +351,7 @@ func (s *singleSourceCacheBolt) getVersionsFor(rev Revision) (uvs []UnpairedVers return } -func (s *singleSourceCacheBolt) getAllVersions() []PairedVersion { - var pvs []PairedVersion +func (s *singleSourceCacheBolt) getAllVersions() (pvs []PairedVersion, ok bool) { err := s.viewSourceBucket(func(src *bolt.Bucket) error { versions := cacheFindLatestValid(src, cacheVersion, s.epoch) if versions == nil { @@ -369,14 +368,15 @@ func (s *singleSourceCacheBolt) getAllVersions() []PairedVersion { return err } pvs = append(pvs, uv.Pair(Revision(v))) + ok = true return nil }) }) if err != nil { s.logger.Println(errors.Wrap(err, "failed to get all cached versions")) - return nil + return nil, false } - return pvs + return } func (s *singleSourceCacheBolt) getRevisionFor(uv UnpairedVersion) (rev Revision, ok bool) { diff --git a/internal/gps/source_cache_bolt_test.go b/internal/gps/source_cache_bolt_test.go index d12cbf960e..d2d2d16710 100644 --- a/internal/gps/source_cache_bolt_test.go +++ b/internal/gps/source_cache_bolt_test.go @@ -119,8 +119,8 @@ func TestBoltCacheTimeout(t *testing.T) { } comparePackageTree(t, ptree, got) - gotV := c.getAllVersions() - if len(gotV) != len(pvs) { + gotV, ok := c.getAllVersions() + if !ok || len(gotV) != len(pvs) { t.Errorf("unexpected versions:\n\t(GOT): %#v\n\t(WNT): %#v", gotV, pvs) } else { SortPairedForDowngrade(gotV) @@ -161,8 +161,8 @@ func TestBoltCacheTimeout(t *testing.T) { } comparePackageTree(t, ptree, gotPtree) - pvs := c.getAllVersions() - if len(pvs) > 0 { + pvs, ok := c.getAllVersions() + if ok || len(pvs) > 0 { t.Errorf("expected no cached versions, but got:\n\t%#v", pvs) } } @@ -194,8 +194,8 @@ func TestBoltCacheTimeout(t *testing.T) { } comparePackageTree(t, ptree, got) - gotV := c.getAllVersions() - if len(gotV) != len(pvs) { + gotV, ok := c.getAllVersions() + if !ok || len(gotV) != len(pvs) { t.Errorf("unexpected versions:\n\t(GOT): %#v\n\t(WNT): %#v", gotV, pvs) } else { SortPairedForDowngrade(gotV) @@ -282,8 +282,8 @@ func TestBoltCacheTimeout(t *testing.T) { } comparePackageTree(t, newPtree, got) - gotV := c.getAllVersions() - if len(gotV) != len(newPVS) { + gotV, ok := c.getAllVersions() + if !ok || len(gotV) != len(newPVS) { t.Errorf("unexpected versions:\n\t(GOT): %#v\n\t(WNT): %#v", gotV, newPVS) } else { SortPairedForDowngrade(gotV) diff --git a/internal/gps/source_cache_multi.go b/internal/gps/source_cache_multi.go index 5ce9108d0f..01ade0ff8a 100644 --- a/internal/gps/source_cache_multi.go +++ b/internal/gps/source_cache_multi.go @@ -77,19 +77,19 @@ func (c *multiCache) getVersionsFor(rev Revision) ([]UnpairedVersion, bool) { return c.disk.getVersionsFor(rev) } -func (c *multiCache) getAllVersions() []PairedVersion { - pvs := c.mem.getAllVersions() - if pvs != nil { - return pvs +func (c *multiCache) getAllVersions() ([]PairedVersion, bool) { + pvs, ok := c.mem.getAllVersions() + if ok { + return pvs, true } - pvs = c.disk.getAllVersions() - if pvs != nil { + pvs, ok = c.disk.getAllVersions() + if ok { c.mem.setVersionMap(pvs) - return pvs + return pvs, true } - return nil + return nil, false } func (c *multiCache) getRevisionFor(uv UnpairedVersion) (Revision, bool) { diff --git a/internal/gps/source_cache_test.go b/internal/gps/source_cache_test.go index d9f2dd3982..7331b4396a 100644 --- a/internal/gps/source_cache_test.go +++ b/internal/gps/source_cache_test.go @@ -305,8 +305,8 @@ func (test singleSourceCacheTest) run(t *testing.T) { } t.Run("getAllVersions", func(t *testing.T) { - got := c.getAllVersions() - if len(got) != len(versions) { + got, ok := c.getAllVersions() + if !ok || len(got) != len(versions) { t.Errorf("unexpected versions:\n\t(GOT): %#v\n\t(WNT): %#v", got, versions) } else { SortPairedForDowngrade(got) @@ -566,8 +566,8 @@ func (discardCache) getVersionsFor(Revision) ([]UnpairedVersion, bool) { return nil, false } -func (discardCache) getAllVersions() []PairedVersion { - return nil +func (discardCache) getAllVersions() ([]PairedVersion, bool) { + return nil, false } func (discardCache) getRevisionFor(UnpairedVersion) (Revision, bool) { diff --git a/internal/gps/source_test.go b/internal/gps/source_test.go index 6614612840..639bec5dba 100644 --- a/internal/gps/source_test.go +++ b/internal/gps/source_test.go @@ -60,8 +60,8 @@ func testSourceGateway(t *testing.T) { t.Fatalf("error on cloning git repo: %s", err) } - cvlist := sg.cache.getAllVersions() - if len(cvlist) != 4 { + cvlist, ok := sg.cache.getAllVersions() + if !ok || len(cvlist) != 4 { t.Fatalf("repo setup should've cached four versions, got %v: %s", len(cvlist), cvlist) }