highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-05-06 09:50:55 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#26749: refactor: Use move semantics instead of custom swap functions

Browse Source 
95ad70ab652ddde7de65f633c36c1378b26a313a test: Default initialize `should_freeze` to `true` (Hennadii Stepanov)
cea50521fe810111a8a3c84ad14f944eafb5b658 refactor: Drop no longer used `swap` member functions (Hennadii Stepanov)
a87fb6bee5a7fb0879b3adea9a29997f1331acb0 clang-tidy: Fix modernize-use-default-member-init in `CScriptCheck` (Hennadii Stepanov)
b4bed5c1f98c0eed18f52fdcea11a420c10ed98d refactor: Drop no longer used `CScriptCheck()` default constructor (Hennadii Stepanov)
d8427cc28e3a9ac3319fb452b16661957c812b8f refactor: Use move semantics in `CCheckQueue::Loop` (Hennadii Stepanov)
9a0b5241396efe3b3ceb3931717c30bb94f99bfb clang-tidy, test: Fix bugprone-use-after-move in `Correct_Queue_range()` (Hennadii Stepanov)
04831fee6dca3eb86cd1d6b9ef879b296263fe35 refactor: Make move semantics explicit for callers (Hennadii Stepanov)
6c2d5972f3544c4f3e987828a99e88f27b62cf87 refactor: Use move semantics in `CCheckQueue::Add` (Hennadii Stepanov)
06820032142a75cc3c5b832045058bc6f6f74786 test, refactor: Avoid `CScriptCheck::swap` in `transaction_tests` (Hennadii Stepanov)
15209d97c6aad7d5c199fe007ad39b91c8ee6562 consensus, refactor: Avoid `CScriptCheck::swap` in `CheckInputScripts` (Hennadii Stepanov)

Pull request description:

  This PR makes code more succinct and readable by using move semantics.

ACKs for top commit:
  martinus:
    re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a
  achow101:
    ACK 95ad70ab652ddde7de65f633c36c1378b26a313a
  TheCharlatan:
    re-ACK 95ad70ab65
  MarcoFalke:
    re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a 🚥

Tree-SHA512: adda760891b12d252dc9b823fe7c41eed660364b6fb1a69f17607d7a31eb0bbb82a80d154a7acfaa241b5de37d42a293c2b6e059f26a8e92d88d3a87c99768fb
This commit is contained in:
fanquake 2023-03-22 10:57:45 +00:00
commit a70911492f
No known key found for this signature in database
GPG Key ID: 2EEB9F5CC09526C1
7 changed files with 45 additions and 82 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
src/bench/checkqueue.cpp
View File

@ -29,7 +29,6 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench)
struct PrevectorJob {
prevector<PREVECTOR_SIZE, uint8_t> p;
PrevectorJob() = default;
explicit PrevectorJob(FastRandomContext& insecure_rand){
p.resize(insecure_rand.randrange(PREVECTOR_SIZE*2));
}
@ -37,10 +36,6 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench)
{
return true;
}
void swap(PrevectorJob& x) noexcept
{
p.swap(x.p);
};
};
CCheckQueue<PrevectorJob> queue {QUEUE_BATCH_SIZE};
// The main thread should be counted to prevent thread oversubscription, and
@ -60,7 +55,7 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench)
// Make insecure_rand here so that each iteration is identical.
CCheckQueueControl<PrevectorJob> control(&queue);
for (auto vChecks : vBatches) {
control.Add(vChecks);
control.Add(std::move(vChecks));
}
// control waits for completion by RAII, but
// it is done explicitly here for clarity

25
src/checkqueue.h
View File

@ -11,6 +11,7 @@
#include <util/threadnames.h>
#include <algorithm>
#include <iterator>
#include <vector>
template <typename T>
@ -111,13 +112,9 @@ private:
// * Try to account for idle jobs which will instantly start helping.
// * Don't do batches smaller than 1 (duh), or larger than nBatchSize.
nNow = std::max(1U, std::min(nBatchSize, (unsigned int)queue.size() / (nTotal + nIdle + 1)));
vChecks.resize(nNow);
for (unsigned int i = 0; i < nNow; i++) {
// We want the lock on the m_mutex to be as short as possible, so swap jobs from the global
// queue to the local batch vector instead of copying.
vChecks[i].swap(queue.back());
queue.pop_back();
}
auto start_it = queue.end() - nNow;
vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end()));
queue.erase(start_it, queue.end());
// Check whether we need to do work at all
fOk = fAllOk;
}
@ -165,7 +162,7 @@ public:
}
//! Add a batch of checks to the queue
void Add(std::vector<T>& vChecks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
void Add(std::vector<T>&& vChecks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
if (vChecks.empty()) {
return;
@ -173,10 +170,7 @@ public:
{
LOCK(m_mutex);
for (T& check : vChecks) {
queue.emplace_back();
check.swap(queue.back());
}
queue.insert(queue.end(), std::make_move_iterator(vChecks.begin()), std::make_move_iterator(vChecks.end()));
nTodo += vChecks.size();
}
@ -239,10 +233,11 @@ public:
return fRet;
}
void Add(std::vector<T>& vChecks)
void Add(std::vector<T>&& vChecks)
{
if (pqueue != nullptr)
pqueue->Add(vChecks);
if (pqueue != nullptr) {
pqueue->Add(std::move(vChecks));
}
}
~CCheckQueueControl()

51
src/test/checkqueue_tests.cpp
View File

@ -43,7 +43,6 @@ struct FakeCheck {
{
return true;
}
void swap(FakeCheck& x) noexcept {};
};
struct FakeCheckCheckCompletion {
@ -53,7 +52,6 @@ struct FakeCheckCheckCompletion {
n_calls.fetch_add(1, std::memory_order_relaxed);
return true;
}
void swap(FakeCheckCheckCompletion& x) noexcept {};
};
struct FailingCheck {
@ -64,10 +62,6 @@ struct FailingCheck {
{
return !fails;
}
void swap(FailingCheck& x) noexcept
{
std::swap(fails, x.fails);
};
};
struct UniqueCheck {
@ -82,10 +76,6 @@ struct UniqueCheck {
results.insert(check_id);
return true;
}
void swap(UniqueCheck& x) noexcept
{
std::swap(x.check_id, check_id);
};
};
@ -113,19 +103,13 @@ struct MemoryCheck {
{
fake_allocated_memory.fetch_sub(b, std::memory_order_relaxed);
};
void swap(MemoryCheck& x) noexcept
{
std::swap(b, x.b);
};
};
struct FrozenCleanupCheck {
static std::atomic<uint64_t> nFrozen;
static std::condition_variable cv;
static std::mutex m;
// Freezing can't be the default initialized behavior given how the queue
// swaps in default initialized Checks.
bool should_freeze {false};
bool should_freeze{true};
bool operator()() const
{
return true;
@ -140,10 +124,17 @@ struct FrozenCleanupCheck {
cv.wait(l, []{ return nFrozen.load(std::memory_order_relaxed) == 0;});
}
}
void swap(FrozenCleanupCheck& x) noexcept
FrozenCleanupCheck(FrozenCleanupCheck&& other) noexcept
{
std::swap(should_freeze, x.should_freeze);
};
should_freeze = other.should_freeze;
other.should_freeze = false;
}
FrozenCleanupCheck& operator=(FrozenCleanupCheck&& other) noexcept
{
should_freeze = other.should_freeze;
other.should_freeze = false;
return *this;
}
};
// Static Allocations
@ -173,14 +164,16 @@ static void Correct_Queue_range(std::vector<size_t> range)
small_queue->StartWorkerThreads(SCRIPT_CHECK_THREADS);
// Make vChecks here to save on malloc (this test can be slow...)
std::vector<FakeCheckCheckCompletion> vChecks;
vChecks.reserve(9);
for (const size_t i : range) {
size_t total = i;
FakeCheckCheckCompletion::n_calls = 0;
CCheckQueueControl<FakeCheckCheckCompletion> control(small_queue.get());
while (total) {
vChecks.resize(std::min(total, (size_t) InsecureRandRange(10)));
vChecks.clear();
vChecks.resize(std::min<size_t>(total, InsecureRandRange(10)));
total -= vChecks.size();
control.Add(vChecks);
control.Add(std::move(vChecks));
}
BOOST_REQUIRE(control.Wait());
if (FakeCheckCheckCompletion::n_calls != i) {
@ -242,7 +235,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure)
vChecks.reserve(r);
for (size_t k = 0; k < r && remaining; k++, remaining--)
vChecks.emplace_back(remaining == 1);
control.Add(vChecks);
control.Add(std::move(vChecks));
}
bool success = control.Wait();
if (i > 0) {
@ -267,7 +260,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure)
std::vector<FailingCheck> vChecks;
vChecks.resize(100, false);
vChecks[99] = end_fails;
control.Add(vChecks);
control.Add(std::move(vChecks));
}
bool r =control.Wait();
BOOST_REQUIRE(r != end_fails);
@ -293,7 +286,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_UniqueCheck)
std::vector<UniqueCheck> vChecks;
for (size_t k = 0; k < r && total; k++)
vChecks.emplace_back(--total);
control.Add(vChecks);
control.Add(std::move(vChecks));
}
}
{
@ -331,7 +324,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Memory)
// to catch any sort of deallocation failure
vChecks.emplace_back(total == 0 || total == i || total == i/2);
}
control.Add(vChecks);
control.Add(std::move(vChecks));
}
}
BOOST_REQUIRE_EQUAL(MemoryCheck::fake_allocated_memory, 0U);
@ -349,11 +342,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup)
std::thread t0([&]() {
CCheckQueueControl<FrozenCleanupCheck> control(queue.get());
std::vector<FrozenCleanupCheck> vChecks(1);
// Freezing can't be the default initialized behavior given how the queue
// swaps in default initialized Checks (otherwise freezing destructor
// would get called twice).
vChecks[0].should_freeze = true;
control.Add(vChecks);
control.Add(std::move(vChecks));
bool waitResult = control.Wait(); // Hangs here
assert(waitResult);
});

