test: Removed implicit CTransaction constructor calls from tests and benchmarks. #14908
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Dec 10, 2018
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
maflcko
reviewed
Dec 10, 2018
src/bench/ccoins_caching.cpp
Outdated
| @@ -76,10 +76,11 @@ static void CCoinsCaching(benchmark::State& state) | |||
| t1.vout[0].scriptPubKey << OP_1; | |||
|
|
|||
| // Benchmark. | |||
| CTransaction cTx(t1); | |||
There was a problem hiding this comment.
can be const and shout be snake case? tx_1?
maflcko
reviewed
Dec 10, 2018
src/bench/mempool_eviction.cpp
Outdated
| @@ -127,7 +128,7 @@ static void MempoolEviction(benchmark::State& state) | |||
| AddTx(tx6_r, 1100LL, pool); | |||
| AddTx(tx7_r, 9000LL, pool); | |||
| pool.TrimToSize(pool.DynamicMemoryUsage() * 3 / 4); | |||
| pool.TrimToSize(GetVirtualTransactionSize(tx1)); | |||
| pool.TrimToSize(GetVirtualTransactionSize(cTx)); | |||
|
Concept ACK Explicit is better than implicit – especially when it comes to conversions. |
|
explicit utACK 8db0c3d |
maflcko
pushed a commit
that referenced
this pull request
Dec 12, 2018
…rom tests and benchmarks. 8db0c3d Removed implicit CTransaction conversion from benchmaks (lucash-dev) ed61abe Removed implicit CTransaction constructor from tests (lucash-dev) Pull request description: This PR was split from #14906 and is a prerequisite for it. It updates tests and benchmarks, removing all implicit calls to `CTransaction(CMutableTransaction&)` constructors. This will make possible making the constructor explicit in the next PR. The original rationale for making the constructor explicit: - Conversion constructors should not be explicit unless there's a strong reason for it (in the opinion of, for example, https://google.github.io/styleguide/cppguide.html, and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Ro-conversion. Let me know your take on this). - This particular conversion is very costly -- it implies a serialization plus hash of the transaction. - Even though `CTransaction` and `CMutableTransaction` represent the same data, they have very different use cases and performance properties. - Making it explicit allows for easier reasoning of performance trade-offs. - There has been previous performance issues caused by unneeded use of this implicit conversion. - This PR creates a map for places to look for possible refactoring and performance gains (this benefit still holds if the PR is not merged). Tree-SHA512: de8073aa6ff8a3153bcbe10818616677ecf9598e4978d8a0b4c39a262e71c36be5679cec08554c760d1f011ba6d37350318248eef15f6d9b86f9e4462b2de0d2
This was referenced Dec 12, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR was split from #14906 and is a prerequisite for it.
It updates tests and benchmarks, removing all implicit calls to
CTransaction(CMutableTransaction&)constructors. This will make possible making the constructor explicit in the next PR.The original rationale for making the constructor explicit:
CTransactionandCMutableTransactionrepresent the same data, they have very different use cases and performance properties.