Optimize encoding of dictionaries #752

fxamacker · 2021-04-01T00:51:51Z

Closes #743

Description

Optimize encoding of dictionaries to:

speed up encoding dictionaries
speed up decoding dictionaries
reduce memory use
reduce size of stored data

Changes

Switch from CBOR map to CBOR array to improve speed and preserve ordering.
Remove key strings from encoding.
Remove old backwards-compatibility decoding code.
Add tests, including round-trip for decoding old format and encoding new format.
Continue to support deferred keys (no change).

Encoding comparisons:

name                  old time/op    new time/op    delta
EncodingSmallValue-4     200µs ± 0%     138µs ± 0%  -31.32%  (p=0.000 n=8+10)
EncodingLargeValue-4    19.4ms ± 1%    13.2ms ± 1%  -32.39%  (p=0.000 n=10+10)

name                  old alloc/op   new alloc/op   delta
EncodingSmallValue-4    57.6kB ± 0%    36.2kB ± 0%  -37.12%  (p=0.000 n=10+9)
EncodingLargeValue-4    4.67MB ± 1%    3.55MB ± 0%  -23.95%  (p=0.000 n=10+10)

name                  old allocs/op  new allocs/op  delta
EncodingSmallValue-4     1.10k ± 0%     0.80k ± 0%  -27.46%  (p=0.000 n=10+10)
EncodingLargeValue-4     92.1k ± 0%     70.8k ± 0%  -23.12%  (p=0.000 n=10+10)

Decoding comparisons:

name                  old time/op    new time/op    delta
DecodingSmallValue-4     192µs ± 0%     145µs ± 0%  -24.21%  (p=0.000 n=10+8)
DecodingLargeValue-4    19.9ms ± 0%    14.7ms ± 0%  -26.29%  (p=0.000 n=10+10)

name                  old alloc/op   new alloc/op   delta
DecodingSmallValue-4    73.9kB ± 0%    60.7kB ± 0%  -17.84%  (p=0.000 n=10+10)
DecodingLargeValue-4    7.49MB ± 0%    5.75MB ± 0%  -23.23%  (p=0.000 n=10+10)

name                  old allocs/op  new allocs/op  delta
DecodingSmallValue-4     1.57k ± 0%     1.36k ± 0%  -13.36%  (p=0.000 n=10+10)
DecodingLargeValue-4      143k ± 0%      122k ± 0%  -14.63%  (p=0.000 n=10+10)

Benchmark comparisons were done on linux_amd64 with Go 1.15.10.

Special thanks to @turbolent and @SupunS for their helpful feedback, explanations, and suggestions during our call!

For contributor use:

Targeted PR against feature/storage-optimizations branch
Linked to Github issue with discussion and accepted design OR link to spec that describes this work
Code follows the standards mentioned here
Updated relevant documentation
Re-reviewed Files changed in the Github PR explorer
Added appropriate labels

Switch from CBOR map to CBOR array to improve speed and preserve ordering. Remove key strings from encoding. Remove old backwards-compatibility decoding code. Add tests, including round-trip for decoding old format and encoding new format. Closes onflow#743

SupunS

Nice! 👏

SupunS · 2021-04-01T12:34:26Z

runtime/interpreter/encode.go

