rewrite the speedscope|chromium exporter logic to ensure that the output is always correct

[adamsitnik]

This is a proper fix for #1178 (at least I hope so)

Intro to file format

Both speedscope and chromium expect the exported data to be in a certain order.

Let's consider the following application:

void M()
{
    A();
    C();
}

void A() => B();
void B() {}
void C() => D();
void D() {}

The expected order of exported profile events is:

Open M
Open A
Open B
Close B
Close A
Open C
Open D
Close D
Close C
Close M

Imagine that you pick up a pen and you want to draw CPU samples in time order from the left to the right. This order makes it very easy.

Past

When I was implementing the initial version of the exporter, I've decided to expand all leaf CPU samples (walk the stack), aggregate all samples per method, and then sort them by time (ascending) and depth. This approach worked for some scenarios, but not all of them. Recently speedscope has introduced a validation of the input data in jlfwong/speedscope#273 and it has broke us, because our files were not always correct (#1178 dotnet/diagnostics#1206)

Proposal

I think that instead of aggregating all samples by methods and then sorting them, we should compare the current call stack with previous one, and generate the open and close events on the fly. (not horizontal but vertical approach).

I've tested the new implementation against all the trace files I had and it works fine.

@swift-kim I've added an integration test for the trace files that you have provided. Please let me know if this is OK.

Examples:

https://www.speedscope.app/#profileURL=https://gist.githubusercontent.com/adamsitnik/dee155b6a0be32f0bbb70f3f65398592/raw/da422696bacfa95b002d390b1cdb3bf02a7a4aff/HeartRateMonitor.speedscope.json

https://www.speedscope.app/#profileURL=https://gist.githubusercontent.com/adamsitnik/6bd989322777b21aa0c6a1987320dfba/raw/1a1df74b6be3cb903dc0105ea996ea3c7b846c05/VoiceMemo.speedscope.json

@brianrob Could you please take a look? I hope that this fix is going to solve all the problems we had with the exporter once and for all.

[josalem]

LGTM! Comments inline. I think this is a more robust way of creating "continuous" events from non-contiguous data. Thanks for the changes!

[josalem]

Is the factor of 1.001 arbitrary or is this based on some characteristic of the metric?

[adamsitnik]

most of the times when events are continuous the following statement should be true

left.Metric == right.RelativeTime - left.RelativeTime

however, RelativeTime is double while Metric is float and we are sometimes loosing a very small fraction of metric in the conversion. the 1.001 is just to cover this possible diff.

[josalem]

I did a similar analysis for an old hack that I used to do pseudo call counting for a cachegrind format converter.

[swift-kim]

The patch worked for all the trace files I have. Thank you for the great work!

[adamsitnik]

The patch worked for all the trace files I have. Thank you for the great work!

thanks for letting me know!

maybe we can add more sample traces here?

sure! as long as they are not very big we should add few more cases

[brianrob]

@adamsitnik thanks for doing this. Apologies for the delay.

[adamsitnik]

thanks for doing this. Apologies for the delay.

thank you for the review!