performance: Agreement state serialization using msgp #4644

iansuvak · 2022-10-14T15:53:20Z

Summary

This change will replace agreement state serialization used for crash recovery purposes from using reflection to using auto-generated code from the msgp library. It currently supports router and player structs, but not actions since they are polymorphic.

Currently I'm using unbound allocbounds since this isn't exposed to network and isn't an attack vector but happy to take suggestions on

TODO

~~Implement handling for messageHandle field which is of type interface{} and is present on messageEvents~~
~~Handle randomized generation for messageHandle which is of interface{} type and isn't currently supported.~~
Above weren't implemented. messageHandle field was explicitly unexported instead.

~~Either add allocbounds or handle codec_tester warnings~~

Test Plan

~~Pass autogenerated tests in codec_tester.go that compare encodings/decodings of randomly generated objects using msgp and go-codec~~
Benchmark test on randomized data shows improvement over the current reflection
Do manual test of starting a node, getting the state serialized to crash.db using the old encoder, shutting it down and successfully deserializing using the new code.

Testing done

New and existing tests pass
Benchmarks show improvement using both new randomized encoding benchmark as well as using a crash.db collected from a testnet node instrumented to insert new rows for each persist call instead of overwriting it.

Result stats over 20 benchmark tests of decode:

Same data created using master branch (reflection) was successfully decoded and reencoded using both libraries and shown to be equal. Test not included since it depends on having access to a binary DB file which shouldn't be committed to the repo:

Tests over Scenario1 with tps-multiplier of 6 show the following improvements on pprof CPU flamegraph:
Reflect CPU
Encode: 3.19% 12.52s
Decode: 5.06% 19.88s
msgp CPU
Encode: 1.36% 2.26s
Decode: 0.93% 1.53s

Reflect alloc objects
Encode: 3.91% 36323960
Decode: 4.07% 37835267

msgp alloc objects
Encode: 0.002% 43941
Decode: 0.67% 14930739

Performance Testing post removing omitempty

Result stats over 20 benchmark tests of decode:

Msgp time stayed about the same while reflect time went up a lot with removal of omitempty. Scenario1 testing is similar to above

But I also broke consistent sorting for ledger and others

All tests passing assuming that messageHandle doesn't need to be serialized

…essage

codecov · 2022-10-17T21:56:30Z

Codecov Report

Merging #4644 (fa48132) into master (7666348) will increase coverage by 0.05%.
The diff coverage is 83.49%.

@@            Coverage Diff             @@
##           master    #4644      +/-   ##
==========================================
+ Coverage   54.42%   54.47%   +0.05%     
==========================================
  Files         403      404       +1     
  Lines       51931    51994      +63     
==========================================
+ Hits        28264    28325      +61     
- Misses      21288    21292       +4     
+ Partials     2379     2377       -2

Impacted Files	Coverage Δ
agreement/actiontype_string.go	`33.33% <0.00%> (ø)`
agreement/asyncVoteVerifier.go	`89.83% <ø> (ø)`
agreement/cryptoVerifier.go	`67.60% <ø> (ø)`
agreement/events.go	`61.62% <ø> (ø)`
agreement/eventtype_string.go	`33.33% <0.00%> (ø)`
agreement/message.go	`66.66% <ø> (ø)`
agreement/proposalManager.go	`96.07% <ø> (ø)`
agreement/proposalStore.go	`100.00% <ø> (ø)`
agreement/proposalTracker.go	`95.16% <ø> (ø)`
agreement/proposalTrackerContract.go	`40.84% <ø> (ø)`
... and 22 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

brianolson

I think I skimmed the whole thing, there's a lot. One oddity popped out to me.

agreement/voteTracker.go

agreement/message.go

agreement/sort.go

cce · 2022-10-31T17:24:24Z

agreement/events.go

@@ -43,7 +43,9 @@ type event interface {
 // A ConsensusVersionView is a view of the consensus version as read from a
 // LedgerReader, associated with some round.
 type ConsensusVersionView struct {
-	Err     serializableError
+	_struct struct{} `codec:","`


what does the comma do?

Nothing other than telling msgp to generate marshaller methods for it. In our fork we treat omitempty as the logical default so you have to include the tag that explicitly leaves out omitempty it if you don't want the behavior.

Nothing before the comma means that field isn't renamed and nothing after it means no special directives like omitempty and others

so it can't be empty I take it, or no tag at all ... OK

jannotti

Seems good, nice testing of before and after.

I notice we rarely (never?) use omitempty. Is that in order to make the encodings the same before and after? I wonder how much space it would save and whether it's worth turning it on once we're confident enough that we don't care about the before/after tests.

jannotti · 2022-10-31T17:28:41Z

agreement/events.go

@@ -43,7 +43,9 @@ type event interface {
 // A ConsensusVersionView is a view of the consensus version as read from a
 // LedgerReader, associated with some round.
 type ConsensusVersionView struct {
-	Err     serializableError
+	_struct struct{} `codec:","`


Why not omitempty? Does this serialize the (usually nil, I assume) Err as a 0 length string?

We aren't using omitempty to make encodings as close as possible to what they were before. The one exception to where they wouldn't be identical is message struct since we are using msgp.Raw there.

The main reason @cce asked me to remove the omitempty hints is for the structs that contain maps since we could accidentally try to insert into a nil map at runtime which would cause a panic.

I'm happy to add omitemptys on all structs that don't export maps if we want to do that.

I wasn't planning on removing them since there is no noticeable CPU impact as I mentioned and the persistence.go implementation only ever stores a single row in the DB and with WAL enabled on the DB the small amount of IO increase here shouldn't be a concern either.

The idea was (and @iansuvak this is how it works right?) that the old reflection code will encode and decode a nil map differently from an empty map (right?) and if we use omitempty, the msgp-generated code will keep encoding and decoding this the same way.

jannotti · 2022-10-31T17:31:54Z

agreement/persistence.go

+	} else {
+		s.Router = protocol.Encode(&rr)
+		s.Player = protocol.Encode(&p)
+	}
 	s.Clock = t.Encode()
 	for _, act := range a {


Should we preallocate s.ActionTypes and s.Actions to be len(a)?

cce · 2022-10-31T18:00:27Z

agreement/persistence_test.go

+
+func TestRandomizedEncodingFullDiskState(t *testing.T) {
+	partitiontest.PartitionTest(t)
+	router, player := randomizeDiskState()


usually randomized tests run a bunch of iterations, maybe wrap this up a loop and make 10K iterations or something?

I did 5k since 10k was taking ~ 20s. I would be pro removing this or shrinking it down post-consensus release

agreement/persistence.go

…lices

cce · 2022-10-31T20:36:02Z

agreement/cryptoVerifier.go

@@ -82,37 +82,41 @@ type (
 		Quit()
 	}

+	//msgp:ignore cryptoVoteRequest


out of curiosity, how did you figure out which types to ignore?

Anything that embeds message really and is not messageEvent. The process of adding the fields was adding the _struct field to the top levels embedded things and then seeing which structs didn't get their interfaces implemented after running make msgp. anything that embeds message got accidentally added and ignoring them didn't introduce any unimplemented method errors.

cce · 2022-11-03T00:07:28Z

protocol/codec_tester.go

@@ -270,6 +277,10 @@ func randomizeValue(v reflect.Value, datapath string, tag string, remainingChang
 				// unexported
 				continue
 			}
+			if st.Name() == "messageEvent" && f.Name == "Tail" {
+				// Don't try and set the Tail field since it's recursive


well, you could have set just one Tail, but this is fine

cce

Did you try using the "autopsy" tool to analyze the new format of cadaver files?

iansuvak · 2022-11-03T14:28:53Z

No I didn't touch cadavers with this change. We should take another look at them at some point though since we are still using them on significant portion of stake

iansuvak added 11 commits October 5, 2022 10:40

WIP: Compiling but failing tests for just player

371b8af

WIP: barebones player serializes

83b7e80

WIP: Player and router are encoded with some missing fields

e2fa904

But I also broke consistent sorting for ledger and others

Changed names back to fix reflectdecoding

9d2c61e

Add sort interfaces for everything except for proposalValues

dc89b11

add sorts for proposalvalue

71464cb

Actually add the agreement/sort.go file

5d72a3d

WIP: still wrestling with it

ea28f9c

remove serializableerrorunderlying

c035014

Fix tests failing on diffs in nil object serde

bd34159

All tests passing assuming that messageHandle doesn't need to be serialized

Merge remote-tracking branch 'upstream/master' into msgp-agreement

869d1f4

iansuvak added Enhancement Team Carbon-11 labels Oct 14, 2022

iansuvak added 3 commits October 17, 2022 11:51

Merge remote-tracking branch 'upstream/master' into msgp-agreement

82ef464

explicitly unexport embedded MessageHandle interface from agreement.m…

b7fe033

…essage

Add reflect flag to encode/decode and benchmarks

e7cfbec

iansuvak force-pushed the msgp-agreement branch from 2222265 to e7cfbec Compare October 17, 2022 20:14

remove currently unused types from msgp generation

61345ff

iansuvak added 7 commits October 17, 2022 20:10

add proposalmanager and voteaggregator back

f9ca8eb

Fix for mismatch on rootRouter object

1fb3c04

clean up

d6cac67

Add test for full diskstate

75a8d79

Fix codegen CI msgp version

ed36eb2

Merge remote-tracking branch 'origin/master' into msgp-agreement

02ea522

Add test for SortProposalValue.Less()

00b4f82

iansuvak changed the title ~~WIP: perf: Agreement state serialization using msgp~~ perf: Agreement state serialization using msgp Oct 19, 2022

iansuvak marked this pull request as ready for review October 20, 2022 20:16

iansuvak assigned cce and algorandskiy Oct 20, 2022

brianolson reviewed Oct 28, 2022

View reviewed changes

agreement/voteTracker.go Outdated Show resolved Hide resolved

remove misplaced comment

565833f

algorandskiy reviewed Oct 31, 2022

View reviewed changes

agreement/message.go Outdated Show resolved Hide resolved

agreement/sort.go Show resolved Hide resolved

address review comments

609bdf8

algorandskiy previously approved these changes Oct 31, 2022

View reviewed changes

cce reviewed Oct 31, 2022

View reviewed changes

cce requested a review from yossigi October 31, 2022 17:25

jannotti reviewed Oct 31, 2022

View reviewed changes

cce reviewed Oct 31, 2022

View reviewed changes

preallocate s.Actions and s.ActionTypes

6fad072

iansuvak dismissed algorandskiy’s stale review via 6fad072 October 31, 2022 18:01

jannotti reviewed Oct 31, 2022

View reviewed changes

agreement/persistence.go Show resolved Hide resolved

iansuvak added 2 commits October 31, 2022 14:11

loop over randomized testing and properly assign into pre-allocated s…

aa5ad07

…lices

Add test to cover action serde

e798292

algorandskiy previously approved these changes Oct 31, 2022

View reviewed changes

cce reviewed Oct 31, 2022

View reviewed changes

re-run "go generate" inside agreement

fa48132

cce dismissed algorandskiy’s stale review via fa48132 October 31, 2022 20:41

algorandskiy approved these changes Oct 31, 2022

View reviewed changes

algorandskiy merged commit ba9c593 into algorand:master Oct 31, 2022

iansuvak deleted the msgp-agreement branch November 1, 2022 17:01

Algo-devops-service mentioned this pull request Nov 1, 2022

go-algorand 3.12.0-beta Release PR #4730

Merged

algorandskiy mentioned this pull request Nov 1, 2022

perf: use mgp in agreement state serialization instead of reflect #4550

Closed

cce reviewed Nov 3, 2022

View reviewed changes

Algo-devops-service mentioned this pull request Nov 10, 2022

go-algorand 3.12.0-stable Release PR #4783

Closed

Algo-devops-service mentioned this pull request Nov 28, 2022

go-algorand 3.12.2-stable Release PR #4837

Merged

iansuvak mentioned this pull request Jul 21, 2023

agreement: add omitempty to MessageHandle in agreement message #5593

Merged

cce mentioned this pull request Aug 15, 2023

clocks: tagged deadlines #5649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

performance: Agreement state serialization using msgp #4644

performance: Agreement state serialization using msgp #4644

iansuvak commented Oct 14, 2022 •

edited

Loading

codecov bot commented Oct 17, 2022 •

edited

Loading

brianolson left a comment

cce Oct 31, 2022

iansuvak Oct 31, 2022

cce Oct 31, 2022

jannotti left a comment

jannotti Oct 31, 2022

iansuvak Oct 31, 2022

cce Oct 31, 2022

jannotti Oct 31, 2022

cce Oct 31, 2022

iansuvak Oct 31, 2022

cce Oct 31, 2022

iansuvak Oct 31, 2022

cce Nov 3, 2022

cce left a comment

iansuvak commented Nov 3, 2022

performance: Agreement state serialization using msgp #4644

performance: Agreement state serialization using msgp #4644

Conversation

iansuvak commented Oct 14, 2022 • edited Loading

Summary

TODO

Test Plan

Testing done

Performance Testing post removing omitempty

codecov bot commented Oct 17, 2022 • edited Loading

Codecov Report

brianolson 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

jannotti 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

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

cce left a comment

Choose a reason for hiding this comment

iansuvak commented Nov 3, 2022

iansuvak commented Oct 14, 2022 •

edited

Loading

codecov bot commented Oct 17, 2022 •

edited

Loading