@@ -755,13 +757,13 @@ func (e *Encoder) prepareDictionaryValue(
 			if err != nil {
 				return nil, err
 			}
-			entries[key] = prepared
+			entries = append(entries, prepared)


Directly setting the value to the index might be slightly better in terms of performance (I haven't tested though, just a hunch)

Suggested change

entries = append(entries, prepared)

entries[index] = prepared

index++

VERY nice catch! Your hunch is correct! 👍

In isolation (small example program), this change provides a bigger improvement.

For encoding, the new way required more coding changes. Encoding speed improvement is around 0.5-0.7% vs append for large data. Thanks again for the feedback, I'll incorporate this change!

New benchmark comparisons won't show this 0.5-0.7% speedup (compared to yesterday's benchmark) because compiling after adding error handling took away this gain.

SupunS · 2021-04-01T12:37:59Z

runtime/interpreter/encode.go

@@ -683,7 +684,8 @@ func (e *Encoder) prepareDictionaryValue(
 		return nil, err
 	}

-	entries := make(map[string]interface{}, v.Entries.Len())
+	// Use CBOR array for entry value to preserve ordering and improve speed.
+	entries := make([]interface{}, 0, v.Entries.Len())


Could we use cborArray here and in other places?

Suggested change

entries := make([]interface{}, 0, v.Entries.Len())

entries := make(cborArray, 0, v.Entries.Len())

Yes! Nice catch! For this PR, I should do this in other places if it's in functions for dictionaries. For non-dictionaries like composite types, I'd prefer to do it in the PR for composite types but I'm OK with either approach.

sure, that sounds good!

fxamacker · 2021-04-01T15:00:45Z

I found some code that required more error handling (needed to check array length). I fixed it and will be more careful before opening my next PR (I had gotten too comfortable using coverage+fuzzing tools to alert me because it's feasible for serialization libraries, probably not for interpreters).

Add error handling to check decoded dictionary array length. Incorporate feedback to replace append with direct assignment for an additional speed gain of 0.5-0.7% for encoding large data.

fxamacker · 2021-04-01T17:01:54Z

New benchmarks after incorporating feedback and adding error-handling are similar to the original benchmark comparisons posted with the PR.

Encoding vs master:

name                  old time/op    new time/op    delta
EncodingSmallValue-4     199µs ± 0%     137µs ± 0%  -31.26%  (p=0.000 n=10+9)
EncodingLargeValue-4    19.4ms ± 0%    13.1ms ± 1%  -32.25%  (p=0.000 n=10+10)

name                  old alloc/op   new alloc/op   delta
EncodingSmallValue-4    57.6kB ± 0%    36.2kB ± 0%  -37.12%  (p=0.000 n=10+10)
EncodingLargeValue-4    4.67MB ± 0%    3.55MB ± 0%  -23.93%  (p=0.000 n=10+10)

name                  old allocs/op  new allocs/op  delta
EncodingSmallValue-4     1.10k ± 0%     0.80k ± 0%  -27.46%  (p=0.000 n=10+10)
EncodingLargeValue-4     92.1k ± 0%     70.8k ± 0%  -23.12%  (p=0.000 n=10+10)

Decoding vs master:

name                  old time/op    new time/op    delta
DecodingSmallValue-4     192µs ± 0%     145µs ± 0%  -24.32%  (p=0.000 n=9+10)
DecodingLargeValue-4    19.9ms ± 1%    14.7ms ± 0%  -26.26%  (p=0.000 n=10+10)

name                  old alloc/op   new alloc/op   delta
DecodingSmallValue-4    73.9kB ± 0%    60.7kB ± 0%  -17.84%  (p=0.000 n=10+10)
DecodingLargeValue-4    7.49MB ± 0%    5.75MB ± 0%  -23.23%  (p=0.000 n=10+9)

name                  old allocs/op  new allocs/op  delta
DecodingSmallValue-4     1.57k ± 0%     1.36k ± 0%  -13.36%  (p=0.000 n=10+10)
DecodingLargeValue-4      143k ± 0%      122k ± 0%  -14.62%  (p=0.000 n=10+10)

Benchmark comparisons were done on linux_amd64 with Go 1.15.10.

turbolent

This looks great, nice work @fxamacker! 👏

No worries about opening earlier, feel free to keep doing that, the review and adding missing things is what the PRs are here for 👍

turbolent · 2021-04-02T00:36:34Z

runtime/interpreter/encode.go

@@ -31,6 +31,8 @@ import (
 	"github.com/onflow/cadence/runtime/common"
 )

+type cborArray = []interface{}


turbolent · 2021-04-02T00:42:55Z

runtime/interpreter/encode.go

 		}
 	}

 	return cbor.Tag{
 		Number: cborTagDictionaryValue,
-		Content: cborMap{
+		Content: cborArray{


Hah, nice that the slice initialization can stay the same, I didn't know this was possible 👍

turbolent · 2021-04-02T00:43:27Z

runtime/interpreter/encoding_test.go

+	if version <= 3 {
+		decoded, err = DecodeValueV3(encoded, &testOwner, nil, version, nil)
+	} else {


Great idea to still support the old one 👍

fxamacker requested a review from turbolent as a code owner April 1, 2021 00:51

fxamacker added the Improvement label Apr 1, 2021

SupunS reviewed Apr 1, 2021

View reviewed changes

fxamacker marked this pull request as draft April 1, 2021 15:06

Improve error handling and speed for dictionaries

c52a6ad

Add error handling to check decoded dictionary array length. Incorporate feedback to replace append with direct assignment for an additional speed gain of 0.5-0.7% for encoding large data.

fxamacker marked this pull request as ready for review April 1, 2021 17:01

fxamacker mentioned this pull request Apr 1, 2021

Optimize CBOR representation of data in Cadence storage layer #738

Closed

turbolent approved these changes Apr 2, 2021

View reviewed changes

turbolent merged commit cede38e into onflow:feature/storage-optimizations Apr 2, 2021

fxamacker mentioned this pull request Apr 5, 2021

Create CBOR encoding and decoding benchmarks using production data #0

Closed

turbolent mentioned this pull request Apr 5, 2021

Benchmark CBOR encoding/decoding using production data #765

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize encoding of dictionaries #752

Optimize encoding of dictionaries #752

fxamacker commented Apr 1, 2021 •

edited

Loading

SupunS left a comment

SupunS Apr 1, 2021 •

edited

Loading

fxamacker Apr 1, 2021

fxamacker Apr 1, 2021 •

edited

Loading

SupunS Apr 1, 2021

fxamacker Apr 1, 2021

SupunS Apr 1, 2021

fxamacker commented Apr 1, 2021 •

edited

Loading

fxamacker commented Apr 1, 2021

turbolent left a comment

turbolent Apr 2, 2021

turbolent Apr 2, 2021

turbolent Apr 2, 2021

	entries = append(entries, prepared)
	entries[index] = prepared
	index++

	entries := make([]interface{}, 0, v.Entries.Len())
	entries := make(cborArray, 0, v.Entries.Len())

Optimize encoding of dictionaries #752

Optimize encoding of dictionaries #752

Conversation

fxamacker commented Apr 1, 2021 • edited Loading

Description

Changes

Encoding comparisons:

Decoding comparisons:

SupunS left a comment

Choose a reason for hiding this comment

SupunS Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

fxamacker Apr 1, 2021

Choose a reason for hiding this comment

fxamacker Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

SupunS Apr 1, 2021

Choose a reason for hiding this comment

fxamacker Apr 1, 2021

Choose a reason for hiding this comment

SupunS Apr 1, 2021

Choose a reason for hiding this comment

fxamacker commented Apr 1, 2021 • edited Loading

fxamacker commented Apr 1, 2021

turbolent left a comment

Choose a reason for hiding this comment

turbolent Apr 2, 2021

Choose a reason for hiding this comment

turbolent Apr 2, 2021

Choose a reason for hiding this comment

turbolent Apr 2, 2021

Choose a reason for hiding this comment

fxamacker commented Apr 1, 2021 •

edited

Loading

SupunS Apr 1, 2021 •

edited

Loading

fxamacker Apr 1, 2021 •

edited

Loading

fxamacker commented Apr 1, 2021 •

edited

Loading