jonhoo / inferno

A Rust port of FlameGraph
Other
1.64k stars 117 forks source link

Differential output only calculates diff correctly for leaves (most specific frames) #295

Open itamarst opened 1 year ago

itamarst commented 1 year ago

Consider a diff of these two (cargo run --bin inferno-diff-folded old1.prof new2.prof | cargo run --bin inferno-flamegraph -- - > out.svg):

parent;first_child 10
parent;second_child 10
parent2 30

and

parent;first_child 30
parent;second_child 30
parent2 30

The resulting flamegraph shows parent as unchanged, even though it's actually gotten larger: Screenshot from 2023-05-31 14-45-11

jonhoo commented 1 year ago

That's... bizarre. Looking at the code some more, I think this isn't the rendering but the actual diff collapse computation. Specifically, I wonder if it's related to this comment: https://github.com/jonhoo/inferno/blob/a497e3eab4d1586b2802b1807ba00cbac802dbc7/src/flamegraph/merge.rs#L92-L94

jasonrhansen commented 1 year ago

FWIW the current implementation matches the output of FlameGraph, and from what I can tell it's the intended behavior. See the part boxed in red below from differential-flame-graphs.

image

jonhoo commented 1 year ago

I'm torn here because the current output is also pretty hard to inuit the meaning of. Maybe the trick here is going to be mainly around how we present the information in the hover popup on frames that have children. I feel like with some labels for which thing got how much slower, it might actually help a lot. I take Brendan's point that this choice is probably the one that gives more useful data, even if it is not the most intuitive one.

itamarst commented 11 months ago

I am not sure I agree with the argument, it's useful to see which general areas had differences. On the other hand I'm wondering if differences in general are even useful for my case, since I'm including line numbers and that means minor changes to code can make a whole file non-comparable. So I'm not going to push on this thread until I get back to thinking about that...