llogiq / flame

An intrusive flamegraph profiling tool for rust.
Apache License 2.0
694 stars 30 forks source link

Add `must_use` attribute to `SpanGuard` #49

Closed Stef-Gijsberts closed 4 years ago

Stef-Gijsberts commented 4 years ago

Motivation

I wanted to profile some library and gave flame a try. It is very easy to get started with the library, which is great!

However, I noticed that some events were not recorded, although I did create a guarded event for it. One of the cases which would not be recorded looks similar to the following:

fn foo() {
    flame::start_guard("Foo");
    perform_cpu_heavy_calculations();
}

As this function clearly performs cpu heavy calculations, I expected it to be recorded, but it was not.

Dropping too soon

I quickly realized my mistake. The guard created by start_guard was instantiated and immediately dropped, because there is no assignment. An example of this phenomenon (link to playground):

struct Guard;

fn create_guard() -> Guard {
    println!("Creating guard");
    Guard
}

impl Drop for Guard {
    fn drop(&mut self) {
        println!("Dropping guard");
    }
}

fn foo() {
    create_guard();
    println!("Performing a CPU intensive task");
    println!("Finished!")
}

fn main() {
    foo();
}

// Output:
// Creating guard
// Dropping guard
// Performing a CPU intensive task
// Finished!

Note that the guard is dropped before the 'CPU intensive task' is performed. This causes the execution time of that task to not be included in profiling result, while the user very probably expected it to be.

To make sure the 'CPU intensive task' is measured as well, the guard can be assigned to a variable (let-binding), like this (link to playground):

// Duplicate code omitted

fn foo() {
    let _guard = create_guard();
    println!("Performing a CPU intensive task");
    println!("Finished!")
}

// Output:
// Creating guard
// Performing a CPU intensive task
// Finished!
// Dropping guard

Binding a variable tells the compiler to drop it at the end of the scope (in this case the function).

Suggested Change

My issue could have been avoided if the #[must_use] attribute would have been used on SpanGuard. To demonstrate this, let's take the first example, but with the must_use attribute:

#[must_use]
struct Guard;

fn create_guard() -> Guard {
    println!("Creating guard");
    Guard
}

impl Drop for Guard {
    fn drop(&mut self) {
        println!("Dropping guard");
    }
}

fn foo() {
    create_guard();
    println!("Performing a CPU intensive task");
    println!("Finished!")
}

fn main() {
    foo();
}

This code will compile, but it will show the following warning while doing so:

warning: unused `Guard` that must be used
  --> src/main.rs:16:5
   |
16 |     create_guard();
   |     ^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_must_use)]` on by default

If I would have seen this warning, I would have immediately fixed my mistake.

We can even show a nice message along with the warning:

#[must_use = "The guard is immediately dropped after instantiation. This is probably not
what you want! Consider using a `let` binding to increase its lifetime."]

Resulting in the following error:

warning: unused `Guard` that must be used
  --> src/main.rs:17:5
   |
17 |     create_guard();
   |     ^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_must_use)]` on by default
   = note: The guard is immediately dropped after instantiation. This is probably not
           what you want. Consider using a `let` binding to increase its lifetime.

Refer to the diff of the commit to see what I'm specifically suggesting.

TyOverby commented 4 years ago

This looks great; thanks!