Upstream flamegraph doesn't discard fractional samples

[zao]

I'm still watching part 2 but given the current state of the source code, I believe that you may have misinterpreted the part of the regex in upstream FlameGraph about removing the fractional part of the samples.

my($stack, $samples) = (/^(.*)\s+?(\d+(?:\.\d*)?)$/);

inferno/src/flamegraph/merge.rs

Lines 94 to 98 in ce1c41a

	// strip fractional part (if any);
	// foobar 1.klwdjlakdj
	if let Some(doti) = samples.find('.') {
	samples = &samples[..doti];
	}

My reading is that the non-capturing group is there to allow input in either integer or fractional format, but not in any way remove it from the surrounding capture. The non-capturing group is there to group up the separator and the fractions for the following ?.

That is, it accepts sample values like 12, 34., and 56.78.

[jonhoo]

Ah, yes, I think you're right! That would mean that the sample counts are all fractional (probably f64). Would you be willing to write up a PR with that change?

[zao]

I don't think I've got the mental bandwidth currently to fully implement this as it propagates throughout the program when you make the samples floating point.

In particular, it affects the lookups of TimedFrame due to f64 not having an ordering for Eq nor having a Hash.

I'd gladly leave it for another contributor or for a stream where you get to explain the Fun in why floats don't have decent orderings.

[jonhoo]

One trick we could play here is to upsample by, say, 100x, and then keep using usize 🤔

[jasonrhansen]

As far as I can tell there is no reason TimedFrame needs to be Eq or Hash. Frame is used as a key in a HashMap, but it doesn't have any fields relating to samples. TimedFrame is only ever stored in a Vec. I think we should be able to use f64 for start_time, end_time, and delta, or am I overlooking something?

[jonhoo]

I did some digging on this in #64 (comment), and my conclusion from reading brendangregg/FlameGraph#18 is that we should probably simply discard the fractional parts of samples, and add support for --factor instead.

[jasonrhansen]

#65 adds --factor instead.