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

perf: Faster state export #14332

Merged
merged 12 commits into from
Dec 19, 2022
Merged

Conversation

Reecepbcups
Copy link
Member

@Reecepbcups Reecepbcups commented Dec 15, 2022

Description

This PR uses better logic to export the genesis state export. This has a real world performance increase of ~43% for a given export (CPU: AMD Ryzen 5 3600 6-Core & Disk: SAMSUNG MZVL2512HCJQ-00B00)

# Command
time junod export --height 6086660 > state_export.json

# With this PR
real    4m45.816s
user    10m34.517s
sys     2m1.134s

# Standard
real    10m52.126s
user    7m7.416s
sys     1m14.892s

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@Reecepbcups Reecepbcups requested a review from a team as a code owner December 15, 2022 22:57
types/module/module.go Fixed Show fixed Hide fixed
types/module/module.go Outdated Show resolved Hide resolved
types/module/module.go Outdated Show resolved Hide resolved
@julienrbrt julienrbrt added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Dec 16, 2022
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm!

types/module/module.go Outdated Show resolved Hide resolved
Reecepbcups and others added 5 commits December 16, 2022 09:14
Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Julien Robert <julien@rbrt.fr>
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks :)

@julienrbrt julienrbrt enabled auto-merge (squash) December 16, 2022 15:45
}

// verify modules exists in app, so that we don't panic in the middle of an export
if err := m.checkModulesExists(modulesToExport); err != nil {
panic(err)
}

for _, moduleName := range modulesToExport {
channels := make([]chan json.RawMessage, len(modulesToExport))
Copy link
Member

Choose a reason for hiding this comment

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

One note here: this will create as many goroutines as modules. This may cause CPU contention. Limiting concurrent exports to GOMAXPROCS may improve perf slightly. This is a great PR tho and a huge improvement. Should be able to do something similar for migrations.

Copy link
Contributor

Choose a reason for hiding this comment

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

good point. but unless I'm mistaken, or things changed, Go is built to execute and handle hundreds of thousands of goroutines, whereas a typical app will have a dozen or so modules at most.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alex is correct here, go 1.18+ can handle ~500k routines per 1GB of memory. So we shouldn't have any issues here. I will mess with this more in the future to see if I can squeeze out more runtime performance.

I will also take a look at doing it for migrations too, thanks for the tip

Copy link
Contributor

Choose a reason for hiding this comment

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

Not all modules may implement HasGenesis. The setup can take this into account (and use append later)

Suggested change
channels := make([]chan json.RawMessage, len(modulesToExport))
channels := make([]chan json.RawMessage, 0, len(modulesToExport))

@julienrbrt julienrbrt added the A:automerge Automatically merge PR once all prerequisites pass. label Dec 16, 2022
@julienrbrt julienrbrt enabled auto-merge (squash) December 16, 2022 15:49
@julienrbrt julienrbrt removed the A:automerge Automatically merge PR once all prerequisites pass. label Dec 16, 2022
@julienrbrt
Copy link
Member

julienrbrt commented Dec 16, 2022

We need to check that data race in e2e tests before merging.
cc @Reecepbcups

types/module/module.go Fixed Show fixed Hide fixed
Comment on lines +404 to +406
go func(module HasGenesis, ch chan json.RawMessage) {
ch <- module.ExportGenesis(ctx, cdc)
}(module, channels[i])

Check notice

Code scanning / CodeQL

Spawning a Go routine

Spawning a Go routine may be a possible source of non-determinism
@alexanderbez alexanderbez enabled auto-merge (squash) December 19, 2022 15:37
tests/Makefile Outdated Show resolved Hide resolved
@julienrbrt
Copy link
Member

Note to self: add a changelog in the backported PR.

tests/Makefile Outdated Show resolved Hide resolved
@alexanderbez alexanderbez merged commit de6ef1e into cosmos:main Dec 19, 2022
mergify bot pushed a commit that referenced this pull request Dec 19, 2022
(cherry picked from commit de6ef1e)
julienrbrt added a commit that referenced this pull request Dec 19, 2022
Co-authored-by: Reece Williams <31943163+Reecepbcups@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
}
}

for i, moduleName := range modulesWithGenesis {
genesisData[moduleName] = <-channels[i]
Copy link
Contributor

Choose a reason for hiding this comment

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

You are iterating over modulesWithGenesis now which may be a subset of modulesToExport. The i value therefore is not the same.
For example if we have [foo, bar] as modules in modulesToExport with foo not implementing HasGenesis then modulesWithGenesis has only the bar element and you would still wait for channel[0] to return.
Some tests on this feature would be very useful

channels[i] = make(chan json.RawMessage)
modulesWithGenesis = append(modulesWithGenesis, moduleName)

go func(module HasGenesis, ch chan json.RawMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This introduces a race condition on github.com/cosmos/cosmos-sdk/store/types.(*infiniteGasMeter).ConsumeGas(). CI on wasmd found this.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for identifying this, agree we need better tests. @Reecepbcups can you help here?

//go:build e2e
// +build e2e
//go:build !race
// +build !race
Copy link
Contributor

Choose a reason for hiding this comment

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

Why deactivate?

modulesWithGenesis = append(modulesWithGenesis, moduleName)

go func(module HasGenesis, ch chan json.RawMessage) {
ch <- module.ExportGenesis(ctx, cdc)
Copy link
Contributor

Choose a reason for hiding this comment

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

To fix the gas meter race, you could set a new one per Go-routine, like ctx.WithGasMeter(sdk.NewInfiniteGasMeter()) .

tac0turtle added a commit that referenced this pull request Dec 22, 2022
tac0turtle added a commit that referenced this pull request Dec 22, 2022
julienrbrt added a commit that referenced this pull request Dec 23, 2022
julienrbrt added a commit that referenced this pull request Dec 26, 2022
mergify bot pushed a commit that referenced this pull request Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants