perf(p2p/conn): Remove unneeded global pool buffers in secret connection by ValarDragon · Pull Request #3403 · cometbft/cometbft

ValarDragon · 2024-07-02T23:46:20Z

Remove unneeded global pool buffers in secret connection, instead just use a byte slice per secret connection, and just once for all packet sends. This is a 15% improvement to Secret connection's CPU time in Write, and 8% to read

We have much bigger system bottlenecks to fix (in the same RecvRoutine and SendRoutine calls), but this felt very low-lift, hence making the PR.

PR checklist

Tests written/updated - N/A
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments
Title follows the Conventional Commits spec

ValarDragon · 2024-07-02T23:55:34Z

(The govulncheck failure is unrelated, just a dependency update that needs to happen)

cason

Great work, simple change, probably high improvement.

But from the code it seems that we did not even need a pool of buffers, synchronized or not, but just a single buffer? Amazing that who implemented that did not realize it.

.changelog/unreleased/improvements/3403-remove-pool-buffer-usage-in-secretconn.md

cason · 2024-07-03T07:02:26Z

I renamed the changelog entry and PR title to p2p/conn because this is the name of the package where the secret connection is implemented.

cason · 2024-07-03T07:04:13Z

Ah, you forgot to run go mod tidy on the new code, since we have removed the libp2p dependency:

$ git diff go.mod
diff --git a/go.mod b/go.mod
index 13b2b9f26..a85559654 100644
--- a/go.mod
+++ b/go.mod
@@ -14,7 +14,6 @@ require (
        github.com/google/orderedcode v0.0.1
        github.com/gorilla/websocket v1.5.3
        github.com/lib/pq v1.10.9
-       github.com/libp2p/go-buffer-pool v0.1.0

…ge-in-secretconn.md Co-authored-by: Daniel <daniel.cason@informal.systems>

ValarDragon · 2024-07-03T09:19:25Z

But from the code it seems that we did not even need a pool of buffers, synchronized or not, but just a single buffer? Amazing that who implemented that did not realize it.

Yup, haha.

I renamed the changelog entry and PR title to p2p/conn because this is the name of the package where the secret connection is implemented.

Thank you! I forgot its based on package, hopefully will correct better for subsequent PRs!

Ah, you forgot to run go mod tidy on the new code, since we have removed the libp2p dependency

Wow, nice consequence of this

…ion (#3403) Closes #3197 Remove unneeded global pool buffers in secret connection, instead just use a byte slice per secret connection, and just once for all packet sends. This is a 15% improvement to Secret connection's CPU time in Write, and 8% to read We have much bigger system bottlenecks to fix (in the same RecvRoutine and SendRoutine calls), but this felt very low-lift, hence making the PR. --- #### PR checklist - [X] Tests written/updated - N/A - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] Updated relevant documentation (`docs/` or `spec/`) and code comments - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Daniel <daniel.cason@informal.systems> (cherry picked from commit d335255) # Conflicts: # .changelog/v0.38.8/improvements/3403-remove-pool-buffer-usage-in-secretconn.md # go.mod # go.sum # p2p/conn/secret_connection.go

adizere · 2024-07-04T10:15:25Z

Should we backport to v1 also?

ValarDragon · 2024-07-04T17:02:52Z

yes please!

melekes · 2024-07-05T06:33:31Z

@mergify backport v1.x

mergify · 2024-07-05T06:33:37Z

backport v1.x

✅ Backports have been created

Details

#3428 perf(p2p/conn): Remove unneeded global pool buffers in secret connection (backport #3403) has been created for branch v1.x

melekes · 2024-07-05T06:33:42Z

@mergify backport v0.37.x

mergify · 2024-07-05T06:33:48Z

backport v0.37.x

✅ Backports have been created

Details

#3427 perf(p2p/conn): Remove unneeded global pool buffers in secret connection (backport #3403) has been created for branch v0.37.x but encountered conflicts

…ion (#3403) Closes #3197 Remove unneeded global pool buffers in secret connection, instead just use a byte slice per secret connection, and just once for all packet sends. This is a 15% improvement to Secret connection's CPU time in Write, and 8% to read We have much bigger system bottlenecks to fix (in the same RecvRoutine and SendRoutine calls), but this felt very low-lift, hence making the PR. --- #### PR checklist - [X] Tests written/updated - N/A - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] Updated relevant documentation (`docs/` or `spec/`) and code comments - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Daniel <daniel.cason@informal.systems> (cherry picked from commit d335255) # Conflicts: # .changelog/v0.37.7/improvements/3403-remove-pool-buffer-usage-in-secretconn.md # go.mod # go.sum # p2p/conn/secret_connection.go

…ion (#3403) Closes #3197 Remove unneeded global pool buffers in secret connection, instead just use a byte slice per secret connection, and just once for all packet sends. This is a 15% improvement to Secret connection's CPU time in Write, and 8% to read We have much bigger system bottlenecks to fix (in the same RecvRoutine and SendRoutine calls), but this felt very low-lift, hence making the PR. --- #### PR checklist - [X] Tests written/updated - N/A - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] Updated relevant documentation (`docs/` or `spec/`) and code comments - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Daniel <daniel.cason@informal.systems> (cherry picked from commit d335255)

…ion (backport #3403) (#3428) Closes #3197 Remove unneeded global pool buffers in secret connection, instead just use a byte slice per secret connection, and just once for all packet sends. This is a 15% improvement to Secret connection's CPU time in Write, and 8% to read We have much bigger system bottlenecks to fix (in the same RecvRoutine and SendRoutine calls), but this felt very low-lift, hence making the PR. --- #### PR checklist - [X] Tests written/updated - N/A - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] Updated relevant documentation (`docs/` or `spec/`) and code comments - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3403 done by [Mergify](https://mergify.com). Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

…ion (backport #3403) (#3427) Closes #3197 Remove unneeded global pool buffers in secret connection, instead just use a byte slice per secret connection, and just once for all packet sends. This is a 15% improvement to Secret connection's CPU time in Write, and 8% to read We have much bigger system bottlenecks to fix (in the same RecvRoutine and SendRoutine calls), but this felt very low-lift, hence making the PR. --- #### PR checklist - [X] Tests written/updated - N/A - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] Updated relevant documentation (`docs/` or `spec/`) and code comments - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3403 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>

…ion (backport #3403) (#3418) Closes #3197 Remove unneeded global pool buffers in secret connection, instead just use a byte slice per secret connection, and just once for all packet sends. This is a 15% improvement to Secret connection's CPU time in Write, and 8% to read We have much bigger system bottlenecks to fix (in the same RecvRoutine and SendRoutine calls), but this felt very low-lift, hence making the PR. --- #### PR checklist - [X] Tests written/updated - N/A - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] Updated relevant documentation (`docs/` or `spec/`) and code comments - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3403 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>

ValarDragon added 4 commits July 3, 2024 00:38

Remove unneeded global pool buffers in secret connection

cfb7eeb

Apply the same to recvFrame

705ccc5

Update test as well

8ed6c99

Add changelog

47be66a

ValarDragon marked this pull request as ready for review July 3, 2024 01:03

ValarDragon requested a review from a team as a code owner July 3, 2024 01:03

ValarDragon requested a review from a team July 3, 2024 01:03

cason approved these changes Jul 3, 2024

View reviewed changes

.changelog/unreleased/improvements/3403-remove-pool-buffer-usage-in-secretconn.md Outdated Show resolved Hide resolved

cason changed the title ~~perf(p2p/secretconn): Remove unneeded global pool buffers in secret connection~~ perf(p2p/conn): Remove unneeded global pool buffers in secret connection Jul 3, 2024

Update .changelog/unreleased/improvements/3403-remove-pool-buffer-usa…

5e1d3b9

…ge-in-secretconn.md Co-authored-by: Daniel <daniel.cason@informal.systems>

ValarDragon and others added 2 commits July 3, 2024 10:20

go mod tidy

45da3b4

Merge branch 'main' into dev/remove_sync_buffer_in_secretconn

e0a0181

cason added this pull request to the merge queue Jul 3, 2024

Merged via the queue into main with commit d335255 Jul 3, 2024

cason deleted the dev/remove_sync_buffer_in_secretconn branch July 3, 2024 19:55

ValarDragon added the backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x label Jul 3, 2024

mergify bot mentioned this pull request Jul 3, 2024

perf(p2p/conn): Remove unneeded global pool buffers in secret connection (backport #3403) #3418

Merged

4 tasks

This was referenced Jul 5, 2024

perf(p2p/conn): Remove unneeded global pool buffers in secret connection (backport #3403) #3427

Merged

perf(p2p/conn): Remove unneeded global pool buffers in secret connection (backport #3403) #3428

Merged

faddat mentioned this pull request Oct 15, 2024

chore: use latest cometbft-db in v0.38.x #4296

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(p2p/conn): Remove unneeded global pool buffers in secret connection#3403

perf(p2p/conn): Remove unneeded global pool buffers in secret connection#3403
cason merged 7 commits intomainfrom
dev/remove_sync_buffer_in_secretconn

ValarDragon commented Jul 2, 2024 •

edited

Loading

Uh oh!

ValarDragon commented Jul 2, 2024

Uh oh!

cason left a comment

Uh oh!

Uh oh!

cason commented Jul 3, 2024

Uh oh!

cason commented Jul 3, 2024

Uh oh!

ValarDragon commented Jul 3, 2024

Uh oh!

adizere commented Jul 4, 2024

Uh oh!

ValarDragon commented Jul 4, 2024

Uh oh!

melekes commented Jul 5, 2024

Uh oh!

mergify bot commented Jul 5, 2024 •

edited

Loading

Uh oh!

melekes commented Jul 5, 2024

Uh oh!

mergify bot commented Jul 5, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ValarDragon commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

ValarDragon commented Jul 2, 2024

Uh oh!

cason left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cason commented Jul 3, 2024

Uh oh!

cason commented Jul 3, 2024

Uh oh!

ValarDragon commented Jul 3, 2024

Uh oh!

adizere commented Jul 4, 2024

Uh oh!

ValarDragon commented Jul 4, 2024

Uh oh!

melekes commented Jul 5, 2024

Uh oh!

mergify bot commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

melekes commented Jul 5, 2024

Uh oh!

mergify bot commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ValarDragon commented Jul 2, 2024 •

edited

Loading

mergify bot commented Jul 5, 2024 •

edited

Loading

mergify bot commented Jul 5, 2024 •

edited

Loading