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

feat: concurrent recheckTx #52

Merged
merged 11 commits into from
Jan 26, 2021
Merged

feat: concurrent recheckTx #52

merged 11 commits into from
Jan 26, 2021

Conversation

jinsan-line
Copy link
Contributor

@jinsan-line jinsan-line commented Jan 25, 2021

Related with: https://github.com/line/link/issues/1152, Finschia/ostracon#163, Finschia/ostracon#168, Finschia/ostracon#169

Description

To optimize performance, we need to increase concurrency. After implementing concurrent checkTx(#49), I'd like to implement concurrent recheckTx.

Motivation and context

How has this been tested?

Checklist:

  • I followed the contributing guidelines.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@jinsan-line jinsan-line self-assigned this Jan 25, 2021
@jinsan-line jinsan-line changed the title Concurrent rechecktx feat: concurrent recheckTx Jan 25, 2021
Copy link

@egonspace egonspace left a comment

Choose a reason for hiding this comment

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

I approve this with only understanding. I will do a precise verification of this in the later release-review.

return waits, signals
}

func (aw *AccountWGs) Waits(waits []*sync.WaitGroup) {

Choose a reason for hiding this comment

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

<This does not need to be modified now, but it is recorded for reference at a later release review.>
It is a little strange to use a third-person verb for a function name. WaitAll would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on your opinion. I think just Wait is better. HDYT?

req.tx, req.err = app.preCheckTx(req.txBytes)
}

func (app *BaseApp) checkTxAsync(req *RequestCheckTxAsync, waits []*sync.WaitGroup, signals []*AccountWG) {

Choose a reason for hiding this comment

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

<This does not need to be modified now, but it is recorded for reference at a later release review.>
Since this function itself does not run asynchronously, it would be better to remove async from the function name.

Copy link
Contributor Author

@jinsan-line jinsan-line Jan 25, 2021

Choose a reason for hiding this comment

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

I think it's proper name. It calls callback so it's a async function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised at cc6f354

sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

func (app *BaseApp) startReactors() {

Choose a reason for hiding this comment

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

<This does not need to be modified now, but it is recorded for reference at a later release review.>
Since there is only one reactor, I think startReactor is better

Copy link
Contributor Author

@jinsan-line jinsan-line Jan 25, 2021

Choose a reason for hiding this comment

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

I intend to add another reactor, deliver tx, so I named it as startReactors first.

baseapp/abci.go Show resolved Hide resolved
Copy link
Contributor

@wetcod wetcod left a comment

Choose a reason for hiding this comment

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

LGTM

@jinsan-line jinsan-line merged commit bc96769 into Finschia:feat/perf Jan 26, 2021
@jinsan-line jinsan-line deleted the concurrent-rechecktx branch January 26, 2021 08:47
@jinsan-line jinsan-line mentioned this pull request Jan 27, 2021
3 tasks
jinsan-line added a commit that referenced this pull request Apr 29, 2021
* chore: simply implement abci.`CheckTxSync()` and abci.`CheckTxAsync()`

* chore: move `accountLock` from `anteTx()` to `checkTxWithLock()`

* feat: impl `abci.CheckTxAsync()` with reactor

# Conflicts:
#	baseapp/baseapp.go

* feat: impl `accountwgs`

* feat: impl `accountwgs_test`

* chore: revise code after cherry-pick

* chore: bump-up tendermint & iavl

* chore: rename func from `startCheckTxAsyncReactor()` to `checkTxAsyncReactor()`

* fix: imports for lint

* fix: imports for lint

* chore: rename `Waits()` to `Wait()`
# Conflicts:
#	baseapp/abci.go
#	baseapp/accountlock.go
#	baseapp/accountwgs_test.go
#	baseapp/baseapp.go
#	baseapp/baseapp_test.go
#	go.mod
#	go.sum
@jinsan-line jinsan-line mentioned this pull request Apr 29, 2021
9 tasks
jinsan-line added a commit that referenced this pull request Apr 29, 2021
* chore: bump up ostracon

* feat: concurrent recheckTx (#52)

* chore: simply implement abci.`CheckTxSync()` and abci.`CheckTxAsync()`

* chore: move `accountLock` from `anteTx()` to `checkTxWithLock()`

* feat: impl `abci.CheckTxAsync()` with reactor

# Conflicts:
#	baseapp/baseapp.go

* feat: impl `accountwgs`

* feat: impl `accountwgs_test`

* chore: revise code after cherry-pick

* chore: bump-up tendermint & iavl

* chore: rename func from `startCheckTxAsyncReactor()` to `checkTxAsyncReactor()`

* fix: imports for lint

* fix: imports for lint

* chore: rename `Waits()` to `Wait()`
# Conflicts:
#	baseapp/abci.go
#	baseapp/accountlock.go
#	baseapp/accountwgs_test.go
#	baseapp/baseapp.go
#	baseapp/baseapp_test.go
#	go.mod
#	go.sum

* fix: add a tag, `goleveldb`, to `test_cover.sh`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants