Skip to content

perf(p2p/conn): Buffer secret connection writes (backport #3346)#3485

Merged
melekes merged 1 commit intov1.xfrom
mergify/bp/v1.x/pr-3346
Jul 10, 2024
Merged

perf(p2p/conn): Buffer secret connection writes (backport #3346)#3485
melekes merged 1 commit intov1.xfrom
mergify/bp/v1.x/pr-3346

Conversation

@mergify
Copy link
Contributor

@mergify mergify bot commented Jul 10, 2024

component of #3198 , this PR buffers writes. What happens is secret conn will often receive a write of (say) 65kb. But it will then split this into 64 1024 byte frames. It does a write on each frame, which is a syscall write. Then starts the next frame. Instead here, we buffer these writes, and do a single syscall write at the end.

I'll come back with benchmarks from running on mainnet. The baseline for this is netconn.Write taking 33% of the time in sendroutine.
image
image

We should do something similar for reads, but due to how the evil_secret_connection test makes non-black-box use of conn, that will require notable test refactors.


PR checklist

  • Tests written/updated
  • 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

This is an automatic backport of pull request #3346 done by [Mergify](https://mergify.com).

component of #3198 , this PR buffers writes. What happens is secret conn
will often receive a write of (say) 65kb. But it will then split this
into 64 1024 byte frames. It does a write on each frame, which is a
syscall write. Then starts the next frame. Instead here, we buffer these
writes, and do a single syscall write at the end.

I'll come back with benchmarks from running on mainnet. The baseline for
this is netconn.Write taking 33% of the time in sendroutine.

![image](https://github.com/cometbft/cometbft/assets/6440154/b7a43188-a69b-41b1-9506-2f66a2d63a74)

![image](https://github.com/cometbft/cometbft/assets/6440154/95f15ff0-94b8-419c-8759-1155d63d32f8)

We should do something similar for reads, but due to how the
evil_secret_connection test makes non-black-box use of conn, that will
require notable test refactors.

---

#### PR checklist

- [ ] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] 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: Anton Kaliaev <anton.kalyaev@gmail.com>
Co-authored-by: Daniel <daniel.cason@informal.systems>
(cherry picked from commit 8422f57)
@mergify mergify bot requested a review from a team as a code owner July 10, 2024 11:32
@mergify mergify bot requested a review from a team July 10, 2024 11:32
@melekes melekes merged commit 157f53d into v1.x Jul 10, 2024
@melekes melekes deleted the mergify/bp/v1.x/pr-3346 branch July 10, 2024 11:42
ValarDragon added a commit to osmosis-labs/cometbft that referenced this pull request Aug 19, 2024
…) (cometbft#3485)

component of cometbft#3198 , this PR buffers writes. What happens is secret conn
will often receive a write of (say) 65kb. But it will then split this
into 64 1024 byte frames. It does a write on each frame, which is a
syscall write. Then starts the next frame. Instead here, we buffer these
writes, and do a single syscall write at the end.

I'll come back with benchmarks from running on mainnet. The baseline for
this is netconn.Write taking 33% of the time in sendroutine.

![image](https://github.com/cometbft/cometbft/assets/6440154/b7a43188-a69b-41b1-9506-2f66a2d63a74)

![image](https://github.com/cometbft/cometbft/assets/6440154/95f15ff0-94b8-419c-8759-1155d63d32f8)

We should do something similar for reads, but due to how the
evil_secret_connection test makes non-black-box use of conn, that will
require notable test refactors.

---

- [ ] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] 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 cometbft#3346 done by
[Mergify](https://mergify.com).

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
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.

2 participants