Skip to content

mempool: batch txs per peer in broadcastTxRoutine#5321

Merged
melekes merged 17 commits intomasterfrom
anton/speedup-broadcastTxRoutine-625
Sep 23, 2020
Merged

mempool: batch txs per peer in broadcastTxRoutine#5321
melekes merged 17 commits intomasterfrom
anton/speedup-broadcastTxRoutine-625

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Sep 2, 2020 •
edited
Loading

Closes #625
Spec PR: tendermint/spec#155

@codecov
Copy link

codecov bot commented Sep 2, 2020 •
edited
Loading

Codecov Report

Merging #5321 into master will increase coverage by 1.43%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #5321      +/-   ##
==========================================
+ Coverage   59.92%   61.36%   +1.43%     
==========================================
  Files         202      259      +57     
  Lines       17476    23407    +5931     
==========================================
+ Hits        10473    14364    +3891     
- Misses       5945     7585    +1640     
- Partials     1058     1458     +400
Impacted Files Coverage Δ
config/toml.go 60.97% <ø> (ø)
mempool/clist_mempool.go 82.52% <ø> (ø)
config/config.go 78.30% <55.55%> (-0.94%) ⬇️
mempool/reactor.go 82.44% <88.57%> (ø)
consensus/reactor.go 73.83% <0.00%> (-3.78%) ⬇️
blockchain/v0/pool.go 76.01% <0.00%> (-2.59%) ⬇️
consensus/replay.go 70.53% <0.00%> (-0.83%) ⬇️
consensus/state.go 68.32% <0.00%> (-0.47%) ⬇️
proxy/multi_app_conn.go 48.64% <0.00%> (ø)
light/verifier.go 79.31% <0.00%> (ø)
... and 58 more

@melekes melekes marked this pull request as ready for review September 2, 2020 11:12
@melekes melekes self-assigned this Sep 2, 2020
@melekes melekes marked this pull request as draft September 2, 2020 11:19
@melekes melekes marked this pull request as ready for review September 2, 2020 11:53
tac0turtle
tac0turtle reviewed Sep 2, 2020
View reviewed changes
tac0turtle
tac0turtle reviewed Sep 2, 2020
View reviewed changes
tac0turtle
tac0turtle approved these changes Sep 3, 2020
View reviewed changes
Copy link
Contributor

@tac0turtle tac0turtle left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested locally, looks good. Would be nice to have some sort of benchmarks for how much it increased performance, but it's a nice to have.

tac0turtle
tac0turtle suggested changes Sep 3, 2020
View reviewed changes
Copy link
Contributor

@tac0turtle tac0turtle left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Whoops, meant to leave comment.

This PR requires a PR in the spec repo since it is changing the p2p msg.

@melekes
Copy link
Contributor Author

melekes commented Sep 4, 2020

Whoops, meant to leave comment.

This PR requires a PR in the spec repo since it is changing the p2p msg.

tendermint/spec#155

melekes
melekes commented Sep 4, 2020
View reviewed changes
@melekes melekes mentioned this pull request Sep 4, 2020
Closed
tac0turtle
tac0turtle approved these changes Sep 8, 2020
View reviewed changes
Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

Approving to dismiss the request for changes

erikgrinaker
erikgrinaker reviewed Sep 9, 2020
View reviewed changes
erikgrinaker
erikgrinaker reviewed Sep 9, 2020
View reviewed changes
@melekes
mempool: remove protoOverheadForTxsMessage
59b47a5 
also, do not check for peer height since it's redundant
@melekes melekes added the T:breaking Type: Breaking Change label Sep 14, 2020
melekes
melekes commented Sep 22, 2020
View reviewed changes
Copy link
Contributor Author

@melekes melekes left a comment

Choose a reason for hiding this comment

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

LGTM

melekes
melekes commented Sep 23, 2020
View reviewed changes
tac0turtle
tac0turtle reviewed Sep 23, 2020
View reviewed changes
@melekes
calculate size on the fly
a863538 
credits to Erik for idea
erikgrinaker
erikgrinaker approved these changes Sep 23, 2020
View reviewed changes
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM

@melekes
Copy link
Contributor Author

melekes commented Sep 23, 2020

NOTE: this PR breaks proto

 proto/tendermint/mempool/types.proto:12:5:Field "1" on message "Message" changed type from "tendermint.mempool.Tx" to "tendermint.mempool.Txs".
proto/tendermint/mempool/types.proto:12:9:Field "1" on message "Message" changed name from "tx" to "txs".

@melekes melekes merged commit ffe2742 into master Sep 23, 2020
@melekes melekes deleted the anton/speedup-broadcastTxRoutine-625 branch September 23, 2020 10:13
@kostko kostko mentioned this pull request Sep 25, 2020
Closed
p4u added a commit to vocdoni/tendermint that referenced this pull request Dec 9, 2020
@p4u
Revert "mempool: batch txs per peer in broadcastTxRoutine (tendermint…
75721cb 
…#5321)"

This reverts commit ffe2742.
@hvanz hvanz mentioned this pull request Mar 6, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ebuchman ebuchman Awaiting requested review from ebuchman

@tessr tessr Awaiting requested review from tessr

2 more reviewers

@tac0turtle tac0turtle tac0turtle approved these changes

@erikgrinaker erikgrinaker erikgrinaker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@melekes melekes

Labels

T:breaking Type: Breaking Change

Projects

None yet

Milestone

v0.34 Stargate

Development

Successfully merging this pull request may close these issues.

mempool: speed up broadcastTxRoutine

4 participants

@melekes @erikgrinaker @tac0turtle @tessr