Easy profiling disabling

mfr-itr commented 6 years ago

Since your lib requires modifications to the source code, it's not easy to disable it for a true release. I see two ways to fix this:

Add a compile time feature which prevents data collection. I'm thinking global const with a compile time value, and a if !STATIC_ENABLED { return } in every function. Since the condition is known at compile time, it should be inlined and removed by rustc.
Add a runtime switch, like flame::disable() for example. It could set an AtomicBool. It would be a bit slower but would allow the activation without recompiling. Now that I thing of it, it would be a very nice feature to have!

These two options could be implemented at the same time, like if !STATIC_ENABLED || !DYNAMIC_ENABLED ....

TyOverby commented 6 years ago

I've had a few thoughts on this, and I think what I'm going to do is a combination of runtime and compile-time checks. Let me know what you think

Runtime Checks

I really like the idea of having a global (Atomic?) bool that turns on/off collection events. This would be very similar to the rust log crate where the application (but not any libraries) are in charge of enabling and disabling logging.

I don't know if there's any benefit to having the global be atomic. I don't really care if multiple threads clobber the value. There's no real reason to "synchronize" reading and writing to this boolean.

Compiletime Check

If you really don't want flame present, you can turn on the "disable-in-release" cargo feature when your application depends on 'flame'.

This "feature" will turn every flame function into an empty function with #[inline(always)] thereby making every call into flame (even from other libraries that depend on flame) a no-op.

TyOverby commented 6 years ago

Also, despite what it looks like, I'm actually working on Flame pretty seriously right now, it's just that all my effort has gone into making a new visualizer, haha

So yeah, if you have any other ideas on how to make Flame more ergonomic, let me know! I'm aiming for a 1.0 release early January

Yamakaky commented 6 years ago

I guess you would have to use an atomic if you want to read the value from multiple threads?

TyOverby commented 6 years ago

You can read from multiple threads without atomics. I think you need UnsafeCell, but proving the safety of UnsafeCell is pretty simple.

mfr-itr commented 6 years ago

Yeah, but it must be written at runtime. Better safe than sorry! Also, we must into account that the function can be called multiple times or after some data was collected. For the first part, just panicking if flame is already disabled should do the job.

parasyte commented 6 years ago

@TyOverby Where/how is the "disable-in-release" cargo feature implemented? I've been using my own feature to annotate all of my flame calls so it can be controlled in my crate. It adds a lot of noise, so I like the sound of your alternative better. I just don't know of any way to use it.

TyOverby commented 6 years ago

@parasyte I have a branch implementing this here: https://github.com/TyOverby/flame/pull/36/files

Unfortunately rust-lang-nursery/rust-semverver doesn't compile on Windows so I can't validate API compatibility. Would it be possible for you to run semver checks on this PR for me?

parasyte commented 6 years ago

@TyOverby I'm on macOS/Linux, primarily. Haven't tried Rust on Windows yet. But yeah, I'll check out this branch. Thanks!

parasyte commented 6 years ago

