metrics: add MetricAtomicU64 and use in metrics #6574

rcoh · 2024-05-20T20:27:12Z

Motivation

As we move to stabilize metrics, we need a graceful way to support U64 on platforms without 64-bit atomics.

Note: this is a breaking change on platforms without 32-bit atomics. Previously, they would have used a lock fallback provided by loom. Now, they simply do not have read access to these metrics.

Solution

This commit adds MetricAtomicU64 which is empty on platforms that do not support 64-bit atomics. Write operations are no-ops and read operations are not defined. This also allows us to remove the loom gating on cfg_metrics. This will allow #6556 to avoid the need for a separate cfg_metrics gate.

Darksonn · 2024-05-21T06:03:15Z

Is this intended to be merged before or after #6556?

rcoh · 2024-05-21T11:57:06Z

Before — can't build the other cr without it

…

On Tue, May 21, 2024, 2:03 AM Alice Ryhl ***@***.***> wrote: Is this intended to be merged before or after #6556 <#6556>? — Reply to this email directly, view it on GitHub <#6574 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADYKZ34ZQ4DVZOPIIGBWILZDLPTRAVCNFSM6AAAAABIAKT7ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRRHAYTEOJXGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Darksonn

Overall lgtm.

Darksonn · 2024-05-22T10:31:47Z

spellcheck.dic

Please also update the counter on the first line.

Darksonn · 2024-05-22T10:32:08Z

tokio/src/macros/cfg.rs

+macro_rules! cfg_64bit_metrics {
+    ($($item:item)*) => {
+        $(
+            #[cfg(all(target_has_atomic = "64"))]
+            #[cfg_attr(docsrs, doc(cfg(target_has_atomic = "64")))]
+            $item
+        )*
+    }
+
+}
+
+macro_rules! cfg_no_64bit_metrics {
+    ($($item:item)*) => {
+        $(
+            // For now, metrics is only disabled in loom tests.
+            // When stabilized, it might have a dedicated feature flag.
+            #[cfg(not(target_has_atomic = "64"))]
+            $item
+        )*
+    }
+
+}


There are some empty lines here (line 238 and 250).

Darksonn · 2024-05-22T10:32:43Z

tokio/src/macros/cfg.rs

+macro_rules! cfg_no_64bit_metrics {
+    ($($item:item)*) => {
+        $(
+            // For now, metrics is only disabled in loom tests.
+            // When stabilized, it might have a dedicated feature flag.


This comment seems out of date.

Darksonn · 2024-05-22T10:33:48Z

tokio/src/runtime/metrics/runtime.rs

+    cfg_64bit_metrics! {

-    /// Returns the number of times that tasks have been forced to yield back to the scheduler
-    /// after exhausting their task budgets.
-    ///
-    /// This count starts at zero when the runtime is created and increases by one each time a task yields due to exhausting its budget.
-    ///
-    /// The counter is monotonically increasing. It is never decremented or
-    /// reset to zero.
-    pub fn budget_forced_yield_count(&self) -> u64 {
-        self.handle
-            .inner
-            .scheduler_metrics()
-            .budget_forced_yield_count
-            .load(Relaxed)
-    }



Extra empty lines.

Darksonn · 2024-05-22T10:34:20Z

tokio/src/runtime/metrics/scheduler.rs

@@ -1,4 +1,4 @@
-use crate::loom::sync::atomic::{AtomicU64, Ordering::Relaxed};
+use crate::{loom::sync::atomic::Ordering::Relaxed, util::metric_atomics::MetricAtomicU64};


Suggested change

use crate::{loom::sync::atomic::Ordering::Relaxed, util::metric_atomics::MetricAtomicU64};

use crate::loom::sync::atomic::Ordering::Relaxed;

use crate::util::metric_atomics::MetricAtomicU64;

Darksonn · 2024-05-22T11:36:48Z

tokio/src/util/metric_atomics.rs

+        // on platforms without 64-bit atomics, fetch-add returns unit
+        pub(crate) fn fetch_add(&self, _value: u64, _ordering: Ordering) {  }


Then I would probably just return unit from both. (And perhaps rename to add.)

mox692 · 2024-05-22T15:36:38Z

tokio/src/macros/cfg.rs

            // For now, metrics is only disabled in loom tests.
            // When stabilized, it might have a dedicated feature flag.
-            #[cfg(all(tokio_unstable, not(loom)))]
+            #[cfg(tokio_unstable)]


Perhaps this comment is also out of date?

mox692 · 2024-05-22T15:37:21Z

tokio/src/macros/cfg.rs

+macro_rules! cfg_64bit_metrics {
+    ($($item:item)*) => {
+        $(
+            #[cfg(all(target_has_atomic = "64"))]


I think we could simplify this condition:

Suggested change

#[cfg(all(target_has_atomic = "64"))]

#[cfg(target_has_atomic = "64")]

Darksonn

Looks good to me.

As we move to stabilize metrics, we need a graceful way to support U64 on platforms without 64-bit atomics. This commit adds MetricAtomicU64 which is empty on platforms that do not support 64-bit atomics. Write operations are no-ops and read operations are not defined.

Darksonn added A-tokio Area: The main tokio crate M-metrics Module: tokio/runtime/metrics labels May 21, 2024

Darksonn reviewed May 22, 2024

View reviewed changes

rcoh force-pushed the metrics-on-loom branch from 2569bca to 50b810a Compare May 22, 2024 14:57

rcoh requested a review from Darksonn May 22, 2024 14:57

mox692 reviewed May 22, 2024

View reviewed changes

Darksonn approved these changes May 22, 2024

View reviewed changes

mox692 approved these changes May 23, 2024

View reviewed changes

rcoh added 4 commits May 23, 2024 10:23

Fix up a number of tests & spellcheck

e7be0aa

Feedback cleanups

969c446

cleanup comments / cfg gates

3fb35a0

rcoh force-pushed the metrics-on-loom branch from 60b9db3 to 3fb35a0 Compare May 23, 2024 14:23

Darksonn merged commit cba86cf into tokio-rs:master May 23, 2024
77 checks passed

rcoh mentioned this pull request May 28, 2024

metrics: stabilize RuntimeMetrics::worker_count #6556

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metrics: add MetricAtomicU64 and use in metrics #6574

metrics: add MetricAtomicU64 and use in metrics #6574

rcoh commented May 20, 2024 •

edited

Loading

Darksonn commented May 21, 2024

rcoh commented May 21, 2024 via email

Darksonn left a comment

Darksonn May 22, 2024

Darksonn May 22, 2024

Darksonn May 22, 2024

Darksonn May 22, 2024

Darksonn May 22, 2024

Darksonn May 22, 2024

mox692 May 22, 2024

mox692 May 22, 2024

Darksonn left a comment

		@@ -1,4 +1,4 @@
		use crate::loom::sync::atomic::{AtomicU64, Ordering::Relaxed};
		use crate::{loom::sync::atomic::Ordering::Relaxed, util::metric_atomics::MetricAtomicU64};

	use crate::{loom::sync::atomic::Ordering::Relaxed, util::metric_atomics::MetricAtomicU64};
	use crate::loom::sync::atomic::Ordering::Relaxed;
	use crate::util::metric_atomics::MetricAtomicU64;

		// on platforms without 64-bit atomics, fetch-add returns unit
		pub(crate) fn fetch_add(&self, _value: u64, _ordering: Ordering) { }

	#[cfg(all(target_has_atomic = "64"))]
	#[cfg(target_has_atomic = "64")]

metrics: add MetricAtomicU64 and use in metrics #6574

metrics: add MetricAtomicU64 and use in metrics #6574

Conversation

rcoh commented May 20, 2024 • edited Loading

Motivation

Solution

Darksonn commented May 21, 2024

rcoh commented May 21, 2024 via email

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

rcoh commented May 20, 2024 •

edited

Loading