Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use std sync primitives instead of parking_lot #343

Closed
BratSinot opened this issue Feb 21, 2023 · 1 comment · Fixed by #344
Closed

Use std sync primitives instead of parking_lot #343

BratSinot opened this issue Feb 21, 2023 · 1 comment · Fixed by #344

Comments

@BratSinot
Copy link
Contributor

Greetings!

Since 1.62 rust start to use sync primitives based on futex (unstead of pthreads) in */Linux systems and it become equal (or faster in some cases) by speed with parking_lot.

We can save "poison" behaviour from parking_lot by this:

    use std::sync::{PoisonError, RwLockReadGuard, RwLockWriteGuard};

    pub(crate) struct RwLock<T>(std::sync::RwLock<T>);

    impl<T> RwLock<T> {
        #[inline]
        pub(crate) fn new(t: T) -> Self {
            Self(std::sync::RwLock::new(t))
        }

        #[inline]
        pub(crate) fn read(&self) -> RwLockReadGuard<T> {
            self.0.read().unwrap_or_else(PoisonError::into_inner)
        }

        #[inline]
        pub(crate) fn write(&self) -> RwLockWriteGuard<T> {
            self.0.write().unwrap_or_else(PoisonError::into_inner)
        }
    }

But the problem is that minimal rust version in yours crates is 1.56.1 and if someone use older compiler version speed will degradate.
In some crates I can add parking_lot as optional:

diff --git a/metrics-exporter-prometheus/Cargo.toml b/metrics-exporter-prometheus/Cargo.toml
index e24aaf2..e6160ea 100644
--- a/metrics-exporter-prometheus/Cargo.toml
+++ b/metrics-exporter-prometheus/Cargo.toml
@@ -17,7 +17,7 @@ categories = ["development-tools::debugging"]
 keywords = ["metrics", "telemetry", "prometheus"]
 
 [features]
-default = ["http-listener", "push-gateway"]
+default = ["http-listener", "push-gateway", "parking_lot"]
 async-runtime = ["tokio", "hyper"]
 http-listener = ["async-runtime", "hyper/server", "ipnet"]
 push-gateway = ["async-runtime", "hyper/client", "tracing"]
@@ -25,7 +25,7 @@ push-gateway = ["async-runtime", "hyper/client", "tracing"]
 [dependencies]
 metrics = { version = "^0.20", path = "../metrics" }
 metrics-util = { version = "^0.14", path = "../metrics-util", default-features = false, features = ["recency", "registry", "summary"] }
-parking_lot = { version = "0.12", default-features = false }
+parking_lot = { version = "0.12", optional = true, default-features = false }
 thiserror = { version = "1", default-features = false }
 quanta = { version = "0.10.0", default-features = false }
 indexmap = { version = "1", default-features = false }
