Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support arbitrary scaling factor #19

Closed
jonhoo opened this issue Feb 2, 2019 · 7 comments
Closed

Support arbitrary scaling factor #19

jonhoo opened this issue Feb 2, 2019 · 7 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@jonhoo
Copy link
Owner

jonhoo commented Feb 2, 2019

The --factor option in flamegraph.pl lets you arbitrarily multiply all sample counts by a constant, probably to show "true counts" if you've undersampled. We should document and adopt that option.

@jonhoo jonhoo added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Feb 2, 2019
@jasonrhansen jasonrhansen self-assigned this Feb 27, 2019
@jasonrhansen
Copy link
Collaborator

I did an initial implementation of this and realized that the output sometimes differs between my version and the Perl version because of differences in the way floating point numbers are rounded when formatting to a string compared to calling round on an f64.

712.5f64.round() as usize results in 713, but print!("{:.0}", 712.5); prints 712.

The Perl version formats the samples like this:

my $samples = sprintf "%.0f", ($etime - $stime) * $factor;

But our version is using num_format, which doesn't support formatting floating point values. Currently I'm doing this before formatting:

let samples = ((frame.end_time - frame.start_time) as f64 * opt.factor).round() as usize;

Do we want to come up with a way to get the sample counts to match the Perl version in every case, or should we not worry about it?

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 28, 2019

I think we specifically need the result to not be a usize, since factor may be something like 0.01. @bcmyers any plans to add support for floating point numbers to num-format?

@jasonrhansen
Copy link
Collaborator

The Perl version formats the samples in the end without the fractional part. Are you saying you want to diverge from this behavior? If so, how many decimal places would we want to show?

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 28, 2019

Ohh, you're entirely right! I misread that part of the Perl code. I think what we want here is f64::floor or f64::trunc (not sure how they're different), both of which would exactly match the Perl version. Let's continue discussion in #65.

@bcmyers
Copy link
Contributor

bcmyers commented Feb 28, 2019

Yes - I had always planned to add floating point support to num-format, but it's a bit more complicated than integers. I'd like to read and (at least partially) understand some of the following kind of stuff before I get started (just a sample of various stuff on the topic that's out there):

But if inferno needs it, I'd be happy to prioritize the feature!

Doing a very naive implementation, by the way, would be super easy, but it would be just that, a very naive implementation. But if you wanted a temporary stop-gap feature added, I could do the naive version quickly while I work on implementing a faster algorithm for later release.

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 28, 2019

@bcmyers thanks, I appreciate that, but from the discussion above I think it's fine for us to just convert to an integer and then format, as that's what the Perl implementation is doing :)

@bcmyers
Copy link
Contributor

bcmyers commented Feb 28, 2019

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants