Incompatibility with functions create more `&mut self` .

[SimonImbrogno]

Just to confirm, If I have a function that ever takes a &mut self.foo or calls a method with a receiver self.bar(&mut self), then everything just breaks?

It complains about reused borrows. Seems really limiting that I can only use the proc macros to measure things in functions that take self but then never call out to anything taking &mut self, etc..

:( the only other alternative being to use the measure! macro around specific blocks, and then creating the Metric container manually I guess? But that still doesn't work if my section under test calls another function with a mutable self?

[magnet]

Rust should be able to understand non-overlapping borrows. I need to test but I believe &mut self.foo should work because it does not overlap with self.metrics. If it doesn't, I believe it could be fixed.

However self.bar() would break if bar takes &mut self because &self.metrics implies a shared borrow on self.

The latter is a tricky problem to solve if we keep the metrics registry in self. A workaround I could see is wrapping your struct in another one (where there metrics registry is) and calling self.wrapped.bar(), which would only exclusively borrow self.wrapped, which corresponds to the first case of borrowing independent fields in self.

[SimonImbrogno]

Hmm yeah, I could try to play around with it a bit to isolate it, but it seemed to have issue with just taking a mutable reference to other fields.

It's a little unfortunate, I have a already implemented a fairly complex system, and am trying to add some metering to specific critical sections, so I'd prefer not to rewrite/wrap everything in order just to appease the borrow checker. :(

A question: Why does metered need to hold onto a &self.metrics? I'm not sure of the implementation details, but I'm assuming it's either storing that reference in a struct or lambda in order to do some updates after the function return statement? (via Drop)?

[magnet]

A question: Why does metered need to hold onto a &self.metrics? I'm not sure of the implementation details, but I'm assuming it's either storing that reference in a struct or lambda in order to do some updates after the function return statement? (via Drop)?

No it's not doing anything with drop or custom structs, it's really just wrapping the calls with callbacks (e.g before/after the metric) and needs to borrow the metric for the duration of the call to do so. I explored several designs before and this was at the time the most convincing one to keep the generated code reasonably vanilla Rust, not store mutable state in a static global, and be compatible with blocking and async code.

Documentation should contain an example of the pseudo-code that metered generates (using the measure! macros).

Hmm yeah, I could try to play around with it a bit to isolate it, but it seemed to have issue with just taking a mutable reference to other fields.

If you can reduce a test case to demonstrate this, that should be something we can fix in metered.

[SimonImbrogno]

I may have been mistaken with the non-overlapping borrows. I think I forgot to comment out a lambda that was causing the issue. 🤦‍♂

So I've been looking through the source and I feel like the issue with mut receivers can be fixed relatively easily:

measure!(metric_ref, { 
    /* something */ 
})

expands to:

{
    let _metric = metric_ref ;
    let _enter = metered::Enter::enter(_metric);
    let _result = {
        /* something */
    };
    metered::metric::on_result(_metric, _enter, &_result);
    _result
}

It just looks like the only issue is that the calls to Enter::enter and metric::on_result both use a stored reference that lives across the scope of the result block.

If the measure macro is changed to accept an expression (and the metered macros are updated accordingly):

macro_rules! measure {
    ($metric:expr, $e:expr) => {{
        let _enter = $crate::Enter::enter($metric);
        let _result = $e;
        $crate::metric::on_result($metric, _enter, &_result);
        _result
    }};
}

then:

measure!(&metric, { 
    /* something */ 
})

expands to:

{
    let _enter = metered::Enter::enter(&metric);
    let _result = {
        /* something */
    };
    metered::metric::on_result(&metric, _enter, &_result);
    _result
}

And you don't have to worry about conflicting borrows across the result block. 🎉

It would be a breaking change for anyone using measure! though 😟 If breaking changes were to be avoided at all costs, you could shim the old, ident matching, version of the macro in for backwards compatibility.

I also noticed that at metered.rs:160 there's a comment about binding the references to variables to avoid "moving issues" but I'm not sure where that would be a problem.. 🤷‍♂ I tested this out on a fork, and all the demos seem to run without issue. Would you be open to that kind of update?

[magnet]

I think your solution was the original code and aliasing the metric a workaround at some point, but I don't remember the details.

I would be open to test your change on my projects / test cases and if it works then merge it.

[SimonImbrogno]

Ok, I've opened a PR so you can give it a try: https://github.com/magnet/metered-rs/pull/14.

I would be curious to see a case where it doesn't work and, if so, how it can worked around.

[magnet]

So far the approach suggested uses unsafe but I think we should be able to find an alternative, maybe with a different layout, that does not and avoids risks of UB (since we're wrapping client code in the metrics, it's hard to guaranteed any invariants)