12
src/test/fuzz/checkqueue.cpp
View File

@ -13,9 +13,7 @@
namespace {
struct DumbCheck {
const bool result = false;
DumbCheck() = default;
bool result = false;
explicit DumbCheck(const bool _result) : result(_result)
{
@ -25,10 +23,6 @@ struct DumbCheck {
{
return result;
}
void swap(DumbCheck& x) noexcept
{
}
};
} // namespace
@ -48,7 +42,7 @@ FUZZ_TARGET(checkqueue)
checks_2.emplace_back(result);
}
if (fuzzed_data_provider.ConsumeBool()) {
check_queue_1.Add(checks_1);
check_queue_1.Add(std::move(checks_1));
}
if (fuzzed_data_provider.ConsumeBool()) {
(void)check_queue_1.Wait();
@ -56,7 +50,7 @@ FUZZ_TARGET(checkqueue)
CCheckQueueControl<DumbCheck> check_queue_control{&check_queue_2};
if (fuzzed_data_provider.ConsumeBool()) {
check_queue_control.Add(checks_2);
check_queue_control.Add(std::move(checks_2));
}
if (fuzzed_data_provider.ConsumeBool()) {
(void)check_queue_control.Wait();

6
src/test/transaction_tests.cpp
View File

@ -547,10 +547,8 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction)
for(uint32_t i = 0; i < mtx.vin.size(); i++) {
std::vector<CScriptCheck> vChecks;
CScriptCheck check(coins[tx.vin[i].prevout.n].out, tx, i, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false, &txdata);
vChecks.push_back(CScriptCheck());
check.swap(vChecks.back());
control.Add(vChecks);
vChecks.emplace_back(coins[tx.vin[i].prevout.n].out, tx, i, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false, &txdata);
control.Add(std::move(vChecks));
}
bool controlCheck = control.Wait();

5
src/validation.cpp
View File

@ -1815,8 +1815,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
// Verify signature
CScriptCheck check(txdata.m_spent_outputs[i], tx, i, flags, cacheSigStore, &txdata);
if (pvChecks) {
pvChecks->push_back(CScriptCheck());
check.swap(pvChecks->back());
pvChecks->emplace_back(std::move(check));
} else if (!check()) {
if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
// Check whether the failure was caused by a
@ -2325,7 +2324,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
return error("ConnectBlock(): CheckInputScripts on %s failed with %s",
tx.GetHash().ToString(), state.ToString());
}
control.Add(vChecks);
control.Add(std::move(vChecks));
}
CTxUndo undoDummy;

21
src/validation.h
View File

@ -314,27 +314,20 @@ private:
unsigned int nIn;
unsigned int nFlags;
bool cacheStore;
ScriptError error;
ScriptError error{SCRIPT_ERR_UNKNOWN_ERROR};
PrecomputedTransactionData *txdata;
public:
CScriptCheck(): ptxTo(nullptr), nIn(0), nFlags(0), cacheStore(false), error(SCRIPT_ERR_UNKNOWN_ERROR) {}
CScriptCheck(const CTxOut& outIn, const CTransaction& txToIn, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn) :
m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), error(SCRIPT_ERR_UNKNOWN_ERROR), txdata(txdataIn) { }
m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), txdata(txdataIn) { }
CScriptCheck(const CScriptCheck&) = delete;
CScriptCheck& operator=(const CScriptCheck&) = delete;
CScriptCheck(CScriptCheck&&) = default;
CScriptCheck& operator=(CScriptCheck&&) = default;
bool operator()();
void swap(CScriptCheck& check) noexcept
{
std::swap(ptxTo, check.ptxTo);
std::swap(m_tx_out, check.m_tx_out);
std::swap(nIn, check.nIn);
std::swap(nFlags, check.nFlags);
std::swap(cacheStore, check.cacheStore);
std::swap(error, check.error);
std::swap(txdata, check.txdata);
}
ScriptError GetScriptError() const { return error; }
};