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: improve performance and modify some abci #287

Merged
merged 40 commits into from
Aug 26, 2021

Conversation

egonspace
Copy link
Contributor

@egonspace egonspace commented Jul 8, 2021

Description

I bound each commit hash and corresponding PR of ebony branch.

50cffac
#273

162c63c
#264

ceec05b
#256

1a500f9
#246

720052e
#229

0ed36c7
#226 fix

1413661
#228

e47f13d
#226

4417941
#225

7da95d6
#224

94c8980
#223

1221fe7
#221

05becb8
#219

800cab3
#217

7ea6e35
"tendermint -> ostracon"

39067a6
#213, key name

5c8e180
#213, TM->OC

877e670
#213, modify tm-db interface

04c5f31
#212

dce3c03
#201

1cf27a5
"fix: use line/tm-db instead of tendermint/tm-db"

727f843
#199

208ceef
#199

Please add a description of the changes that this PR introduces and the files that
are the most critical to review.

Closes: #XXX

@CLAassistant
Copy link

CLAassistant commented Jul 8, 2021

CLA assistant check
All committers have signed the CLA.

@egonspace egonspace self-assigned this Jul 8, 2021
@egonspace egonspace added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Jul 8, 2021
@egonspace egonspace added the WIP label Jul 8, 2021
@egonspace
Copy link
Contributor Author

main branch is not ready for ci.
I'll rebase this branch after that main branch could run ci. @tnasu

egonspace added 22 commits July 8, 2021 20:30
* chore: use default db backend among the available ones

* chore: bump up iavl, tm-db
* fix: error handling after check tx

* fix: typo

* chore: (mempool) remove postCheck and impl reserve

* chore: fix tests

* chore: revise log (remove checkTx.Code)

* chore: add `CONTRACT` for `mem.proxyAppConn.CheckTxAsync()`

* chore: revise numTxs, txsBytes for `ErrMempoolIsFull` in reserve()

* chore: revise to remove redundant `isFull()`

* fix: remove tx from cache when `app errors` or `failed to reserve`

* Revert "chore: revise to remove redundant `isFull()`"

This reverts commit 55990ec.
…schia#219)

* fix: revise to call Begin/EndRecheck even though `mem.Size()` is 0

* chore: revise local_client.go

* fix: lint error

* chore: recheckTxs() just return if mem.Size() == 0
* feat: impl checkTxAsyncReactor() (Finschia#168)

* fix: tests

* fix: lint errors
…a#226)

* chore: revise abci.Client, Async() interfaces

* chore: regen mock w/ mockery 2.7.4

* fix: lint error

* fix: test_race

* mempool.Flush() flushes all txs from mempool so it should get `Lock()` instead of `RLock()`
* chore: remove iavl dependency

* chore: fix lint error
The build tag makes disable go implementation of secp256k1.
Cause there is no C implementation, a build error will occur when using tag `libsecp256k1`.
* perf: optimize checking the txs size

* ci: add GOPRIVATE to workflows

* test: add a unit test

* fix: fix lint errors
* perf: do not flush wal when receive consensus msgs
@egonspace egonspace force-pushed the merge_ebony_to_main branch from 776ea2e to 50cffac Compare July 8, 2021 11:32
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #287 (c8cbd4b) into main (f53ee28) will increase coverage by 1.47%.
The diff coverage is 68.25%.

@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
+ Coverage   61.18%   62.65%   +1.47%     
==========================================
  Files         272      272              
  Lines       29988    30166     +178     
==========================================
+ Hits        18347    18900     +553     
+ Misses       9963     9546     -417     
- Partials     1678     1720      +42     
Impacted Files Coverage Δ
abci/types/application.go 0.00% <0.00%> (ø)
abci/types/messages.go 16.05% <0.00%> (-1.55%) ⬇️
blockchain/v2/processor_context.go 63.15% <0.00%> (ø)
blockchain/v2/reactor.go 34.91% <ø> (ø)
cmd/ostracon/commands/light.go 15.82% <0.00%> (ø)
cmd/ostracon/commands/root.go 46.87% <0.00%> (ø)
config/toml.go 68.00% <ø> (ø)
consensus/replay_file.go 0.00% <0.00%> (ø)
consensus/replay_stubs.go 17.50% <0.00%> (-0.45%) ⬇️
crypto/bls/bls.go 48.41% <ø> (ø)
... and 90 more

@egonspace egonspace removed the WIP label Aug 24, 2021
@egonspace egonspace requested review from torao, tnasu and Kynea0b August 24, 2021 11:27
abci/client/local_client.go Show resolved Hide resolved
libs/log/tm_logger.go Outdated Show resolved Hide resolved
libs/log/tmfmt_logger.go Outdated Show resolved Hide resolved
light/example_test.go Show resolved Hide resolved
light/example_test.go Show resolved Hide resolved
tests.mk Outdated Show resolved Hide resolved
tests.mk Outdated Show resolved Hide resolved
tests.mk Outdated Show resolved Hide resolved
test/e2e/docker/Dockerfile Show resolved Hide resolved
test/e2e/docker/Dockerfile Show resolved Hide resolved
DurationCommitRechecking: discard.NewHistogram(),
DurationWaitingForNewRound: discard.NewHistogram(),

DurationGaugeProposal: discard.NewGauge(),
Copy link
Contributor

Choose a reason for hiding this comment

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

About variable name: Why is Gauge added only to variable names whose type is Gauge? For example, it doesn't seem to be attached to Histgram type variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are DurationProposal and DurationGaugeProposal. Grafana needs a histogram, and Gauge is comfortable when investigating in person, so I put both.

state/execution.go Outdated Show resolved Hide resolved
Namespace: namespace,
Subsystem: MetricsSubsystem,
Name: "duration_proposal_block_receiving",
Help: "Duration of receiving all proposal block parts",
Name: "duration_commit_committing",
Copy link
Contributor

@Kynea0b Kynea0b Aug 25, 2021

Choose a reason for hiding this comment

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

About variable names: Looking at the original code, it seems good to compose it with the words that explain Help. Isn't it duration_committing_updated_state instead of duration_commit_committing?

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'm sorry but this is hard to change. Grafana is using this name. This is what happened when I made the name more hierarchical.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the name used in Grafana?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! I dont know user name and password ^^;.

abci/client/grpc_client.go Show resolved Hide resolved
abci/example/counter/counter.go Show resolved Hide resolved
blockchain/v0/reactor_test.go Show resolved Hide resolved
types/block.go Outdated Show resolved Hide resolved
types/block.go Outdated Show resolved Hide resolved
Copy link
Contributor

@torao torao left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@tnasu tnasu left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: enhancement Classification: New feature or its request, or improvement in maintainability of code Stale For github bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants