Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

codec: new TxHandler byte decoder #4266

Merged
merged 9 commits into from
Aug 19, 2022

Conversation

algorandskiy
Copy link
Contributor

Summary

Use msgp marshaller instead of reflect in TxHandler

BenchmarkTxHandlerDecoder-8       	      68	  17116004 ns/op	 3335105 B/op	   51105 allocs/op
BenchmarkTxHandlerDecoderMsgp-8   	     122	  10213123 ns/op	 1189860 B/op	     614 allocs/op
BenchmarkTxHandlerProcessDecoded-8   	   62826	     19184 ns/op	    8956 B/op	      57 allocs/op
vs old
BenchmarkTxHandlerProcessDecoded-8   	   47414	     21560 ns/op	    8957 B/op	      57 allocs/op

Test Plan

TODO: write tests

@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #4266 (1a12135) into master (bf7aaec) will increase coverage by 0.03%.
The diff coverage is 42.30%.

@@            Coverage Diff             @@
##           master    #4266      +/-   ##
==========================================
+ Coverage   55.18%   55.22%   +0.03%     
==========================================
  Files         398      398              
  Lines       50137    50148      +11     
==========================================
+ Hits        27669    27692      +23     
+ Misses      20156    20141      -15     
- Partials     2312     2315       +3     
Impacted Files Coverage Δ
cmd/goal/clerk.go 8.75% <0.00%> (ø)
cmd/goal/multisig.go 10.00% <0.00%> (ø)
protocol/codec_tester.go 10.61% <0.00%> (ø)
data/txHandler.go 11.11% <20.00%> (+11.11%) ⬆️
protocol/codec.go 64.21% <80.00%> (+1.85%) ⬆️
cmd/tealdbg/local.go 73.95% <100.00%> (ø)
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
ledger/blockqueue.go 84.48% <0.00%> (-1.15%) ⬇️
data/transactions/verify/txn.go 43.85% <0.00%> (-0.88%) ⬇️
ledger/acctupdates.go 69.29% <0.00%> (-0.60%) ⬇️
... and 8 more

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

brianolson
brianolson previously approved these changes Jul 21, 2022
jannotti
jannotti previously approved these changes Jul 21, 2022
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this allows for every single call to protocol.NewDecoderBytes([]byte) to be replaced by your new NewMsgpDecoderBytes([]byte) unless the returned decoder is being used in a weird way. Seems like all we do with them is call Decode. And if we know we are decoding msgpack data (implied by the fact we set the codec to msgp in NewDecoderBytes), there's no point in using the Decoder type and going through reflection in Decoder.Decode() just let the Decode() on MsgpDecoderBytes get the job done statically.

@algorandskiy
Copy link
Contributor Author

Thank you for the feedback, I'll add a protocol/codec random data/length unit test and merge.

@algorandskiy algorandskiy requested a review from jannotti August 18, 2022 21:16
@algorandskiy algorandskiy changed the title poc: new TxHandler decoder codec: new TxHandler byte decoder Aug 19, 2022
@algorandskiy
Copy link
Contributor Author

Rebased/resolved a merge conflict

@algorandskiy algorandskiy merged commit 2848e37 into algorand:master Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants