Skip to content

Conversation

jalil-salame
Copy link

Previously we stored an Arc<Box<dyn Fn()>>, but we can directly store the function in the Arc.

We could also take an impl Fn() -> f64 + Send + Sync instead of a Box<dyn Fn() -> f64 + Send + Sync> and do the allocation inside new:

impl PullingGauge {
    pub fn new<S1: Into<String>, S2: Into<String>>(
        name: S1,
        help: S2,
        value: impl Fn() -> f64 + Send + Sync,
    ) -> crate::Result<Self> {
        Ok(PullingGauge {
            value: Arc::new(value),
            desc: crate::core::Desc::new(name.into(), help.into(), Vec::new(), HashMap::new())?,
        })
    }
}

This is more efficient and lets you write the closure directly on the calling site:

// Previously
let pulling_gauge = PullingGauge::new("foo", "bar", Box::new(|| static_vec.len() as f64));
// Now
let pulling_gauge = PullingGauge::new("foo", "bar", || static_vec.len() as f64);

This wouldn't be a breaking change since Box<dyn Fn()> implements Fn(), but it would make it less efficient since you are now storing a Box inside the Arc.

Copy link

ti-chi-bot bot commented Aug 25, 2025

Welcome @jalil-salame! It looks like this is your first PR to tikv/rust-prometheus 🎉

Previously we stored an `Arc<Box<dyn Fn()>>`, but we can directly store
the function in the `Arc`.

We could also take an `impl Fn() -> f64 + Send + Sync` instead of a
`Box<dyn Fn() -> f64 + Send + Sync>` and do the allocation inside `new`:

```rust
impl PullingGauge {
    pub fn new<S1: Into<String>, S2: Into<String>>(
        name: S1,
        help: S2,
        value: impl Fn() -> f64 + Send + Sync,
    ) -> crate::Result<Self> {
        Ok(PullingGauge {
            value: Arc::new(value),
            desc: crate::core::Desc::new(name.into(), help.into(), Vec::new(), HashMap::new())?,
        })
    }
}
```

This is more efficient and lets you write the closure directly on the
calling site:

```rust
// Previously
let pulling_gauge = PullingGauge::new("foo", "bar", Box::new(|| static_vec.len() as f64));
// Now
let pulling_gauge = PullingGauge::new("foo", "bar", || static_vec.len() as f64);
```

This wouldn't be a breaking change since `Box<dyn Fn()>` implements `Fn()`,
but it would make it less efficient since you are now storing a `Box`
inside the `Arc`.

Signed-off-by: Jalil David Salamé Messina <[email protected]>
@jalil-salame
Copy link
Author

cargo is picking incompatible dependencies for 1.81 which is why the tests are failing T-T

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

Successfully merging this pull request may close these issues.

1 participant