diff --git a/metrics-exporter-prometheus/src/builder.rs b/metrics-exporter-prometheus/src/builder.rs
index 2e5bd6c..2db688a 100644
--- a/metrics-exporter-prometheus/src/builder.rs
+++ b/metrics-exporter-prometheus/src/builder.rs
@@ -31,7 +31,6 @@ use hyper::{
 use indexmap::IndexMap;
 #[cfg(feature = "http-listener")]
 use ipnet::IpNet;
-use parking_lot::RwLock;
 use quanta::Clock;
 #[cfg(any(feature = "http-listener", feature = "push-gateway"))]
 use tokio::runtime;
@@ -48,6 +47,7 @@ use crate::common::Matcher;
 use crate::distribution::DistributionBuilder;
 use crate::recorder::{Inner, PrometheusRecorder};
 use crate::registry::AtomicStorage;
+use crate::sync::RwLock;
 use crate::{common::BuildError, PrometheusHandle};
 
 #[cfg(any(feature = "http-listener", feature = "push-gateway"))]
@@ -480,7 +480,7 @@ impl PrometheusBuilder {
                                     let body = body
                                         .map_err(|_| ())
                                         .map(|mut b| b.copy_to_bytes(b.remaining()))
-                                        .map(|b| (&b[..]).to_vec())
+                                        .map(|b| (b[..]).to_vec())
                                         .and_then(|s| String::from_utf8(s).map_err(|_| ()))
                                         .unwrap_or_else(|_| {
                                             String::from("<failed to read response body>")
diff --git a/metrics-exporter-prometheus/src/lib.rs b/metrics-exporter-prometheus/src/lib.rs
index 399d942..e19dcd9 100644
--- a/metrics-exporter-prometheus/src/lib.rs
+++ b/metrics-exporter-prometheus/src/lib.rs
@@ -113,5 +113,6 @@ pub mod formatting;
 mod recorder;
 
 mod registry;
+mod sync;
 
 pub use self::recorder::{PrometheusHandle, PrometheusRecorder};
diff --git a/metrics-exporter-prometheus/src/recorder.rs b/metrics-exporter-prometheus/src/recorder.rs
index 6d746df..223969b 100644
--- a/metrics-exporter-prometheus/src/recorder.rs
+++ b/metrics-exporter-prometheus/src/recorder.rs
@@ -5,7 +5,6 @@ use std::sync::Arc;
 use indexmap::IndexMap;
 use metrics::{Counter, Gauge, Histogram, Key, KeyName, Recorder, SharedString, Unit};
 use metrics_util::registry::{Recency, Registry};
-use parking_lot::RwLock;
 use quanta::Instant;
 
 use crate::common::Snapshot;
@@ -14,6 +13,7 @@ use crate::formatting::{
     key_to_parts, sanitize_metric_name, write_help_line, write_metric_line, write_type_line,
 };
 use crate::registry::GenerationalAtomicStorage;
+use crate::sync::RwLock;
 
 pub(crate) struct Inner {
     pub registry: Registry<Key, GenerationalAtomicStorage>,
diff --git a/metrics-exporter-prometheus/src/sync.rs b/metrics-exporter-prometheus/src/sync.rs
index e69de29..dfb3b8d 100644
--- a/metrics-exporter-prometheus/src/sync.rs
+++ b/metrics-exporter-prometheus/src/sync.rs
@@ -0,0 +1,29 @@
+#[cfg(feature = "parking_lot")]
+pub(crate) use parking_lot::RwLock;
+
+#[cfg(not(feature = "parking_lot"))]
+mod std_mutex {
+    use std::sync::{PoisonError, RwLockReadGuard, RwLockWriteGuard};
+
+    pub(crate) struct RwLock<T>(std::sync::RwLock<T>);
+
+    impl<T> RwLock<T> {
+        #[inline]
+        pub(crate) fn new(t: T) -> Self {
+            Self(std::sync::RwLock::new(t))
+        }
+
+        #[inline]
+        pub(crate) fn read(&self) -> RwLockReadGuard<T> {
+            self.0.read().unwrap_or_else(PoisonError::into_inner)
+        }
+
+        #[inline]
+        pub(crate) fn write(&self) -> RwLockWriteGuard<T> {
+            self.0.write().unwrap_or_else(PoisonError::into_inner)
+        }
+    }
+}
+
+#[cfg(not(feature = "parking_lot"))]
+pub(crate) use std_mutex::*;

But in some crates it already optional hidden by different flag, like in metrics-util:

recency = ["parking_lot", "registry", "quanta"]

and I can't make usage of std sync primitive by flag and save old behaviour.

I see only 3 ways:

  1. remove parking_lot deps;
  2. use std sync primitives by default and make usage of parking_lot optional;
  3. doesn't bore at all and continue to use parking_lot;

But I want to remove extra deps and option 3 isn't acceptable personally for me =)

Any thoughts on this case?

@tobz
Copy link
Member

tobz commented Feb 26, 2023

@BratSinot I'd be OK with removing the parking_lot dependency entirely.

I mostly only ever used it because I disliked the additional result handling introduced by handling poisoned lock errors... but it's probably worth just adding a wrapper like you suggested (or something else) and using the stdlib version to reduce the number of dependencies, especially given the idea of pushing towards 1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants