From c2e1b9d94b30fca10262d165b7fe8f7991b843bd Mon Sep 17 00:00:00 2001 From: John Spray Date: Mon, 13 May 2024 18:29:17 +0100 Subject: [PATCH] Avoid Mutex around physical size metric --- pageserver/src/metrics.rs | 58 ++++++++----------- pageserver/src/tenant.rs | 10 +--- .../src/tenant/remote_timeline_client.rs | 4 +- 3 files changed, 27 insertions(+), 45 deletions(-) diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs index b8c919a520..c9dbad7ae6 100644 --- a/pageserver/src/metrics.rs +++ b/pageserver/src/metrics.rs @@ -2335,6 +2335,7 @@ use pin_project_lite::pin_project; use std::collections::HashMap; use std::num::NonZeroUsize; use std::pin::Pin; +use std::sync::atomic::AtomicU64; use std::sync::{Arc, Mutex}; use std::task::{Context, Poll}; use std::time::{Duration, Instant}; @@ -2345,25 +2346,25 @@ use crate::tenant::mgr::TenantSlot; /// Maintain a per timeline gauge in addition to the global gauge. pub(crate) struct PerTimelineRemotePhysicalSizeGauge { - last_set: u64, + last_set: AtomicU64, gauge: UIntGauge, } impl PerTimelineRemotePhysicalSizeGauge { fn new(per_timeline_gauge: UIntGauge) -> Self { Self { - last_set: per_timeline_gauge.get(), + last_set: AtomicU64::new(0), gauge: per_timeline_gauge, } } - fn set(&mut self, sz: u64) { + pub(crate) fn set(&self, sz: u64) { self.gauge.set(sz); - if sz < self.last_set { - REMOTE_PHYSICAL_SIZE_GLOBAL.sub(self.last_set - sz); + let prev = self.last_set.swap(sz, std::sync::atomic::Ordering::Relaxed); + if sz < prev { + REMOTE_PHYSICAL_SIZE_GLOBAL.sub(prev - sz); } else { - REMOTE_PHYSICAL_SIZE_GLOBAL.add(sz - self.last_set); + REMOTE_PHYSICAL_SIZE_GLOBAL.add(sz - prev); }; - self.last_set = sz; } pub(crate) fn get(&self) -> u64 { self.gauge.get() @@ -2372,7 +2373,7 @@ impl PerTimelineRemotePhysicalSizeGauge { impl Drop for PerTimelineRemotePhysicalSizeGauge { fn drop(&mut self) { - REMOTE_PHYSICAL_SIZE_GLOBAL.sub(self.last_set); + REMOTE_PHYSICAL_SIZE_GLOBAL.sub(self.last_set.load(std::sync::atomic::Ordering::Relaxed)); } } @@ -2380,7 +2381,7 @@ pub(crate) struct RemoteTimelineClientMetrics { tenant_id: String, shard_id: String, timeline_id: String, - pub(crate) remote_physical_size_gauge: Mutex>, + pub(crate) remote_physical_size_gauge: PerTimelineRemotePhysicalSizeGauge, calls: Mutex>, bytes_started_counter: Mutex>, bytes_finished_counter: Mutex>, @@ -2388,38 +2389,27 @@ pub(crate) struct RemoteTimelineClientMetrics { impl RemoteTimelineClientMetrics { pub fn new(tenant_shard_id: &TenantShardId, timeline_id: &TimelineId) -> Self { + let tenant_id_str = tenant_shard_id.tenant_id.to_string(); + let shard_id_str = format!("{}", tenant_shard_id.shard_slug()); + let timeline_id_str = timeline_id.to_string(); + + let remote_physical_size_gauge = PerTimelineRemotePhysicalSizeGauge::new( + REMOTE_PHYSICAL_SIZE + .get_metric_with_label_values(&[&tenant_id_str, &shard_id_str, &timeline_id_str]) + .unwrap(), + ); + RemoteTimelineClientMetrics { - tenant_id: tenant_shard_id.tenant_id.to_string(), - shard_id: format!("{}", tenant_shard_id.shard_slug()), - timeline_id: timeline_id.to_string(), + tenant_id: tenant_id_str, + shard_id: shard_id_str, + timeline_id: timeline_id_str, calls: Mutex::new(HashMap::default()), bytes_started_counter: Mutex::new(HashMap::default()), bytes_finished_counter: Mutex::new(HashMap::default()), - remote_physical_size_gauge: Mutex::new(None), + remote_physical_size_gauge, } } - pub(crate) fn remote_physical_size_set(&self, sz: u64) { - let mut guard = self.remote_physical_size_gauge.lock().unwrap(); - let gauge = guard.get_or_insert_with(|| { - PerTimelineRemotePhysicalSizeGauge::new( - REMOTE_PHYSICAL_SIZE - .get_metric_with_label_values(&[ - &self.tenant_id, - &self.shard_id, - &self.timeline_id, - ]) - .unwrap(), - ) - }); - gauge.set(sz); - } - - pub(crate) fn remote_physical_size_get(&self) -> u64 { - let guard = self.remote_physical_size_gauge.lock().unwrap(); - guard.as_ref().map(|gauge| gauge.get()).unwrap_or(0) - } - pub fn remote_operation_time( &self, file_kind: &RemoteOpFileKind, diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index ab55c37fe5..340545e621 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -2233,18 +2233,10 @@ impl Tenant { for timeline in timelines { result.resident_size += timeline.metrics.resident_physical_size_gauge.get(); - // FIXME: it's too expensive to take a mutex in this loop result.physical_size += timeline .remote_client .as_ref() - .and_then(|c| { - c.metrics - .remote_physical_size_gauge - .lock() - .unwrap() - .as_ref() - .map(|g| g.get()) - }) + .map(|c| c.metrics.remote_physical_size_gauge.get()) .unwrap_or(0); result.max_logical_size = std::cmp::max( result.max_logical_size, diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index 698e9d1f0e..afd0cc0c34 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -461,11 +461,11 @@ impl RemoteTimelineClient { } else { 0 }; - self.metrics.remote_physical_size_set(size); + self.metrics.remote_physical_size_gauge.set(size); } pub fn get_remote_physical_size(&self) -> u64 { - self.metrics.remote_physical_size_get() + self.metrics.remote_physical_size_gauge.get() } //