Confusing behaviour in area selection of multiple perf sample types

[LalitMaganti]

Here's an example of an area selection I made just now

So this was a trace with many different types of callstacks involved. In #4638, @cphlipot1 changed things so that, by default, the callstacks use the perf counter to weight the flamegraph. However, in mixed counter area selections like this, this can be quite misleading as you're only really seeing the callstacks visualized for one of the tracks you selected not all of them.

Some ideas:

1. We should make the flamegraph universally show sample count by default. This is a slight revert of ui: weight flamegraphs by perf event count #4638 but is intuitive and clear for most people's usecases but will be annoying when you do care about the counter
2. We should do nothing - IMO not preferred due to the surprise factor.
3. We could make it so that if there's a single counter, we use the counter by default and if multiple we use sample count by default.

Personally, I'm leaning towards 3 but open to suggestions.

@cphlipot1 @rsavitski for thoughts

[cphlipot1]

Option 1 or 3 sound good to me, although have a slight preference towards 3.

I also agree option 2 is is the least preferable option based on what you have shown above. As a user, I don't think i would ever want "binder_transaction" to be the default when "cpu-clock" is available.

Just to throw out an idea for an "Option 4", we could prioritize "cpu-cycles" over other events like "binder_transaction" which IMO is generally what people would want to see when looking at a flamegraph to understand code hot spots, and would also be an improvement over option2. This brings up the question though of what you do when cpu-clock is not present though.

[cphlipot1]

Also with respect to showing total sample count by default, I'm a bit unclear what the benefit of mixing samples from many different kinds of events into a single flamegraph would be. For my own usages i would probably only want to view these as separate flame graphs.

I think there is a subset of Perfetto users who do not understand lower level details perf events, sampling or how flamegraphs are constructed, and simply equate flamegraph=time spent in code. I have slight concerns that if we aggregate sample counts from all these different events, what we are actually displaying in the flamegraph may not be obvious to the users and may not be what they would expect the flame graph to show by default.

If this sounds like a valid concern to you, we may want to consider something along the lines of my "option 4" above.

[LalitMaganti]

I think there is a subset of Perfetto users who do not understand lower level details perf events, sampling or how flamegraphs are constructed, and simply equate flamegraph=time spent in code. I have slight concerns that if we aggregate sample counts from all these different events, what we are actually displaying in the flamegraph may not be obvious to the users and may not be what they would expect the flame graph to show by default.

I 100% agree with you in this paragraph. I'm very certain there are users like this. I guess my unease comes from the fact that, in general in Perfetto UI, we don't bother distinguishing between "breakdowns" of events in this way. For example, you could make the argument in the table for details panels, we should distinguish between "async" and "sync" slices in our table and/or flamegraph. But we don't because we are really reasoning interms of "category of event" and trying to give you the most consistent view across that dataset.

Though you could argue: we don't merge callstacks across different sources (e.g. track event callstacks vs perf callstacks) so why is this any different? And that's exactly why I even raised this issue :)

If this sounds like a valid concern to you, we may want to consider something along the lines of my "option 4" above.

I think I still maybe slightly prefer option 2 but this would be my fallback certainly!

[cphlipot1]

Right now i think right now the current behavior (option 2) just sorts events by name and which ever appears at the top of the list is what gets displayed by default. In your case this is "binder_transaction" because it starts with the letter 'b', so the current logic we for what we display by default is rather arbitrary.

I think a more opinionated sort could make sense if that was something we wanted to explore. for example i thiink the following sort order could be sensible:

1. HW CPU cycles events
2. SW CPU clock events
3. instructions
4. all other events.

The first 3 above in general would correlate with time spent in code, and might make more sense to show at top of the list and be displayed by default if available.

[LalitMaganti]

I think we should certainly explore sorting independently of what the default should be. Also want to hear from Ryan as well.