kv cleanups #28

phritz · 2020-04-02T18:28:02Z

this is the first batch of kv cleanups. note there are separate logical commits.

in the last commit to make construction of maps more efficient (stop copying noms maps to new noms maps) i uncovered several bugs related to newlines in encoded values, and fixed them. eg we were including newlines in the Operation.Value when generating a diff, checksumming over the newline byte, etc.

Progress towards rocicorp/replicache#22

vercel · 2020-04-02T18:28:06Z

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/rocicorp/diff-server/718snyl1c
✅ Preview: https://diff-server-git-kv.rocicorp.now.sh

aboodman

OK I didn't look at the tests closely, but red flag on the stuff with canonicalization and correctness. Maybe we should just bite the bullet and write the canonicalizer now. Or re-canon in Remove and Diff ?

aboodman · 2020-04-02T19:33:44Z

kv/checksum_test.go

+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.wantPanic {


Test you got the panic you were expecting? There are lots of reasons things might panic (I've actually gotten bitten by tests like this before).

aboodman · 2020-04-02T19:35:30Z

kv/map.go

@@ -16,31 +18,55 @@ import (
 type Map struct {
 	noms types.ValueReadWriter
 	nm   types.Map
-	c    Checksum
+	Sum  Checksum


Oooh a totally public field. Bold. Daring. But this implies caller is allowed to set Sum to some totally different value. That doesn't seem like what we want.

you're right. done.

aboodman · 2020-04-02T19:47:01Z

kv/map.go

+func (m Map) Empty() bool {
+	return m.nm.Empty()
+}
+
 func bytesFromNomsValue(value types.Valuable) ([]byte, error) {
 	// Here we could check value.Kind() if we wanted.


ToJSON does that error handling for us, so this comment can go away.

kv/map.go

aboodman · 2020-04-02T19:52:43Z

kv/map.go

+	return me.Build()
+}
+
+// WrapMap wraps a noms Map with additional logic replicache needs.


Suggested change

// WrapMap wraps a noms Map with additional logic replicache needs.

// FromNoms creates a map from an existing Noms Map and Checksum.

kv/map.go

aboodman · 2020-04-02T20:30:10Z

kv/map.go

+		if err != nil {
+			return Map{}, fmt.Errorf("failed to convert value for key %s to json", k)
+		}
+		if err := me.SetCanonicalized(k, canonicalizedValue); err != nil {


I think this function and SetCanonicalized can go away:

Change definition of types.ClientViewResponse.ClientView to map[string]json.RawMessage

When processing clientView iterate the decoded map[string]json.RawMessage and call editor.Set repeatedly.

There is no performance benefit that I can see of current method to what I proposed above, and it reuses more code.

kv/map.go

kv/patch.go

aboodman · 2020-04-02T21:27:43Z

kv/map.go

@@ -49,6 +75,7 @@ func (m Map) NomsMap() types.Map {
 }

 // Get returns the json bytes for the given key, which must exist.
+// Note the json bytes gotten include a trailing newline.


I'm sorry I totally forgot that our ToJSON canonicalized. In that case, I agree with you that that function and kv.Map should be committing to return canonical JSON, including stripping the newline. So sorry.

ok will put this in a followup

aboodman · 2020-04-02T21:34:37Z

kv/map.go

 func (me *MapEditor) Set(key string, value []byte) error {
+	var v interface{}


Why not rely on the fact that Get() canonicalizes?

The implementation of MapEditor.Set could be:

Call nomsjson.FromJSON to decode to a Noms value

Call nomsjson.ToJSON to get back a canonical bytearray

Compute the checksum

Store the Noms value

because you have no guarantee that that noms value from step 1 is going to be identical to the noms value you'd get if you parsed the canonical json from step 2. in fact, it will not be identical in many cases eg if the original value has unnormalized strings.

i added a comment to this effect. this stuff is trickier than it looks.

aboodman · 2020-04-02T21:35:22Z

kv/map.go

+}
+
+// SetCanonicalized sets the value for a given key. It assumes value is canonicalized.
+func (me *MapEditor) SetCanonicalized(key string, value []byte) error {


I think this can go away. See comment at NewMapFromPile.

aboodman · 2020-04-02T21:51:40Z

kv/map.go

+	Sum  Checksum
+}
+
+// NewMap returns a new Map. This constructor will panic if there is


This function is only used by tests, and is inefficient due to the copy from string to []byte. Maybe just move it into a test file to avoid having it accidentally be considered a part of the real interface.

its used by several packages so i renamed it to NewMapForTest and placed it in a separate file called testing.go.

aboodman · 2020-04-02T21:52:23Z

kv/map.go

+		if err != nil {
+			return Map{}, fmt.Errorf("failed to convert value for key %s to json", k)
+		}
+		if err := me.SetCanonicalized(k, canonicalizedValue); err != nil {


There is no performance benefit that I can see of current method to what I proposed above, and it reuses more code.

phritz added 4 commits April 1, 2020 10:42

have Commit.Data return a kv.Map

a6ddc4b

db.Put uses a kv.Map

9c8e438

make map checksum a little more ergonomic

f3f7365

make Map construction more efficient and fix checksum bugs revealed

57aaa84

update comment

c541eb7

phritz requested a review from aboodman April 2, 2020 18:33

vercel bot deployed to Preview April 2, 2020 18:33 View deployment

aboodman reviewed Apr 2, 2020

View reviewed changes

address code review comments

88bae50

vercel bot deployed to Preview April 3, 2020 00:35 View deployment

phritz merged commit a846986 into master Apr 3, 2020

phritz deleted the kv branch April 3, 2020 00:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kv cleanups #28

kv cleanups #28

phritz commented Apr 2, 2020 •

edited

Loading

vercel bot commented Apr 2, 2020 •

edited

Loading

aboodman left a comment •

edited

Loading

aboodman Apr 2, 2020

phritz Apr 2, 2020

aboodman Apr 2, 2020

phritz Apr 2, 2020

aboodman Apr 2, 2020

phritz Apr 3, 2020

aboodman Apr 2, 2020

phritz Apr 2, 2020

aboodman Apr 2, 2020 •

edited

Loading

aboodman Apr 2, 2020

phritz Apr 3, 2020

aboodman Apr 2, 2020

phritz Apr 3, 2020

aboodman Apr 2, 2020

phritz Apr 2, 2020

phritz Apr 3, 2020

aboodman Apr 2, 2020

phritz Apr 3, 2020

aboodman Apr 2, 2020

phritz Apr 2, 2020

aboodman Apr 2, 2020

	// WrapMap wraps a noms Map with additional logic replicache needs.
	// FromNoms creates a map from an existing Noms Map and Checksum.

		func (me *MapEditor) Set(key string, value []byte) error {
		var v interface{}

kv cleanups #28

kv cleanups #28

Conversation

phritz commented Apr 2, 2020 • edited Loading

vercel bot commented Apr 2, 2020 • edited Loading

aboodman left a comment • 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

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

aboodman Apr 2, 2020 • 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

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

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

phritz commented Apr 2, 2020 •

edited

Loading

vercel bot commented Apr 2, 2020 •

edited

Loading

aboodman left a comment •

edited

Loading

aboodman Apr 2, 2020 •

edited

Loading