Optimize merkledb hashing

[StephenButtolph]

Why this should be merged

Before:

Benchmark_CalculateID/empty_node-12         	 5420020	       219.5 ns/op	     144 B/op	       1 allocs/op
Benchmark_CalculateID/has_value-12          	 5191927	       233.1 ns/op	     144 B/op	       1 allocs/op
Benchmark_CalculateID/1_child-12            	 3997491	       297.8 ns/op	     177 B/op	       2 allocs/op
Benchmark_CalculateID/2_children-12         	 3156843	       379.3 ns/op	     210 B/op	       2 allocs/op
Benchmark_CalculateID/16_children-12        	  815900	      1515   ns/op	     720 B/op	       2 allocs/op

After:

Benchmark_CalculateID/empty_node-12         	11578461	       101.8 ns/op	       0 B/op	       0 allocs/op
Benchmark_CalculateID/has_value-12          	10477825	       113.2 ns/op	       0 B/op	       0 allocs/op
Benchmark_CalculateID/1_child-12            	 7845493	       155.4 ns/op	       0 B/op	       0 allocs/op
Benchmark_CalculateID/2_children-12         	 5576605	       215.0 ns/op	       0 B/op	       0 allocs/op
Benchmark_CalculateID/16_children-12        	 1000000	      1009   ns/op	       0 B/op	       0 allocs/op

How this works

Allocs currently happen in 2 places.

1. For the bytes.Buffer
2. For the maps.Keys

• By writing directly into the sha256 writer, we can avoid allocating the bytes.Buffer.
• By allocating a slice large enough for the biggest supported branch factor, we can avoid allocating the key slice on the heap (it is placed on the stack).

How this was tested

• Added benchmark
• CI

[patrick-ogrady]

On my benchmarks, I found this improved e2e performance by ~10-20%.

[StephenButtolph]

Tested this against current master with:

func TestHashNode(t *testing.T) {
	tests := []struct {
		name         string
		n            *node
		expectedHash string
	}{
		{
			name:         "empty node",
			n:            newNode(Key{}),
			expectedHash: "rbhtxoQ1DqWHvb6w66BZdVyjmPAneZUSwQq9uKj594qvFSdav",
		},
		{
			name: "has value",
			n: func() *node {
				n := newNode(Key{})
				n.setValue(maybe.Some([]byte("value1")))
				return n
			}(),
			expectedHash: "2vx2xueNdWoH2uB4e8hbMU5jirtZkZ1c3ePCWDhXYaFRHpCbnQ",
		},
		{
			name:         "has key",
			n:            newNode(ToKey([]byte{0, 1, 2, 3, 4, 5, 6, 7})),
			expectedHash: "2vA8ggXajhFEcgiF8zHTXgo8T2ALBFgffp1xfn48JEni1Uj5uK",
		},
		{
			name: "1 child",
			n: func() *node {
				n := newNode(Key{})
				childNode := newNode(ToKey([]byte{255}))
				childNode.setValue(maybe.Some([]byte("value1")))
				n.addChildWithID(childNode, 4, childNode.calculateID(&mockMetrics{}))
				return n
			}(),
			expectedHash: "YfJRufqUKBv9ez6xZx6ogpnfDnw9fDsyebhYDaoaH57D3vRu3",
		},
		{
			name: "2 children",
			n: func() *node {
				n := newNode(Key{})

				childNode1 := newNode(ToKey([]byte{255}))
				childNode1.setValue(maybe.Some([]byte("value1")))

				childNode2 := newNode(ToKey([]byte{237}))
				childNode2.setValue(maybe.Some([]byte("value2")))

				n.addChildWithID(childNode1, 4, childNode1.calculateID(&mockMetrics{}))
				n.addChildWithID(childNode2, 4, childNode2.calculateID(&mockMetrics{}))
				return n
			}(),
			expectedHash: "YVmbx5MZtSKuYhzvHnCqGrswQcxmozAkv7xE1vTA2EiGpWUkv",
		},
		{
			name: "16 children",
			n: func() *node {
				n := newNode(Key{})

				for i := byte(0); i < 16; i++ {
					childNode := newNode(ToKey([]byte{i << 4}))
					childNode.setValue(maybe.Some([]byte("some value")))

					n.addChildWithID(childNode, 4, childNode.calculateID(&mockMetrics{}))
				}
				return n
			}(),
			expectedHash: "5YiFLL7QV3f441See9uWePi3wVKsx9fgvX5VPhU8PRxtLqhwY",
		},
	}
	for _, test := range tests {
		t.Run(test.name, func(t *testing.T) {
			hash := test.n.calculateID(&mockMetrics{})
			require.Equal(t, test.expectedHash, hash.String())
		})
	}
}

Which seems to produce the same results as before.

[StephenButtolph]

I used strings here because the error text is much more readable in case of a failure.

[StephenButtolph]

🚀 this gets to remove these heuristics

[StephenButtolph]

I'm a bit torn on the usefulness of this function.

[patrick-ogrady]

dumb question: is there a reason we don't just make this an array where the key is the index?

[StephenButtolph]

is there a reason we don't just make this an array where the key is the index?

None that I can think of.

[patrick-ogrady]

If we did it to save memory, it seems like these extra ops we need to do during encoding are probably not worth it (not to mention that it may not actually save).

[patrick-ogrady]

just nits