Skip to content

bench: Adds a benchmark for CheckInputScripts #29745

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

bench: Adds a benchmark for CheckInputScripts #29745

wants to merge 1 commit into from

Conversation

kevkevinpal
Copy link
Contributor

@kevkevinpal kevkevinpal commented Mar 26, 2024 •
edited
Loading

Creates new benchmark for CheckInputScripts

Motivated by this comment #17175 (comment)

I haven't really done benchmarking a ton before so any comments would be useful

What the benchmark looks like when I run ./src/bench/bench_bitcoin

|           ns/script |            script/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|            6,946.20 |          143,963.61 |    5.2% |      0.01 | :wavy_dash: `BenchCheckInputScripts` (Unstable with ~148.0 iters. Increase `minEpochIterations` to e.g. 1480)

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 26, 2024 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30141 (kernel: De-globalize validation caches by TheCharlatan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot added the Tests label Mar 26, 2024
@kevkevinpal kevkevinpal force-pushed the benchCheckInputScripts branch from f9e0dfc to ec7eb07 Compare March 26, 2024 23:55
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/23129699098

@kevkevinpal kevkevinpal force-pushed the benchCheckInputScripts branch from ec7eb07 to 22d8759 Compare March 27, 2024 00:28
@kevkevinpal
Copy link
Contributor Author

looks like the checks are failing because it requires cs_main mutex so I'll see what I can do about that

@DrahtBot DrahtBot mentioned this pull request Mar 27, 2024
Draft
5 tasks
@kevkevinpal kevkevinpal marked this pull request as draft March 27, 2024 12:53
@DrahtBot DrahtBot mentioned this pull request Mar 27, 2024
Closed
l0rinc
l0rinc reviewed Mar 27, 2024
View reviewed changes
src/Makefile.bench.include
@@ -12,6 +12,7 @@ GENERATED_BENCH_FILES = $(RAW_BENCH_FILES:.raw=.raw.h)

bench_bench_bitcoin_SOURCES = \
$(RAW_BENCH_FILES) \
bench/check_input_scrips.cpp \
Copy link
Contributor

Choose a reason for hiding this comment

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

the rest of the benchmarks are in alphabetical order

src/validation.cpp
@@ -133,12 +133,6 @@ const CBlockIndex* Chainstate::FindForkInGlobalIndex(const CBlockLocator& locato
return m_chain.Genesis();
}

bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
Copy link
Contributor

Choose a reason for hiding this comment

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

please separate production changes from benchmark changes into commits with focused explanations

@DrahtBot DrahtBot mentioned this pull request May 23, 2024
Merged
@DrahtBot
Copy link
Contributor

Are you still working on this?

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 8, 2024

🐙 This pull request conflicts with the target branch and needs rebase.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 5, 2024

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@maflcko maflcko closed this Oct 9, 2024
@maflcko
Copy link
Member

maflcko commented Oct 9, 2024

Closing for now due to inactivity since the day it was opened. Leave a comment below if you want this re-opened, or open a new pull.

@kevkevinpal
Copy link
Contributor Author

Closing for now due to inactivity since the day it was opened. Leave a comment below if you want this re-opened, or open a new pull.

ya sorry about that I can leave it up for grabs if anyone else wants to look at it otherwise I can reopen when I have a more finalized PR open, again sorry for leaving it open but inactive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@l0rinc l0rinc l0rinc left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
CI failed Needs rebase Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kevkevinpal @DrahtBot @maflcko @l0rinc