From 200e9dfa29d03ba4c881b0e9fb620b7ff4910ef8 Mon Sep 17 00:00:00 2001 From: Bradley Olson <34992650+BradleyOlson64@users.noreply.github.com> Date: Thu, 16 Mar 2023 02:00:56 -0700 Subject: [PATCH] Issue 4393: Correcting Unnecessary Use of Arc (#6882) * Added participation and queue sizes metrics * First draft of all metric code * Tests pass * Changed Metrics to field on participation + queues * fmt * Improving naming * Refactor, placing timer in ParticipationRequest * fmt * Final cleanup * Revert "Final cleanup" This reverts commit 02e5608df64b2e0f7810905e4508673b2037d351. * Changing metric names * Implementing Eq only for unit tests * fmt * Removing Clone trait from ParticipationRequest * fmt * Moved clone functionality to tests helper --- .../dispute-coordinator/src/initialized.rs | 2 +- .../node/core/dispute-coordinator/src/lib.rs | 2 +- .../src/participation/queues/mod.rs | 12 +++--- .../src/participation/queues/tests.rs | 41 ++++++++++++------- .../src/participation/tests.rs | 2 +- 5 files changed, 35 insertions(+), 24 deletions(-) diff --git a/polkadot/node/core/dispute-coordinator/src/initialized.rs b/polkadot/node/core/dispute-coordinator/src/initialized.rs index 620c58fbb7..2dbba5be32 100644 --- a/polkadot/node/core/dispute-coordinator/src/initialized.rs +++ b/polkadot/node/core/dispute-coordinator/src/initialized.rs @@ -916,7 +916,7 @@ impl Initialized { } else { self.metrics.on_queued_best_effort_participation(); } - let request_timer = Arc::new(self.metrics.time_participation_pipeline()); + let request_timer = self.metrics.time_participation_pipeline(); let r = self .participation .queue_participation( diff --git a/polkadot/node/core/dispute-coordinator/src/lib.rs b/polkadot/node/core/dispute-coordinator/src/lib.rs index de155c1e8f..d82c3a06c6 100644 --- a/polkadot/node/core/dispute-coordinator/src/lib.rs +++ b/polkadot/node/core/dispute-coordinator/src/lib.rs @@ -347,7 +347,7 @@ impl DisputeCoordinatorSubsystem { ?candidate_hash, "Found valid dispute, with no vote from us on startup - participating." ); - let request_timer = Arc::new(self.metrics.time_participation_pipeline()); + let request_timer = self.metrics.time_participation_pipeline(); participation_requests.push(( ParticipationPriority::with_priority_if(is_included), ParticipationRequest::new( diff --git a/polkadot/node/core/dispute-coordinator/src/participation/queues/mod.rs b/polkadot/node/core/dispute-coordinator/src/participation/queues/mod.rs index cbfb71e20b..ebe742ac8f 100644 --- a/polkadot/node/core/dispute-coordinator/src/participation/queues/mod.rs +++ b/polkadot/node/core/dispute-coordinator/src/participation/queues/mod.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use std::{cmp::Ordering, collections::BTreeMap, sync::Arc}; +use std::{cmp::Ordering, collections::BTreeMap}; use futures::channel::oneshot; use polkadot_node_subsystem::{messages::ChainApiMessage, overseer}; @@ -65,12 +65,12 @@ pub struct Queues { } /// A dispute participation request that can be queued. -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct ParticipationRequest { candidate_hash: CandidateHash, candidate_receipt: CandidateReceipt, session: SessionIndex, - _request_timer: Arc>, // Sends metric data when request is dropped + _request_timer: Option, // Sends metric data when request is dropped } /// Whether a `ParticipationRequest` should be put on best-effort or the priority queue. @@ -117,7 +117,7 @@ impl ParticipationRequest { pub fn new( candidate_receipt: CandidateReceipt, session: SessionIndex, - request_timer: Arc>, + request_timer: Option, ) -> Self { Self { candidate_hash: candidate_receipt.hash(), @@ -142,8 +142,8 @@ impl ParticipationRequest { } } -// We want to compare participation requests in unit tests, so we -// only implement Eq for tests. +// We want to compare and clone participation requests in unit tests, so we +// only implement Eq and Clone for tests. #[cfg(test)] impl PartialEq for ParticipationRequest { fn eq(&self, other: &Self) -> bool { diff --git a/polkadot/node/core/dispute-coordinator/src/participation/queues/tests.rs b/polkadot/node/core/dispute-coordinator/src/participation/queues/tests.rs index 63df0d0a11..f4623e7638 100644 --- a/polkadot/node/core/dispute-coordinator/src/participation/queues/tests.rs +++ b/polkadot/node/core/dispute-coordinator/src/participation/queues/tests.rs @@ -18,7 +18,6 @@ use crate::{metrics::Metrics, ParticipationPriority}; use ::test_helpers::{dummy_candidate_receipt, dummy_hash}; use assert_matches::assert_matches; use polkadot_primitives::{BlockNumber, Hash}; -use std::sync::Arc; use super::{CandidateComparator, ParticipationRequest, QueueError, Queues}; @@ -27,7 +26,7 @@ fn make_participation_request(hash: Hash) -> ParticipationRequest { let mut receipt = dummy_candidate_receipt(dummy_hash()); // make it differ: receipt.commitments_hash = hash; - let request_timer = Arc::new(Metrics::default().time_participation_pipeline()); + let request_timer = Metrics::default().time_participation_pipeline(); ParticipationRequest::new(receipt, 1, request_timer) } @@ -39,6 +38,18 @@ fn make_dummy_comparator( CandidateComparator::new_dummy(relay_parent, *req.candidate_hash()) } +/// Make a partial clone of the given ParticipationRequest, just missing +/// the request_timer field. We prefer this helper to implementing Clone +/// for ParticipationRequest, since we only clone requests in tests. +fn clone_request(request: &ParticipationRequest) -> ParticipationRequest { + ParticipationRequest { + candidate_receipt: request.candidate_receipt.clone(), + candidate_hash: request.candidate_hash.clone(), + session: request.session, + _request_timer: None, + } +} + /// Check that dequeuing acknowledges order. /// /// Any priority item will be dequeued before any best effort items, priority and best effort with @@ -59,35 +70,35 @@ fn ordering_works_as_expected() { .queue_with_comparator( make_dummy_comparator(&req1, Some(1)), ParticipationPriority::BestEffort, - req1.clone(), + clone_request(&req1), ) .unwrap(); queue .queue_with_comparator( make_dummy_comparator(&req_prio, Some(1)), ParticipationPriority::Priority, - req_prio.clone(), + clone_request(&req_prio), ) .unwrap(); queue .queue_with_comparator( make_dummy_comparator(&req3, Some(2)), ParticipationPriority::BestEffort, - req3.clone(), + clone_request(&req3), ) .unwrap(); queue .queue_with_comparator( make_dummy_comparator(&req_prio_2, Some(2)), ParticipationPriority::Priority, - req_prio_2.clone(), + clone_request(&req_prio_2), ) .unwrap(); queue .queue_with_comparator( make_dummy_comparator(&req5_unknown_parent, None), ParticipationPriority::BestEffort, - req5_unknown_parent.clone(), + clone_request(&req5_unknown_parent), ) .unwrap(); assert_matches!( @@ -132,14 +143,14 @@ fn candidate_is_only_dequeued_once() { .queue_with_comparator( make_dummy_comparator(&req1, None), ParticipationPriority::BestEffort, - req1.clone(), + clone_request(&req1), ) .unwrap(); queue .queue_with_comparator( make_dummy_comparator(&req_prio, Some(1)), ParticipationPriority::Priority, - req_prio.clone(), + clone_request(&req_prio), ) .unwrap(); // Insert same best effort again: @@ -147,7 +158,7 @@ fn candidate_is_only_dequeued_once() { .queue_with_comparator( make_dummy_comparator(&req1, None), ParticipationPriority::BestEffort, - req1.clone(), + clone_request(&req1), ) .unwrap(); // insert same prio again: @@ -155,7 +166,7 @@ fn candidate_is_only_dequeued_once() { .queue_with_comparator( make_dummy_comparator(&req_prio, Some(1)), ParticipationPriority::Priority, - req_prio.clone(), + clone_request(&req_prio), ) .unwrap(); // Insert first as best effort: @@ -163,7 +174,7 @@ fn candidate_is_only_dequeued_once() { .queue_with_comparator( make_dummy_comparator(&req_best_effort_then_prio, Some(2)), ParticipationPriority::BestEffort, - req_best_effort_then_prio.clone(), + clone_request(&req_best_effort_then_prio), ) .unwrap(); // Then as prio: @@ -171,7 +182,7 @@ fn candidate_is_only_dequeued_once() { .queue_with_comparator( make_dummy_comparator(&req_best_effort_then_prio, Some(2)), ParticipationPriority::Priority, - req_best_effort_then_prio.clone(), + clone_request(&req_best_effort_then_prio), ) .unwrap(); @@ -183,7 +194,7 @@ fn candidate_is_only_dequeued_once() { .queue_with_comparator( make_dummy_comparator(&req_prio_then_best_effort, Some(3)), ParticipationPriority::Priority, - req_prio_then_best_effort.clone(), + clone_request(&req_prio_then_best_effort), ) .unwrap(); // Then as best effort: @@ -191,7 +202,7 @@ fn candidate_is_only_dequeued_once() { .queue_with_comparator( make_dummy_comparator(&req_prio_then_best_effort, Some(3)), ParticipationPriority::BestEffort, - req_prio_then_best_effort.clone(), + clone_request(&req_prio_then_best_effort), ) .unwrap(); diff --git a/polkadot/node/core/dispute-coordinator/src/participation/tests.rs b/polkadot/node/core/dispute-coordinator/src/participation/tests.rs index a6e5f86616..a5fbe2d6a5 100644 --- a/polkadot/node/core/dispute-coordinator/src/participation/tests.rs +++ b/polkadot/node/core/dispute-coordinator/src/participation/tests.rs @@ -72,7 +72,7 @@ async fn participate_with_commitments_hash( }; let session = 1; - let request_timer = Arc::new(participation.metrics.time_participation_pipeline()); + let request_timer = participation.metrics.time_participation_pipeline(); let req = ParticipationRequest::new(candidate_receipt, session, request_timer); participation