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

[Feature]: Replace ABCIMessageLogs .String() method with faster JSON library #18906

Closed
ValarDragon opened this issue Dec 27, 2023 · 7 comments · Fixed by #18963
Closed

[Feature]: Replace ABCIMessageLogs .String() method with faster JSON library #18906

ValarDragon opened this issue Dec 27, 2023 · 7 comments · Fixed by #18963
Assignees
Labels
T:feature-request T: Performance Performance improvements

Comments

@ValarDragon
Copy link
Contributor

Summary

ABCIMessageLogs.String() uses legacy amino for JSON marshalling. As far as I can tell, this usage is fully compatible with the stdlib.

We see this method is taking a surprising amount of time on Osmosis. Its taking about 2% of the state execution time during block sync.

// String implements the fmt.Stringer interface for the ABCIMessageLogs type.
func (logs ABCIMessageLogs) String() (str string) {
if logs != nil {
cdc := codec.NewLegacyAmino()
raw, err := cdc.MarshalJSON(logs)
if err == nil {
str = string(raw)
}
}
return str
}

Can we replace this method with a faster, standard JSON marshalling library? I am making an issue instead of a PR just to sanity check this is integrator compatible. I don't see a way it wouldn't be compatible, but this does seem bad to break

Problem Definition

This is taking too much CPU time

Proposed Feature

Replace amino JSON marshalling here with a faster JSON marshalling library.

@ValarDragon ValarDragon added T:feature-request T: Performance Performance improvements labels Dec 27, 2023
@alexanderbez
Copy link
Contributor

As long as it's compatible with stdlib JSON, I think it should be OK and state compatible. In such a case, we can consider https://github.com/sugawarayuuta/sonnet

@tac0turtle
Copy link
Member

there are somethings that will change, mainly because amino is protobuf style json which is not normal json. the changes are minimal and probably better in terms of ux since people dont use this with protobuf but normal json decoders/encoders

@ValarDragon
Copy link
Contributor Author

ABCI Message Logs is a slice of ABCIMessage , which itself is a struct of just various nested strings (and one uint): https://github.com/cosmos/cosmos-sdk/blob/main/proto/cosmos/base/abci/v1beta1/abci.proto#L53-L79

A bit surprised theres even an incompatability!

@ValarDragon
Copy link
Contributor Author

For large txs during block sync with big events, were actually seeing this take 15% of the state machine execution time! (Defined as deliverTx/Beginblock/ENdblock time measured in a blocksync pprof of state.execBlockOnProxyApp )

@alexanderbez
Copy link
Contributor

I'm glad you're really benchmarking various aspects of the module codebase that have largely remained untouched for a long time.

Do you have a recommendation for an alternative JSON library?

@ValarDragon
Copy link
Contributor Author

The standard library for now would make this much faster! In the future, sometime compiled would be great, as this structure is super simple -- but no need to complicate it for now!

Glad the speed investigations are appreciated! Was feeling really committed to improving state machine sync speeds! And then we keep facing issues on Osmosis that are related, so its becoming more and more of a priority haha.

The hot loop of looking at CPU profiles and making stuff go away is really nice though :)

Tons of wins are actually already in SDK main / the new store work as well, which is super exciting!

@alexanderbez
Copy link
Contributor

Cool! Ok, so I think let's just convert this to using stdlib JSON then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:feature-request T: Performance Performance improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants