gps: source cache: protobuf integration #1127

jmank88 · 2017-09-05T15:27:18Z

What does this do / why do we need it?

This change replaces most of the custom bolt cache encoding with Protocol Buffers, and includes some other optimizations as well (smaller and re-usable constant keys, smaller 'set' keys, Batch updates to bolt).

What should your reviewer look out for in this PR?

Optimizations. Correctness.

Which issue(s) does this PR fix?

Follow up from #1098 in the series towards #431

jmank88 · 2017-09-05T15:29:12Z

Gopkg.toml

+
+[[constraint]]
+  name = "github.com/jmank88/nuts"
+  version = "0.2.0"


We don't have to vendor this if we don't want to pollute. Usages are few enough to copy in.

i'm fine with bringing it in

jmank88 · 2017-09-05T15:32:56Z

internal/gps/source_cache.proto

+	}
+	Type type = 1;
+	string value = 2;
+	//TODO strongly typed Semver field


We're set up to drop whatever sort of Semver message type we come up with into here (and in a forward compatible way, if we want to commit first).

cool cool, ok - but right now, things are inoperable with this missing, right?

It is operable - as much as the tests validate anyways. It is using string serialization into the value field for now.

jmank88 · 2017-09-05T15:37:52Z

Codeclimate does not like protobuf

jmank88 · 2017-09-08T12:46:09Z

CI checks are blocked by golang/go#21804 right now, but we can likely work around it by redefining some methods as functions, if we need to.
Edit: No longer blocked.

sdboyer · 2017-09-11T18:47:29Z

CI checks are blocked by golang/go#21804 right now, but we can likely work around it by redefining some methods as functions, if we need to.

if we feel the methods are better than the funcs for our design, then i'd prefer we specify an exception to staticcheck for this particular error. if you look back over previous versions of .travis.yml, i know we used to do this for other problems.

sdboyer

couple quick things, not a comprehensive review

sdboyer · 2017-09-11T18:47:57Z

Gopkg.toml

+
+[[constraint]]
+  name = "github.com/jmank88/nuts"
+  version = "0.2.0"


i'm fine with bringing it in

sdboyer · 2017-09-11T18:53:48Z

internal/gps/source_cache.proto

+	}
+	Type type = 1;
+	string value = 2;
+	//TODO strongly typed Semver field


cool cool, ok - but right now, things are inoperable with this missing, right?

sdboyer · 2017-09-11T19:17:38Z

internal/gps/source_cache.pb.go

+}
+
+// LockedProjectMsg is a serializable representation of LockedProject.
+type LockedProjectMsg struct {


and this has to be exported, right? can we rearrange to an internal subpackage or something?

Shouldn't be a problem to put it in an internal subpackage. That might indirectly dodge the go/types problem too.

jmank88 · 2017-09-12T16:50:00Z

@sdboyer Ok if I squash/rebase and force push to clean up the prune mess?
Can wait for a quick look at e78bc49 first if you want.

Edit: Did this

jmank88 · 2017-09-13T12:50:02Z

What are the license header rules regarding generated files? Should I find a way to inject a header into source_cache.pb.go? Or do we just exclude *.pb.go files from the license check?

This file does not have a license header, whatever that means: https://github.com/golang/protobuf/blob/master/proto/testdata/test.pb.go

jmank88 · 2017-09-18T12:36:04Z

hack/validate-licence.bash

@@ -8,5 +8,5 @@
 set -e

 go build ./hack/licenseok
-find . -path ./vendor -prune -o -type f -name "*.go"\
+find . -path ./vendor -prune -o -regex ".+\.pb\.go$" -prune -o -type f -regex ".*\.\(go\|proto\)$"\


The intention was to include *.proto files and exclude the generated *.pb.go files (but not pb.go!). I stumbled my way into this, but perhaps there is a simpler or more readable way to do so.

i have no immediate ideas about better ways of doing this; i'm fine with a slightly wonky regex.

jmank88 · 2017-09-18T12:40:36Z

.codeclimate.yml

@@ -15,3 +15,4 @@ exclude_paths:
  - internal/gps/_testdata
  - cmd/dep/testdata
  - testdata
+  - internal/gps/internal/pb


Something like **.pb.go (if legal here) would be more general and continue to work as things change.

sdboyer

a few more incremental notes here

sdboyer · 2017-09-20T01:59:24Z

internal/gps/source_cache_bolt.go

@@ -73,31 +74,31 @@ func (c *boltCache) close() error {
 //
 // 1) Versions buckets hold version keys with revision values:
 //
-//	Bucket: "versions:<timestamp>"
-//	Keys: "branch:<branch>", "defaultBranch:<branch>", "ver:<version>"
+//	Bucket: "v<timestamp>"


it may be worth not having sub-buckets for each of these: boltdb/bolt#422 (comment)

for now i tend to prefer uniformity, but it's something we could explore with benchmarks later on, if we feel so inclined.

The primary motivation was being able to delete entire buckets in a single op, rather than prefix scanning and deleting each key. Another was avoiding repeated, long, complex key-prefixes. Perhaps the Revision-versions buckets have few enough entries to yield speed up from prefixes, but most other buckets are quite large and/or would have keys much longer than their values.

sdboyer · 2017-09-20T02:06:04Z

internal/gps/source_cache_bolt_encode.go

-	}
-}
+var (
+	cacheKeyC = []byte("c")


it's fine to use the abbreviated key for efficiency reasons, but let's use full words rather than abbreviations in the consts.

They don't map 1-1, which is why I opted to name them what they are, not how they are used.

Is this better?

var ( cacheKeyComment = []byte("c") cacheKeyConstraint = cacheKeyComment cacheKeyError = []byte("e") cacheKeyHash = []byte("h") cacheKeyIgnored = []byte("i") cacheKeyImport = cacheKeyIgnored cacheKeyLock = []byte("l") cacheKeyName = []byte("n") cacheKeyOverride = []byte("o") cacheKeyPTree = []byte("p") cacheKeyRequired = []byte("r") cacheKeyRevision = cacheKeyRequired cacheKeyTestImport = []byte("t") cacheRevision = byte('r') cacheVersion = byte('v') )

I wish this all could have dropped into a separate cache package to avoid all the annoying prefixes.

(Regarding splitting packages, IIRC it would require at least (maybe only?) pushing version stuff into a separate gps/version package to avoid cycles)

ah yeah, i see. in this case, though, a bit of duplication for the reader's sake is acceptable.

(Regarding splitting packages, IIRC it would require at least (maybe only?) pushing version stuff into a separate gps/version package to avoid cycles)

i've considered such a thing quite a bit, but i don't think we can currently manage it because of all the reliance on hidden properties. i'd be open to refactoring that and just exporting everything - i was probably defending against a bogeyman by not exporting it - but i don't want to make that a yak we have to shave for this.

I started a separate (from master) experimental conversion, leveraging aliases quite heavily for now to minimize churn, and it's not actually too bad - plus some of the API ends up cleaner. I dropped in a few exported helper functions, rather than exporting all the internals. Not that helper functions are necessarily a better solution than exporting the types, but it proved the concept without too much refactoring.

This can all be considered separately from this PR and caching in general though.

sdboyer · 2017-09-20T02:14:28Z

internal/gps/source_cache_bolt_encode.go

@@ -203,9 +174,13 @@ func cacheGetManifest(b *bolt.Bucket) (RootManifest, error) {
 	}


premature optimization question: does a cheap call exist that allows us to get the number of items in each of the buckets, so that we can provide a capacity hint on the make calls?

There are all sorts of BucketStats available via Stats() but they have to be computed so I didn't explore that path.

For the ones using sequential integer keys (behaving like sets), we could theoretically inspect the last key to get the total items. I don't know how the costs of jumping the cursor around, or scanning backwards, compares to re-allocating the slice.

More generally, and perhaps simply, we could just store size hints in additional top level keys.

size hints as additional top-level keys is interesting. seems like a basically zero-cost way of doing things - that is, as long as the use pattern continues to be that these buckets tend to get dropped and wholly regenerated, rather than incrementally updated. if incremental updates become more the norm, then i'd be concerned that we're create a write amplifier. that might not be that costly, especially if the workflow is heavily read-oriented (which, of course it should be), but...well, with all these considerations, we'd need benchmarks!

probably best to leave it as-is for now, though, and put a big TODO on it noting the possible future direction to explore.

sdboyer · 2017-09-21T05:21:38Z

ok yeah, add that TODO then we can merge

sdboyer · 2017-09-21T05:21:50Z

pre-emptively approving

jmank88 requested a review from sdboyer as a code owner September 5, 2017 15:27

googlebot added the cla: yes label Sep 5, 2017

jmank88 commented Sep 5, 2017

View reviewed changes

jmank88 mentioned this pull request Sep 7, 2017

staticcheck "couldn't load packages due to errors" but go test/build run fine dominikh/go-tools#169

Closed

dominikh mentioned this pull request Sep 8, 2017

go/types: rejects valid program involving interfaces and cycles golang/go#21804

Closed

ibrasho added the area: gps label Sep 9, 2017

sdboyer reviewed Sep 12, 2017

View reviewed changes

jmank88 added 2 commits September 15, 2017 10:50

gps: source cache: protobuf integration

8008324

hack: update validate-license to include *.proto and exclude *.pb.go

58a88e9

jmank88 commented Sep 18, 2017

View reviewed changes

sdboyer suggested changes Sep 20, 2017

View reviewed changes

sdboyer approved these changes Sep 21, 2017

View reviewed changes

expand cache keys; note future optimizations

d67284a

jmank88 merged commit 59eca1b into golang:master Sep 22, 2017

jmank88 deleted the source_cache_proto branch September 22, 2017 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gps: source cache: protobuf integration #1127

gps: source cache: protobuf integration #1127

jmank88 commented Sep 5, 2017

jmank88 Sep 5, 2017

sdboyer Sep 11, 2017

jmank88 Sep 5, 2017

sdboyer Sep 11, 2017

jmank88 Sep 12, 2017

jmank88 commented Sep 5, 2017

jmank88 commented Sep 8, 2017 •

edited

Loading

sdboyer commented Sep 11, 2017

sdboyer left a comment

sdboyer Sep 11, 2017

sdboyer Sep 11, 2017

sdboyer Sep 11, 2017

jmank88 Sep 12, 2017

jmank88 commented Sep 12, 2017 •

edited

Loading

jmank88 commented Sep 13, 2017 •

edited

Loading

jmank88 Sep 18, 2017 •

edited

Loading

sdboyer Sep 20, 2017

jmank88 Sep 18, 2017

sdboyer left a comment

sdboyer Sep 20, 2017

jmank88 Sep 20, 2017

sdboyer Sep 20, 2017

jmank88 Sep 20, 2017 •

edited

Loading

jmank88 Sep 20, 2017

sdboyer Sep 21, 2017

jmank88 Sep 21, 2017 •

edited

Loading

sdboyer Sep 20, 2017

jmank88 Sep 20, 2017

sdboyer Sep 21, 2017

sdboyer commented Sep 21, 2017

sdboyer commented Sep 21, 2017

		@@ -203,9 +174,13 @@ func cacheGetManifest(b *bolt.Bucket) (RootManifest, error) {
		}

gps: source cache: protobuf integration #1127

gps: source cache: protobuf integration #1127

Conversation

jmank88 commented Sep 5, 2017

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Which issue(s) does this PR fix?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 commented Sep 5, 2017

jmank88 commented Sep 8, 2017 • edited Loading

sdboyer commented Sep 11, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 commented Sep 12, 2017 • edited Loading

jmank88 commented Sep 13, 2017 • edited Loading

jmank88 Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Sep 21, 2017

sdboyer commented Sep 21, 2017

jmank88 commented Sep 8, 2017 •

edited

Loading

jmank88 commented Sep 12, 2017 •

edited

Loading

jmank88 commented Sep 13, 2017 •

edited

Loading

jmank88 Sep 18, 2017 •

edited

Loading

jmank88 Sep 20, 2017 •

edited

Loading

jmank88 Sep 21, 2017 •

edited

Loading