8000 perf(p2p/conn): Remove unneeded global pool buffers in secret connection by ValarDragon · Pull Request #3403 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jul 3, 2024
Merged

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

merged 7 commits into from
Jul 3, 2024

Conversation

ValarDragon
Copy link
Contributor
@ValarDragon ValarDragon commented Jul 2, 2024 •
edited
Loading

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

  • 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
Copy link
Contributor Author

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

@ValarDragon ValarDragon marked this pull request as ready for review July 3, 2024 01:03
@ValarDragon ValarDragon requested review from a team as code owners July 3, 2024 01:03
cason
cason approved these changes Jul 3, 2024
View reviewed changes
Copy link
Contributor
@cason cason left a comment

Choose a reason for hiding this comment

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

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.

@cason 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
@cason
Copy link
Contributor
cason commented Jul 3, 2024

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
Copy link
Contributor
cason commented Jul 3, 2024

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

@ValarDragon @cason
Update .changelog/unreleased/improvements/3403-remove-pool-buffer-usa…
5e1d3b9 
…ge-in-secretconn.md

Co-authored-by: Daniel <daniel.cason@informal.systems>
@ValarDragon
Copy link
Contributor Author

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

@cason cason added this pull request to the merge queue Jul 3, 2024
Merged via the queue into main with commit d335255 Jul 3, 2024
39 checks passed
@cason cason deleted the dev/remove_sync_buffer_in_secretconn branch July 3, 2024 19:55
@ValarDragon ValarDragon added the backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x label Jul 3, 2024
mergify bot pushed a commit that referenced this pull request Jul 3, 2024
@ValarDragon @mergify
perf(p2p/conn): Remove unneeded global pool buffers in secret connect…
5bcf949 
…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
@mergify mergify bot mentioned this pull request Jul 3, 2024
Merged
4 tasks
@adizere
Copy link
Member
adizere commented Jul 4, 2024

Should we backport to v1 also?

@ValarDragon
Copy link
Contributor Author

yes please!

@melekes
Copy link
Contributor
melekes commented Jul 5, 2024

@mergify backport v1.x

@mergify Mergify
Copy link
Contributor
mergify bot commented Jul 5, 2024 •
edited
Loading

backport v1.x

✅ Backports have been created

@melekes
Copy link
Contributor
melekes commented Jul 5, 2024

@mergify backport v0.37.x

@mergify Mergify
Copy link
Contributor
mergify bot commented Jul 5, 2024 •
edited
Loading

backport v0.37.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 5, 2024
@ValarDragon @mergify
perf(p2p/conn): Remove unneeded global pool buffers in secret connect…
f20fbf7 
…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
mergify bot pushed a commit that referenced this pull request Jul 5, 2024
@ValarDragon @cason @mergify
perf(p2p/conn): Remove unneeded global pool buffers in secret connect…
11e2040 
…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)
This was referenced Jul 5, 2024
Merged
Merged
melekes pushed a commit that referenced this pull request Jul 5, 2024
@mergify @ValarDragon
perf(p2p/conn): Remove unneeded global pool buffers in secret connect…
5f63ddf 
…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>
melekes added a commit that referenced this pull request Jul 5, 2024
@mergify @ValarDragon
@melekes @hvanz
perf(p2p/conn): Remove unneeded global pool buffers in secret connect…
e2666e5 
…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>
melekes added a commit that referenced this pull request Jul 5, 2024
@mergify @ValarDragon
@melekes @hvanz
perf(p2p/conn): Remove unneeded global pool buffers in secret connect…
9ca0b49 
…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>
@faddat faddat mentioned this pull request Oct 15, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cason cason cason approved these changes

Assignees
No one assigned
Labels
backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x
Projects
No open projects
CometBFT
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Secret Connection: Remove sync.Pool, and put those buffers in the connection struct
4 participants
@ValarDragon @cason @adizere @melekes
0