@TyOverby cargo +nightly semver reports the following errors (on master, and disable-feature):

       Fresh itoa v0.4.1
       Fresh dtoa v0.4.2
       Fresh serde v1.0.37
       Fresh num-traits v0.2.2
       Fresh unicode-xid v0.1.0
       Fresh libc v0.2.40
       Fresh lazy_static v1.0.0
       Fresh serde_json v1.0.13
       Fresh proc-macro2 v0.3.6
       Fresh thread-id v3.3.0
       Fresh quote v0.5.1
       Fresh syn v0.13.1
       Fresh serde_derive_internals v0.23.0
       Fresh serde_derive v1.0.37
       Fresh flame v0.2.1-pre (file:///Users/parasyte/other-projects/flame)
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
    Updating registry `https://github.com/rust-lang/crates.io-index`
       Fresh unicode-xid v0.1.0
       Fresh lazy_static v0.2.11
       Fresh dtoa v0.4.2
       Fresh itoa v0.4.1
       Fresh serde v1.0.37
       Fresh num-traits v0.2.2
       Fresh libc v0.2.40
       Fresh proc-macro2 v0.3.6
       Fresh serde_json v1.0.13
       Fresh thread-id v3.3.0
       Fresh quote v0.5.1
       Fresh syn v0.13.1
       Fresh serde_derive_internals v0.23.0
       Fresh serde_derive v1.0.37
       Fresh flame v0.2.2
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
version bump: 0.2.2 -> (breaking) -> 0.2.3
warning: technically breaking changes in `new::_IMPL_SERIALIZE_FOR_Span::<impl serde::ser::Serialize for new::Span>`
   --> /Users/parasyte/other-projects/flame/src/lib.rs:106:37
    |
106 | #[cfg_attr(feature = "json", derive(Serialize))]
    |                                     ^^^^^^^^^
    |
    = note: trait impl generalized or newly added (technically breaking)

warning: technically breaking changes in `new::_IMPL_SERIALIZE_FOR_Note::<impl serde::ser::Serialize for new::Note>`
   --> /Users/parasyte/other-projects/flame/src/lib.rs:130:37
    |
130 | #[cfg_attr(feature = "json", derive(Serialize))]
    |                                     ^^^^^^^^^
    |
    = note: trait impl generalized or newly added (technically breaking)

warning: technically breaking changes in `new::_IMPL_SERIALIZE_FOR_Thread::<impl serde::ser::Serialize for new::Thread>`
   --> /Users/parasyte/other-projects/flame/src/lib.rs:144:37
    |
144 | #[cfg_attr(feature = "json", derive(Serialize))]
    |                                     ^^^^^^^^^
    |
    = note: trait impl generalized or newly added (technically breaking)

warning: path changes to `dump_html_custom`
  --> /Users/parasyte/other-projects/flame/src/html.rs:5:1
   |
5  | / pub fn dump_html_custom<W: Write>(mut out: W, spans: &[Span]) -> IoResult<()> {
6  | |     fn dump_spans<W: Write>(out: &mut W, span: &Span) -> IoResult<()> {
7  | |         try!(writeln!(out, "{{"));
8  | |         try!(writeln!(out, r#"name: "{}","#, span.name));
...  |
72 | |     Ok(())
73 | | }
   | |_^
   |
note: added path (technically breaking)
  --> /Users/parasyte/other-projects/flame/src/lib.rs:557:27
   |
557| pub use html::{dump_html, dump_html_custom};
   |                           ^^^^^^^^^^^^^^^^

warning: technically breaking changes in `<new::ALL_THREADS as lazy_static::LazyStatic>`
   |
   = note: trait impl generalized or newly added (technically breaking)

error: breaking changes in `old::_IMPL_SERIALIZE_FOR_Span::<impl serde::ser::Serialize for old::Span>`
   --> /Users/parasyte/.cargo/registry/src/github.com-1ecc6299db9ec823/flame-0.2.2/src/lib.rs:104:37
    |
104 | #[cfg_attr(feature = "json", derive(Serialize))]
    |                                     ^^^^^^^^^
    |
    = warning: trait impl specialized or removed (breaking)

error: breaking changes in `old::_IMPL_SERIALIZE_FOR_Note::<impl serde::ser::Serialize for old::Note>`
   --> /Users/parasyte/.cargo/registry/src/github.com-1ecc6299db9ec823/flame-0.2.2/src/lib.rs:128:37
    |
128 | #[cfg_attr(feature = "json", derive(Serialize))]
    |                                     ^^^^^^^^^
    |
    = warning: trait impl specialized or removed (breaking)

error: breaking changes in `old::_IMPL_SERIALIZE_FOR_Thread::<impl serde::ser::Serialize for old::Thread>`
   --> /Users/parasyte/.cargo/registry/src/github.com-1ecc6299db9ec823/flame-0.2.2/src/lib.rs:142:37
    |
142 | #[cfg_attr(feature = "json", derive(Serialize))]
    |                                     ^^^^^^^^^
    |
    = warning: trait impl specialized or removed (breaking)

error: breaking changes in `<old::ALL_THREADS as lazy_static::LazyStatic>`
   |
   = warning: trait impl specialized or removed (breaking)

error: aborting due to 4 previous errors

The lazy_static changes make sense; that dependency was updated. I am really confused about the errors with serde, though ...

TyOverby commented 6 years ago

Sorry for how long it's taken. I just published v0.2.1-pre2 and I'm going to see about publishing a new minor version with these changes after I figure out how to run semverver on different releases :D

zicklag commented 5 years ago

Just wanted to note here that using Flamer does make it a bit easier to automatically disable Flame based on a feature flag because you can use the #[cfg_attr(feature = "flame_it", flame)] syntax.

It would still be great to be able to disable the other Flame functions at compile time with a feature flag, like you described above, though. :+1: :slightly_smiling_face:

llogiq / flame

Easy profiling disabling #31

Runtime Checks

Compiletime Check