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(consensus/state): Change finalizeCommit to use applyVerifiedBloc… #39

Merged
merged 2 commits into from
May 2, 2024

Conversation

czarcas7ic
Copy link
Member

…k (backport cometbft#2928) (cometbft#2945)

Simplest component of cometbft#2854

We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in cometbft#2854 to remedy)

From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block.


PR checklist



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

…k (backport cometbft#2928) (cometbft#2945)

Simplest component of cometbft#2854 

We already run ValidateBlock in finalizeCommit, so this PR removes one
extra redundant call by using ApplyVerifiedBlock. (The other call is
also redundant, but that likely requires a more complex caching strategy
as noted in cometbft#2854 to remedy)

From my understanding of these benchmarks, at Osmosis 150 validators,
this should be saving ~13ms of execution time per block.

---

#### PR checklist

- [X] Tests written/updated - Theres no test to update, as its
impossible to reach the difference in codepaths!
- [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#2928 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
@czarcas7ic czarcas7ic added S:backport/v24 backport to the osmo-v24/v0.37.4 branch S:backport/v25 backport to the osmo-v25/v0.37.4 branch labels May 2, 2024
@czarcas7ic czarcas7ic marked this pull request as ready for review May 2, 2024 23:34
@czarcas7ic czarcas7ic merged commit 6fe6ee1 into osmo/v0.37.4 May 2, 2024
18 checks passed
mergify bot pushed a commit that referenced this pull request May 2, 2024
#39)

* perf(consensus/state): Change finalizeCommit to use applyVerifiedBlock (backport cometbft#2928) (cometbft#2945)

Simplest component of cometbft#2854

We already run ValidateBlock in finalizeCommit, so this PR removes one
extra redundant call by using ApplyVerifiedBlock. (The other call is
also redundant, but that likely requires a more complex caching strategy
as noted in cometbft#2854 to remedy)

From my understanding of these benchmarks, at Osmosis 150 validators,
this should be saving ~13ms of execution time per block.

---

#### PR checklist

- [X] Tests written/updated - Theres no test to update, as its
impossible to reach the difference in codepaths!
- [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#2928 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>

* fix merge conflict

---------

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 6fe6ee1)
mergify bot pushed a commit that referenced this pull request May 2, 2024
#39)

* perf(consensus/state): Change finalizeCommit to use applyVerifiedBlock (backport cometbft#2928) (cometbft#2945)

Simplest component of cometbft#2854

We already run ValidateBlock in finalizeCommit, so this PR removes one
extra redundant call by using ApplyVerifiedBlock. (The other call is
also redundant, but that likely requires a more complex caching strategy
as noted in cometbft#2854 to remedy)

From my understanding of these benchmarks, at Osmosis 150 validators,
this should be saving ~13ms of execution time per block.

---

#### PR checklist

- [X] Tests written/updated - Theres no test to update, as its
impossible to reach the difference in codepaths!
- [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#2928 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>

* fix merge conflict

---------

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 6fe6ee1)
czarcas7ic added a commit that referenced this pull request May 3, 2024
#39) (#46)

* perf(consensus/state): Change finalizeCommit to use applyVerifiedBlock (backport cometbft#2928) (cometbft#2945)

Simplest component of cometbft#2854

We already run ValidateBlock in finalizeCommit, so this PR removes one
extra redundant call by using ApplyVerifiedBlock. (The other call is
also redundant, but that likely requires a more complex caching strategy
as noted in cometbft#2854 to remedy)

From my understanding of these benchmarks, at Osmosis 150 validators,
this should be saving ~13ms of execution time per block.

---

#### PR checklist

- [X] Tests written/updated - Theres no test to update, as its
impossible to reach the difference in codepaths!
- [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#2928 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>

* fix merge conflict

---------

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 6fe6ee1)

Co-authored-by: Adam Tucker <[email protected]>
czarcas7ic added a commit that referenced this pull request May 3, 2024
#39) (#47)

* perf(consensus/state): Change finalizeCommit to use applyVerifiedBlock (backport cometbft#2928) (cometbft#2945)

Simplest component of cometbft#2854

We already run ValidateBlock in finalizeCommit, so this PR removes one
extra redundant call by using ApplyVerifiedBlock. (The other call is
also redundant, but that likely requires a more complex caching strategy
as noted in cometbft#2854 to remedy)

From my understanding of these benchmarks, at Osmosis 150 validators,
this should be saving ~13ms of execution time per block.

---

#### PR checklist

- [X] Tests written/updated - Theres no test to update, as its
impossible to reach the difference in codepaths!
- [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#2928 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>

* fix merge conflict

---------

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 6fe6ee1)

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
S:backport/v24 backport to the osmo-v24/v0.37.4 branch S:backport/v25 backport to the osmo-v25/v0.37.4 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant