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 02e5608.

* Changing metric names

* Implementing Eq only for unit tests

* fmt

* Removing Clone trait from ParticipationRequest

* fmt

* Moved clone functionality to tests helper

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(

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(

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 <http://www.gnu.org/licenses/>.
 
-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<Option<prometheus::HistogramTimer>>, // Sends metric data when request is dropped
+	_request_timer: Option<prometheus::HistogramTimer>, // 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<Option<prometheus::HistogramTimer>>,
+		request_timer: Option<prometheus::HistogramTimer>,
 	) -> 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 {

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,46 +143,46 @@ 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:
 	queue
 		.queue_with_comparator(
 			make_dummy_comparator(&req1, None),
 			ParticipationPriority::BestEffort,
-			req1.clone(),
+			clone_request(&req1),
 		)
 		.unwrap();
 	// insert same prio again:
 	queue
 		.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:
 	queue
 		.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:
 	queue
 		.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,15 +194,15 @@ 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:
 	queue
 		.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();
 

node/core/dispute-coordinator/src/participation/tests.rs

@@ -72,7 +72,7 @@ async fn participate_with_commitments_hash<Context>(
 	};
 	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