Make the dependencies which are not directly used for flamegraph generation optional about inferno HOT 11 CLOSED

I have mixed feeling whenever uninlining should be kept or not in its current state (that would depend on how useful people would find it in practice), however if kept I would definitely at least explicitly and loudly document that it might generate incorrect results in some cases, and that it might not be immediately obvious that the output is incorrect.

from inferno.

@jasonrhansen hmm... Given that it's pretty much useless at the moment, I'm almost tempted to stick it in a branch and leave #31 open (+ maybe expand with the points above). That way we can also remove what is essentially a big chunk of unused code.

from inferno.

I just pushed the tag uninline-support, which I think is all we need!

from inferno.

That's awesome! I think the way to fix this is to add two new features: uninline and cli, both of which are enabled by default (so that cargo install inferno will still work). uninline brings in object, gimli, memmap, and addr2line, whereas cli brings in structopt and env_logger. We'll also need to add required-features = ["cli"] to both [[bin]] entries in Cargo.toml, and make the uninlining code #[cfg(feature = "uninline")].

One question here is how to handle the fact that Options will now have a field that is only sometimes available (i.e., with the "uninline" feature). We could either make it use a builder pattern, or we could add a private field to make it so that people always add ..Default::default(). One day, we'll have #[non_exhaustive], but alas.

@jasonrhansen want to take a stab at this?

from inferno.

Sounds good to me!

One question here is how to handle the fact that Options will now have a field that is only sometimes available

I think the usual pattern when dealing with such cases is to just return an error at runtime if a feature which wasn't enabled at compile time is used. That way we retain source compatibility between crates which use different subsets of features. But making in into a builder would work too.

from inferno.

We could do that (error at runtime), though it makes me a little sad. It also means that the type of the field has to be something not tied to any of the optional crates. In this case that latter requirement is probably fine since I'm guessing it'll just be a bool.

Somewhat separately, I wonder if uninlining shouldn't even be a default feature given #31. As it stands, it's unlikely to even be all that useful! We'd still want it to get tested (so add an entry in .travis.yml for testing with that feature enabled) though.

from inferno.

Somewhat separately, I wonder if uninlining shouldn't even be a default feature given #31. As it stands, it's unlikely to even be all that useful! We'd still want it to get tested (so add an entry in .travis.yml for testing with that feature enabled) though.

Personally I would argue that unless we can guarantee that it can be always done correctly it shouldn't be enabled as default. There's the issue of ASLR, but also there are other subtle traps here which complicate matters, like for example: (My profiler tries to handle all of these; I'm not sure if it would be even possible to handle these based only on the output of perf script.)

• The profiling might have been done in a container, so the paths of the modules might need to be translated.
• The profiling might have been done on another machine, so the binaries with the debug info would have to be verified that they're the same as those used during the profiling. (Otherwise you'd get seemingly fine looking but totally incorrect symbols.)
• The profiling might have been done on another machine on another CPU architecture.
• Often the debug info is kept in a separate file (e.g. libfoo.so will have libfoo.so.debug); you have to load libfoo.so, check if you've loaded the correct libfoo.so, look at its .gnu_debuglink section to see what .debug file you should load, load that, and verify that its build ID actually matches, and then use it.
• Just looking at the memory maps is not enough to defeat the ASLR in every case AFAIK; you need access to the original binary and grab its PT_LOAD program headers to see where everything was supposed to be mapped, match it to where it was actually mapped, and do some arithmetic to translate the addresses from what you had at runtime into what is in the symbol table.

It's tricky to handle all of this, and it's a lot of extra complexity.

from inferno.

I completely agree that un-inlining shouldn't be a default feature. I'll go ahead and add the uninline and cli features, but only enable cli by default.

from inferno.

Mmm, yeah, you're not wrong... @jasonrhansen I have half a mind to just cut support for un-inlining given @koute's comments above and the severity of #31. Thoughts?

from inferno.

I'm not opposed to removing un-inlining support. It's not really that useful in its current state. But if we documented the issues, as @koute suggested, we could keep it behind a non-default feature and it shouldn't be a problem. Sometime down the road we could try to solve some of the issues, but I don't think it's a high priority.

I'm leaning toward keeping it and documenting the issues, but it's your call @jonhoo.

from inferno.

@jonhoo Works for me! I'll just remove all of the un-inlining code, but still add the cli feature. Do you want to push a branch with the un-inline stuff?

from inferno.