Skip to content

chore: fix benchmark tests #81

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

Closed
wants to merge 12 commits into from
Closed

Conversation

damiannolan
Copy link

@damiannolan damiannolan commented Mar 26, 2025

Gives love to benchmarks.

  • Adds a single build flag benchmarks instead of two different ones
  • Adjusts the make target to actually run benchmarks
  • Adjusts tests to account for deliverTx -> finalizeBlock
  • Fixes some test structure and compilation errors which exists pre-upgrade
  • Refactors PFB half second test to use a binary search based approach.

Ideally the results.md file could be replaced by some prettified markdown output from the benchmark tests

@damiannolan damiannolan marked this pull request as ready for review March 27, 2025 15:23
@damiannolan
Copy link
Author

currently looking at BenchmarkProcessProposal_PFB_Half_Second as it seems to run indefinitely

prepareProposalRequest := types.RequestPrepareProposal{
Txs: rawTxs[start:end],
Height: 10,
for start <= end {
Copy link
Author

Choose a reason for hiding this comment

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

I reworked this test to use a binary search based approach, as I found that when I was running these locally and trying to debug with the upgrade to v0.50 that in some instances the txNum would keep oscillating back and forth and wouldn't be able to hit within the bounds.

It seems to work nice enough now. The benchmark doesn't take quite as long to run, in some random instances it can hit the case where it won't find a ProcessProp time at the target but at least it does not run indefinitely, and running the bench again will normally succeed.
It could probably be further reworked to never hit this, but I didn't want to introduce too many changes. You will never get a deterministic ProcessProp time from what I can see.

Copy link

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

dope! thanks for fixing these

since the app PR is huge I think simply waiting to merge this would be beneficial, however I'll defer to others if they have a strong opinion

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