Optimize HashInput functions to use scratch buffer provided by onflow/atree #1296

fxamacker · 2021-12-04T01:43:09Z

Description

Optimize HashInput functions to use a small scratch buffer provided by onflow/atree.

Targeted PR against master 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

codecov-commenter · 2021-12-04T01:54:02Z

Codecov Report

Merging #1296 (60c41f9) into master (651817e) will increase coverage by 0.01%.
The diff coverage is 92.66%.

@@            Coverage Diff             @@
##           master    #1296      +/-   ##
==========================================
+ Coverage   77.39%   77.41%   +0.01%     
==========================================
  Files         279      279              
  Lines       36048    36113      +65     
==========================================
+ Hits        27899    27956      +57     
- Misses       7062     7066       +4     
- Partials     1087     1091       +4

Flag	Coverage Δ
unittests	`77.41% <92.66%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
runtime/interpreter/value.go	`80.21% <92.66%> (+0.10%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 651817e...60c41f9. Read the comment docs.

github-actions · 2021-12-04T01:55:05Z

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 651817e
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Results

	old.txt	new.txt
	time/op		delta
RuntimeResourceDictionaryValues-2	14.0ms ± 5%	13.8ms ± 2%	~	(p=0.209 n=7+7)
RuntimeFungibleTokenTransfer-2	1.22ms ±29%	1.43ms ± 4%	~	(p=0.138 n=7+6)
ParseDeploy/byte_array-2	27.6ms ± 4%	27.2ms ± 4%	~	(p=0.805 n=7+7)
ParseDeploy/decode_hex-2	1.10ms ± 4%	1.07ms ± 1%	~	(p=0.051 n=7+6)
ParseArray-2	18.5ms ± 6%	17.8ms ± 2%	~	(p=0.295 n=7+6)
QualifiedIdentifierCreation/One_level-2	2.08ns ± 1%	2.07ns ± 0%	~	(p=0.461 n=7+7)
QualifiedIdentifierCreation/Three_levels-2	130ns ± 3%	130ns ± 7%	~	(p=0.778 n=7+7)
ContractInterfaceFungibleToken-2	37.3µs ± 6%	37.5µs ± 3%	~	(p=0.620 n=7+7)
CheckContractInterfaceFungibleTokenConformance-2	125µs ± 6%	125µs ± 4%	~	(p=1.000 n=7+7)
InterpretRecursionFib-2	2.31ms ±10%	2.24ms ± 9%	~	(p=0.535 n=7+7)
NewInterpreter/new_interpreter-2	929ns ± 4%	919ns ± 4%	~	(p=0.445 n=7+6)
ParseInfix-2	20.0µs ± 4%	19.2µs ± 1%	−3.98%	(p=0.008 n=7+6)
NewInterpreter/new_sub-interpreter-2	1.80µs ± 4%	1.72µs ± 8%	−4.36%	(p=0.026 n=7+7)
ParseFungibleToken-2	379µs ± 2%	359µs ± 1%	−5.39%	(p=0.002 n=6+6)

	alloc/op		delta
RuntimeResourceDictionaryValues-2	4.04MB ± 0%	4.04MB ± 0%	~	(p=0.181 n=6+7)
RuntimeFungibleTokenTransfer-2	238kB ± 0%	238kB ± 0%	~	(p=0.710 n=7+7)
QualifiedIdentifierCreation/One_level-2	0.00B	0.00B	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	64.0B ± 0%	64.0B ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	26.5kB ± 0%	26.5kB ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	65.7kB ± 0%	65.7kB ± 0%	~	(p=1.000 n=7+7)
InterpretRecursionFib-2	1.24MB ± 0%	1.24MB ± 0%	~	(p=0.580 n=6+7)
NewInterpreter/new_interpreter-2	720B ± 0%	720B ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	1.11kB ± 0%	1.11kB ± 0%	~	(all equal)

	allocs/op		delta
QualifiedIdentifierCreation/One_level-2	0.00	0.00	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	2.00 ± 0%	2.00 ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	457 ± 0%	457 ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	1.07k ± 0%	1.07k ± 0%	~	(all equal)
InterpretRecursionFib-2	25.0k ± 0%	25.0k ± 0%	~	(all equal)
NewInterpreter/new_interpreter-2	11.0 ± 0%	11.0 ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	32.0 ± 0%	32.0 ± 0%	~	(all equal)
RuntimeResourceDictionaryValues-2	102k ± 0%	102k ± 0%	−0.00%	(p=0.004 n=7+7)
RuntimeFungibleTokenTransfer-2	4.53k ± 0%	4.53k ± 0%	−0.02%	(p=0.042 n=7+6)

turbolent

Nice! Thank you for optimizing this 👍

turbolent · 2021-12-08T12:58:45Z

runtime/interpreter/value.go

+	if length <= len(scratch) {
+		buf = scratch[:length]
+	} else {
+		buf = make([]byte, length)
+	}


turbolent · 2021-12-08T12:59:42Z

runtime/interpreter/value.go

+// HashInput returns a byte slice containing:
+// - HashInputTypeInt8 (1 byte)
+// - int8 value (1 byte)


Thank you for also adding comments to the unchanged functions 👍

turbolent · 2021-12-08T13:06:49Z

@fxamacker It looks like coverage went down as the new cases where the input does not fit into the scratch are not covered by tests (e.g. https://app.codecov.io/gh/onflow/cadence/compare/1296/tree/runtime/interpreter/value.go#D1L352).

Could you please add some tests with longer/larger values? Thank you!

fxamacker · 2021-12-09T01:50:16Z

@fxamacker It looks like coverage went down as the new cases where the input does not fit into the scratch are not covered by tests (e.g. https://app.codecov.io/gh/onflow/cadence/compare/1296/tree/runtime/interpreter/value.go#D1L352).

Could you please add some tests with longer/larger values? Thank you!

Yikes! Thanks for catching this! 👍 It would've been bad to merge with this mistake. I added more tests. Many thanks!

turbolent

Nice!

turbolent · 2021-12-09T19:05:02Z

runtime/interpreter/value_test.go

@@ -1116,82 +1118,242 @@ func TestGetHashInput(t *testing.T) {
 			value:    NewUIntValueFromUint64(10),
 			expected: []byte{byte(HashInputTypeUInt), 10},
 		},
+		"UInt min": {


Great idea to use min & max values 👍

Optimize HashInput() to use scratch buffer

6c920ee

fxamacker added the Performance label Dec 4, 2021

fxamacker requested review from turbolent, ramtinms and SupunS December 4, 2021 01:43

fxamacker self-assigned this Dec 4, 2021

Fix lint error

f6f33e5

turbolent approved these changes Dec 8, 2021

View reviewed changes

fxamacker added 2 commits December 8, 2021 17:53

Add more tests for HashInput functions

cac07b9

Fix test error

ba6da54

Merge branch 'master' into fxamacker/optimize-hashinput

60c41f9

fxamacker requested a review from turbolent December 9, 2021 17:54

turbolent approved these changes Dec 9, 2021

View reviewed changes

fxamacker merged commit a9ef2ce into master Dec 9, 2021

turbolent deleted the fxamacker/optimize-hashinput branch March 1, 2022 00:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize HashInput functions to use scratch buffer provided by onflow/atree #1296

Optimize HashInput functions to use scratch buffer provided by onflow/atree #1296

fxamacker commented Dec 4, 2021

codecov-commenter commented Dec 4, 2021 •

edited

Loading

github-actions bot commented Dec 4, 2021 •

edited

Loading

turbolent left a comment

turbolent Dec 8, 2021

turbolent Dec 8, 2021

turbolent commented Dec 8, 2021

fxamacker commented Dec 9, 2021

turbolent left a comment

turbolent Dec 9, 2021

Optimize HashInput functions to use scratch buffer provided by onflow/atree #1296

Optimize HashInput functions to use scratch buffer provided by onflow/atree #1296

Conversation

fxamacker commented Dec 4, 2021

Description

codecov-commenter commented Dec 4, 2021 • edited Loading

Codecov Report

github-actions bot commented Dec 4, 2021 • edited Loading

Cadence Benchstat comparison

Results

turbolent left a comment

Choose a reason for hiding this comment

turbolent Dec 8, 2021

Choose a reason for hiding this comment

turbolent Dec 8, 2021

Choose a reason for hiding this comment

turbolent commented Dec 8, 2021

fxamacker commented Dec 9, 2021

turbolent left a comment

Choose a reason for hiding this comment

turbolent Dec 9, 2021

Choose a reason for hiding this comment

codecov-commenter commented Dec 4, 2021 •

edited

Loading

github-actions bot commented Dec 4, 2021 •

edited

Loading