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.

Sign up for GitHub

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

Jump to bottom

perf: Faster state export #14332

Merged
merged 12 commits into from
Dec 19, 2022
Merged

perf: Faster state export #14332

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a29e3c5
go routine for exporting genesis modules
Reecepbcups Dec 15, 2022
b69c231
Update types/module/module.go
Reecepbcups Dec 16, 2022
0ede0ad
Update types/module/module.go
Reecepbcups Dec 16, 2022
fb382a6
init slice with set length
Reecepbcups Dec 16, 2022
dd9ba1b
add index to for loop
Reecepbcups Dec 16, 2022
a76614d
Merge branch 'main' into reece/faster-export
julienrbrt Dec 16, 2022
b908cd2
only receive if module has genesis
Reecepbcups Dec 17, 2022
e352bf3
ignore race for e2e export test only
Reecepbcups Dec 17, 2022
5f16585
Merge branch 'main' into reece/faster-export
Reecepbcups Dec 18, 2022
ffcf5c4
Merge branch 'main' into reece/faster-export
alexanderbez Dec 19, 2022
37ba2e5
add test server incov tests
julienrbrt Dec 19, 2022
1abdfde
add coverage for sonarcloud
julienrbrt Dec 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions tests/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ test-integration:
test-integration-cov:
go test ./integration/... -timeout 30m -coverpkg=../... -coverprofile=integration-profile.out -covermode=atomic

test-e2e:
go test ./e2e/... -mod=readonly -timeout 30m -race -tags='e2e'
test-e2e: test-e2e-server
go test ./e2e/... -mod=readonly -timeout 30m -tags='e2e'

test-e2e-cov:
go test ./e2e/... -mod=readonly -timeout 30m -race -tags='e2e' -coverpkg=../... -coverprofile=e2e-profile.out -covermode=atomic
test-e2e-cov: test-e2e-server
go test ./e2e/... -mod=readonly -timeout 30m -race -tags='e2e' -coverpkg=../... -coverprofile=e2e-profile.out -covermode=atomic

test-e2e-server:
go test ./e2e/server -mod=readonly -timeout 30m -tags='e2e'
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions tests/e2e/server/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//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?


package server_test

Expand Down
25 changes: 16 additions & 9 deletions types/module/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,27 +383,34 @@ func (m *Manager) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) map[string
// ExportGenesisForModules performs export genesis functionality for modules
func (m *Manager) ExportGenesisForModules(ctx sdk.Context, cdc codec.JSONCodec, modulesToExport []string) map[string]json.RawMessage {
genesisData := make(map[string]json.RawMessage)
if len(modulesToExport) == 0 {
for _, moduleName := range m.OrderExportGenesis {
if module, ok := m.Modules[moduleName].(HasGenesis); ok {
genesisData[moduleName] = module.ExportGenesis(ctx, cdc)
}
}

return genesisData
if len(modulesToExport) == 0 {
modulesToExport = m.OrderExportGenesis
}

// 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))

modulesWithGenesis := make([]string, 0, len(modulesToExport))

for i, moduleName := range modulesToExport {
if module, ok := m.Modules[moduleName].(HasGenesis); ok {
genesisData[moduleName] = module.ExportGenesis(ctx, cdc)
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?

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()) .

}(module, channels[i])
Comment on lines +404 to +406

Check notice

Code scanning / CodeQL

Spawning a Go routine

Spawning a Go routine may be a possible source of non-determinism
}
}

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

}

return genesisData
}

Expand Down