refactor: make script Solver's often-unused solutions parameter optional #33757
Conversation
Make the `vSolutionsRet` parameter of `Solver()` nullable pointer allowing callers that only need the return value to avoid allocating and populating the solutions vector. Benchmark results show a 25.0% performance improvement (4,240,283 to 5,300,923 operations per second) for callers that don't need solutions. This approach addresses the performance concern raised in bitcoin#33645 more fundamentally than the vector reuse optimization, while simplifying the call sites at the same time. Co-authored-by: Raimo33 <[email protected]>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33757. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Was waiting for corecheck to finish, but it shows only |
|
The proposed approach increases complexity both at the callsites and in the implementation of the function itself. All the added Further, auto SolveType(CScript const& scriptPubKey) -> TxoutType;
auto Solve(CScript const& scriptPubKey) -> std::tuple<TxoutType, std::vector<std::vector<unsigned char>>>; |
I agree on having a separate method for this. It was my initial approach. It involves some code duplication and adding an extra test but might be worth it to avoid all the
Wouldn't this increase allocs/copies? I'd say it's perfectly fine to leave it as is with the vector reference as parameter. I'd lean towards returning it if it didn't involve |
|
If we're going to touch this code, my preference would be to go with @purpleKarrot's approach of two separate functions, moving the If the allocation overhead of that is a concern, I think the proper solution is a follow-up to get rid of the |
|
Thank you for the comments, appreciate the high-level review! @purpleKarrot, I though of both solutions you have suggested (I also passionately hate output parameters) but since I meant this as a call-site-simplification while eliminating useless work (though the cases where the
That could work, split the different results by type instead of an enum ( Thanks! |
5 participants
Summary
Many callsites only need to determine the script type and don't use the parsed solutions.
Previously, these callers still incurred the cost of allocating and populating a
std::vector<std::vector<unsigned char>>.Fix
Changing
Solversignature from reference to nullable pointer parameter withnullptrdefault allows calling the method for its result only. Note that we can't callstd::optionalhere because that would copy the values but we need the result to be provided from the outside to avoid the copies.Inside the
Solverwe guard all accesses to the solutions parameter.Callsites were changed to pass
&solutionsonly when they need the data and removed when we don't.Origin & Alternative
This was originally discovered in https://github.com/bitcoin/bitcoin/pull/32279/files#diff-060e8fd790fc1c3e18c64327a7395bb5b2d6d57db9792cc666bd8d7354a40c0bR1154-R1159 where we needed to test the allocation characteristics of different templates.
This supersedes the vector reuse optimization approach in #33645 (comment) by addressing the problem more fundamentally - avoiding unnecessary work entirely rather than optimizing it. The author of the different change was added as a coauthor.