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

Speedup CPU time of channel.WritePacketMsgTo #2951

Open
3 tasks
Tracked by #3053
ValarDragon opened this issue Apr 30, 2024 · 1 comment
Open
3 tasks
Tracked by #3053

Speedup CPU time of channel.WritePacketMsgTo #2951

ValarDragon opened this issue Apr 30, 2024 · 1 comment
Labels
enhancement New feature or request needs-triage This issue/PR has not yet been triaged by the team.

Comments

@ValarDragon
Copy link
Collaborator

ValarDragon commented Apr 30, 2024

Feature Request

Summary

Whenever we want to send a packet, we spend a considerable amount of time in CPU overheads (that are actually under the flowrate limitation, and thereby hurting throughput). When its time to send, we have to spend awhile:

  • re-encoding data
  • doing proto marshalling
  • allocating buffers

image

This time is all under flow-rate limitation and a single threaded bottleneck for communicating to this peer (as far as I understand)

Time delays

If we look at the time here, there is a few parts:

Re: The triple wrapping problem. It seems this is something we can backwards compatibly speedup with some "hand rolled" mimicking the proto-encoding tricks, or having less heap allocations. Neither feels elegant right now though. I hope other people have better solutions. Longer term, ideally we just don't triple wrap.

For the new internal buffer per packet, ideally we can just make the channel use the already allocated protowriter that exists in sendRoutine?

There are other flow rate problems that aren't part of this, but I leave that to another github issue.

@ValarDragon ValarDragon added enhancement New feature or request needs-triage This issue/PR has not yet been triaged by the team. labels Apr 30, 2024
@ValarDragon
Copy link
Collaborator Author

ValarDragon commented Apr 30, 2024

Oh we have a really easy fix for Proto writer is supposed to use an internal buffer. We make a new object and then must re-allocate the internal buffer on every packet.

We can just pass the sendRoutine protoWriter through the entire sendPacketMsg stack. I'll PR that after #2953

github-merge-queue bot pushed a commit that referenced this issue May 2, 2024
…et delays (#2952)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to #2951 

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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
mergify bot pushed a commit that referenced this issue May 2, 2024
…et delays (#2952)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to #2951

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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

(cherry picked from commit fbf5c60)
mergify bot pushed a commit that referenced this issue May 2, 2024
…et delays (#2952)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to #2951

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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

(cherry picked from commit fbf5c60)

# Conflicts:
#	p2p/conn/connection.go
mergify bot pushed a commit that referenced this issue May 2, 2024
…et delays (#2952)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to #2951

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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

(cherry picked from commit fbf5c60)

# Conflicts:
#	p2p/conn/connection.go
melekes added a commit that referenced this issue May 2, 2024
…et delays (backport #2952) (#2975)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to #2951 

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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 #2952 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
melekes added a commit that referenced this issue May 2, 2024
…et delays (backport #2952) (#2974)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to #2951 

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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 #2952 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
melekes added a commit that referenced this issue May 2, 2024
…et delays (backport #2952) (#2973)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to #2951 

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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 #2952 done by
[Mergify](https://mergify.com).

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
czarcas7ic pushed a commit to osmosis-labs/cometbft that referenced this issue May 2, 2024
…et delays (backport cometbft#2952) (cometbft#2975)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to cometbft#2951 

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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 cometbft#2952 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
czarcas7ic added a commit to osmosis-labs/cometbft that referenced this issue May 3, 2024
…et delays (backport cometbft#2952) (cometbft#2975) (#42)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to cometbft#2951 

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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 cometbft#2952 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
mergify bot pushed a commit to osmosis-labs/cometbft that referenced this issue May 3, 2024
…et delays (backport cometbft#2952) (cometbft#2975) (#42)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to cometbft#2951

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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 cometbft#2952 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
(cherry picked from commit 9ee1d13)
mergify bot pushed a commit to osmosis-labs/cometbft that referenced this issue May 3, 2024
…et delays (backport cometbft#2952) (cometbft#2975) (#42)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to cometbft#2951

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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 cometbft#2952 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
(cherry picked from commit 9ee1d13)
czarcas7ic added a commit to osmosis-labs/cometbft that referenced this issue May 3, 2024
…et delays (backport cometbft#2952) (cometbft#2975) (#42) (#52)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to cometbft#2951

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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 cometbft#2952 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
(cherry picked from commit 9ee1d13)

Co-authored-by: Adam Tucker <[email protected]>
czarcas7ic added a commit to osmosis-labs/cometbft that referenced this issue May 3, 2024
…et delays (backport cometbft#2952) (cometbft#2975) (#42) (#53)

Somehow this minint call is appearing in CPU profiles. Its too large of
an appearance to be declared as noise, but I really don't get why its
there. Must be some weird system effect I don't get. (perhaps due to
being the result of a function call for a slice index? idk)

This PR just removes the function call, since its not needed, as we
already branch on the if statement.

Profile showing it:

![image](https://github.com/cometbft/cometbft/assets/6440154/a12c8918-e4ce-4a3d-bad4-5ae678988f94)

Contributes a 3% improvement to cometbft#2951

---

#### PR checklist

- [x] Tests written/updated - fully compatible with what already exists
- [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 cometbft#2952 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
(cherry picked from commit 9ee1d13)

Co-authored-by: Adam Tucker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-triage This issue/PR has not yet been triaged by the team.
Projects
None yet
Development

No branches or pull requests

1 participant