Skip to content

feat: async commit#16175

Closed
yihuang wants to merge 10 commits intocosmos:mainfrom
yihuang:async-commit
Closed

feat: async commit#16175
yihuang wants to merge 10 commits intocosmos:mainfrom
yihuang:async-commit

Conversation

@yihuang
Copy link
Copy Markdown
Collaborator

@yihuang yihuang commented May 16, 2023

Description

Closes: #16173


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)

Comment thread x/upgrade/module.go Outdated
Key *store.KVStoreKey
Cdc codec.Codec
AddressCodec address.Codec
CommitMultiStore store.CommitMultiStore
Copy link
Copy Markdown
Collaborator Author

@yihuang yihuang May 16, 2023

Choose a reason for hiding this comment

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

Have no idea if it's correct, not familiar with the new depinject stuff.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is correct, but you need something that will provide the CommitMultiStore().

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Where should I put that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that it is instantiated after appBuilder.Build, AFAIK you actually need to use a setter and not have this in the keeper, and do something like PopulateVersionMap but for the CMS.
Then you can invoke that function in the init function. I'd say something like this in x/upgrade/module.go:

	appmodule.Register(&modulev1.Module{},
		appmodule.Invoke(PopulateVersionMap),
+		appmodule.Invoke(PopulateCommitMultiStore),
	)
func PopulateCommitMultiStore(upgradeKeeper *keeper.Keeper, bApp *baseapp.BaseApp) {
	if upgradeKeeper == nil {
		return
	}

	upgradeKeeper.SetCommitMultiStore(bApp.CommitMultiStore())
}

Note, that I have not tested the above.

Comment thread store/iavl/store.go Outdated

const (
DefaultIAVLCacheSize = 500000
CommitQueueBuffer = 10 // TODO configurable
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
CommitQueueBuffer = 10 // TODO configurable
CommitQueueBuffer = 10 // maximum pending commit versions.

I guess we don't actually need to make it configurable?

Comment thread x/upgrade/abci.go
panic(fmt.Errorf("unable to write upgrade info to filesystem: %s", err.Error()))
}

if err := k.WaitAsyncCommit(); err != nil {
Copy link
Copy Markdown
Collaborator Author

@yihuang yihuang May 16, 2023

Choose a reason for hiding this comment

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

Suggested change
if err := k.WaitAsyncCommit(); err != nil {
// avoid state rollback after restart
if err := k.WaitAsyncCommit(); err != nil {

Comment thread store/rootmulti/store.go
Comment on lines +503 to +505
for _, store := range rs.stores {
errs = append(errs, store.WaitAsyncCommit())
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment thread store/iavl/store.go
Comment on lines +159 to +174
go func() {
defer close(quitChan)

for expVersion := range commitQueue {
_, version, err := st.tree.SaveVersion()
if err != nil {
quitChan <- err
break
}

if version != expVersion {
quitChan <- fmt.Errorf("version sanity check failed: %d != %d", expVersion, version)
break
}
}
}()

Check notice

Code scanning / CodeQL

Spawning a Go routine

Spawning a Go routine may be a possible source of non-determinism
Comment thread baseapp/options.go Outdated
Comment thread store/rootmulti/store.go Outdated
Comment on lines +812 to +819
for key, store := range rs.stores {
if store.GetStoreType() == types.StoreTypeIAVL {
// If the store is wrapped with an inter-block cache, we must first unwrap
// it to get the underlying IAVL store.
store = rs.GetCommitKVStore(key)
store.(types.StoreWithCommitBufferSize).SetCommitBufferSize(size)
}
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
@yihuang
Copy link
Copy Markdown
Collaborator Author

yihuang commented May 17, 2023

don't work with current iavl implementation yet, see #16173 for discussions.

@yihuang yihuang closed this May 17, 2023
@yihuang yihuang deleted the async-commit branch May 17, 2023 03:25
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.

[Feature]: save iavl tree asynchronously to improve throughput

3 participants