Should not collapse adjacent equivalently-named stacks in flame chart mode

[Mark-Simulacrum]

For example, with the input of

a;b 1
a;b 1
a 0

I would expect 3 nodes in the output:

• a with width 2
• b with width 1
• b with width 1

Currently, though, inferno combines the two bs into one. I wasn't able to quickly follow the merging code so not sure exactly what is causing this, but seems like a bug.

[jonhoo]

Yeah, I agree this seems like a bug in flame chart mode. The merging code is not particularly readable, which stems from the fact that it was directly ported from the perl version (which is arguably even worse). I believe the issue arises here:

inferno/src/flamegraph/merge.rs

Line 43 in 432d653

while last.peek() == this.peek() {

Specifically, what happens is that the merge logic tries to find the shared prefix between two consecutive entries to determine what frames should be added or removed. In your case, the shared prefix is the entire entry, so the two entries are merged (shared_depth += 1). None of the subsequent code which adds/removes stacks is run since the suffix is empty.

The simple solution here would be to notify this method that we're in flamechart mode, and then modify the code such that if this == last and we're in flamechart mode, it ends the shared prefix loop one iteration early. The rest of the code should take care of adding the appropriate frames.

That solution has a shortcoming though, which is that if given stacks like:

a 1
a;b;c 1
a;b;c 1

It'll render two c frames on a shared stack of a;b, which may not be what you'd expect. If you'd want separate b;c stacks on a shared a stack, you'd need to end the prefix loop based on the difference in depth of last and the depth of the stack before last.

[Mark-Simulacrum]

Hm, interesting. I had not thought about the complication you note with determining where in the stack we should split. I suspect that the current textual format is just not capable of representing the desired information, the interface needs to represent a tree and that's not possible by just listing parents of nodes.

That said, there is a workaround -- we can support some way of identifying nodes uniquely, e.g., name#ID and the ID would be ignored and not displayed. That would mean that the tree structure could be represented faithfully in the current format. But that also seems hard to do -- #ID is likely to conflict with someone's names...

[jonhoo]

In some sense, this gets back to the point I was making in #186 — inferno is currently built to match flamegraph very closely, and that includes its assumptions about how applications are profiled (by sampling) which are encoded all the way down to the input format (the "collapsed" text files).

I think long term, the solution here is to try and move inferno away from the assumptions that flamegraph makes. The biggest thing here will be to make collapsing just one way to feed data to the flamegrapher. Essentially, we'd want #30 to give an API that give the caller more control over how time is measured and frames are rendered, and then implement an "adapter" that takes a collapsed text file and uses that API to produce what it does today. Whereas more sophisticated implementations (like yours) could use the API to render what they want.

Now, realistically, that is a decent amount of effort. It's not even clear to me what such an API should look like. Maybe what we want is to add an additional "tree constructor stage", which consists essentially of the flamegraph flow function. So the pipeline would go:

collapse -> build tree -> graph tree

In your case, you would forego the first two stages, and just directly graph your tree. For the perf-based backend (and the others that need collapsing), they would use the collapser they use today, then they'd all use the same tree builder (merge::flow), and then they'd pass that to the grapher.

[Mark-Simulacrum]

Hm, actually, I think I found a solution -- sort of a hack, but it seems to work.

Each distinct call stack can be delimited by a zero-width spacer which will not even be included in the final SVG, so is entirely transparent to the user I think.

For example, if you had a calling b twice, which in turn called c, you would encode that like so:

a 1
a;b;c 1
a;spacer 0
a;b;c 1

But if a called b once, which then called c twice, then that would be encoded like this:

a 1
a;b;c 1
a;b;spacer 0
a;b;c 1

I don't know if it makes sense to document this somehow...

[jonhoo]

Yes, I like that workaround! And best of all, it requires no changes :p

I agree with you that it'd be good to document. Since there isn't really a library API at the moment, it's not quite clear where that documentation would go though. The crate-level documentation is currently mainly about the command-line interface, so that doesn't seem quite right. My inclination for now is to place it in the module docs for the flamegraph module. That's where it's most likely to be read.