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

perf: Minor speedup to consenus metrics MarkLateVote #3017

Merged

Conversation

ValarDragon
Copy link
Collaborator

@ValarDragon ValarDragon commented May 6, 2024

Minor speedup to metrics MarkLateVote.

I saw the excess string allocation calls, so made a quick PR to remove it. This saves between .18s-25s from the consensus mutex across this one hour block sync. (Likely not at all consensus critical) It also appears in receiving votes

image

(Saves ToLower(), .String(), .TrimPrefix() and newObject calls. The new call has comparable complexity to .String())


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

@ValarDragon ValarDragon requested review from a team as code owners May 6, 2024 23:43
Copy link
Contributor

@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.

Thanks @ValarDragon ❤️

@melekes melekes added backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x Tell Mergify to backport the PR to v1.x labels May 7, 2024
@melekes melekes added this pull request to the merge queue May 7, 2024
Merged via the queue into cometbft:main with commit 4a37c00 May 7, 2024
36 checks passed
mergify bot pushed a commit that referenced this pull request May 7, 2024
Minor speedup to metrics MarkLateVote.

I saw the excess string allocation calls, so made a quick PR to remove
it. This saves between .18s-25s from the consensus mutex across this one
hour block sync. (Likely not at all consensus critical) It also appears
in receiving votes

![image](https://github.com/cometbft/cometbft/assets/6440154/61875b0d-b419-4900-a74c-76c65087bca4)

(Saves ToLower(), .String(), .TrimPrefix() and newObject calls. The new
call has comparable complexity to .String())

---

#### PR checklist

- [x] Tests written/updated
- [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 4a37c00)
mergify bot pushed a commit that referenced this pull request May 7, 2024
Minor speedup to metrics MarkLateVote.

I saw the excess string allocation calls, so made a quick PR to remove
it. This saves between .18s-25s from the consensus mutex across this one
hour block sync. (Likely not at all consensus critical) It also appears
in receiving votes

![image](https://github.com/cometbft/cometbft/assets/6440154/61875b0d-b419-4900-a74c-76c65087bca4)

(Saves ToLower(), .String(), .TrimPrefix() and newObject calls. The new
call has comparable complexity to .String())

---

#### PR checklist

- [x] Tests written/updated
- [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 4a37c00)

# Conflicts:
#	consensus/metrics.go
mergify bot pushed a commit that referenced this pull request May 7, 2024
Minor speedup to metrics MarkLateVote.

I saw the excess string allocation calls, so made a quick PR to remove
it. This saves between .18s-25s from the consensus mutex across this one
hour block sync. (Likely not at all consensus critical) It also appears
in receiving votes

![image](https://github.com/cometbft/cometbft/assets/6440154/61875b0d-b419-4900-a74c-76c65087bca4)

(Saves ToLower(), .String(), .TrimPrefix() and newObject calls. The new
call has comparable complexity to .String())

---

#### PR checklist

- [x] Tests written/updated
- [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 4a37c00)

# Conflicts:
#	consensus/metrics.go
melekes added a commit that referenced this pull request May 7, 2024
…#3024)

Minor speedup to metrics MarkLateVote.

I saw the excess string allocation calls, so made a quick PR to remove
it. This saves between .18s-25s from the consensus mutex across this one
hour block sync. (Likely not at all consensus critical) It also appears
in receiving votes


![image](https://github.com/cometbft/cometbft/assets/6440154/61875b0d-b419-4900-a74c-76c65087bca4)

(Saves ToLower(), .String(), .TrimPrefix() and newObject calls. The new
call has comparable complexity to .String())

---

#### PR checklist

- [x] Tests written/updated
- [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 #3017 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>
melekes added a commit that referenced this pull request May 7, 2024
…#3026)

Minor speedup to metrics MarkLateVote.

I saw the excess string allocation calls, so made a quick PR to remove
it. This saves between .18s-25s from the consensus mutex across this one
hour block sync. (Likely not at all consensus critical) It also appears
in receiving votes


![image](https://github.com/cometbft/cometbft/assets/6440154/61875b0d-b419-4900-a74c-76c65087bca4)

(Saves ToLower(), .String(), .TrimPrefix() and newObject calls. The new
call has comparable complexity to .String())

---

#### PR checklist

- [x] Tests written/updated
- [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 #3017 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>
melekes added a commit that referenced this pull request May 7, 2024
…#3025)

Minor speedup to metrics MarkLateVote.

I saw the excess string allocation calls, so made a quick PR to remove
it. This saves between .18s-25s from the consensus mutex across this one
hour block sync. (Likely not at all consensus critical) It also appears
in receiving votes


![image](https://github.com/cometbft/cometbft/assets/6440154/61875b0d-b419-4900-a74c-76c65087bca4)

(Saves ToLower(), .String(), .TrimPrefix() and newObject calls. The new
call has comparable complexity to .String())

---

#### PR checklist

- [x] Tests written/updated
- [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 #3017 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>
ValarDragon added a commit to osmosis-labs/cometbft that referenced this pull request May 25, 2024
…ft#3017) (cometbft#3026)

Minor speedup to metrics MarkLateVote.

I saw the excess string allocation calls, so made a quick PR to remove
it. This saves between .18s-25s from the consensus mutex across this one
hour block sync. (Likely not at all consensus critical) It also appears
in receiving votes


![image](https://github.com/cometbft/cometbft/assets/6440154/61875b0d-b419-4900-a74c-76c65087bca4)

(Saves ToLower(), .String(), .TrimPrefix() and newObject calls. The new
call has comparable complexity to .String())

---

#### PR checklist

- [x] Tests written/updated
- [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#3017 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>
ValarDragon added a commit to osmosis-labs/cometbft that referenced this pull request May 25, 2024
perf: Minor speedup to consenus metrics MarkLateVote (backport cometbft#3017)…
mergify bot added a commit to osmosis-labs/cometbft that referenced this pull request May 28, 2024
…ft#3017) (cometbft#3026)

Minor speedup to metrics MarkLateVote.

I saw the excess string allocation calls, so made a quick PR to remove
it. This saves between .18s-25s from the consensus mutex across this one
hour block sync. (Likely not at all consensus critical) It also appears
in receiving votes

![image](https://github.com/cometbft/cometbft/assets/6440154/61875b0d-b419-4900-a74c-76c65087bca4)

(Saves ToLower(), .String(), .TrimPrefix() and newObject calls. The new
call has comparable complexity to .String())

---

#### PR checklist

- [x] Tests written/updated
- [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#3017 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>
(cherry picked from commit 29dd011)
PaddyMc pushed a commit to osmosis-labs/cometbft that referenced this pull request May 28, 2024
…ft#3017)… (backport #74) (#79)

* perf: Minor speedup to consenus metrics MarkLateVote (backport cometbft#3017) (cometbft#3026)

Minor speedup to metrics MarkLateVote.

I saw the excess string allocation calls, so made a quick PR to remove
it. This saves between .18s-25s from the consensus mutex across this one
hour block sync. (Likely not at all consensus critical) It also appears
in receiving votes

![image](https://github.com/cometbft/cometbft/assets/6440154/61875b0d-b419-4900-a74c-76c65087bca4)

(Saves ToLower(), .String(), .TrimPrefix() and newObject calls. The new
call has comparable complexity to .String())

---

#### PR checklist

- [x] Tests written/updated
- [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#3017 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>
(cherry picked from commit 29dd011)

* Add changelog

(cherry picked from commit 6392e64)

# Conflicts:
#	CHANGELOG.md

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <dojha@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x Tell Mergify to backport the PR to v1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants