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

Implement --factor option #65

Merged
merged 7 commits into from
Feb 28, 2019
Merged

Conversation

jasonrhansen
Copy link
Collaborator

Implements #19 with a difference from the Perl implementation in how sample counts get rounded, as explained in my comment on that issue. We need to decide if this is acceptable or not.

I've also combined all of the flamegraph tests into one file since they all duplicated the main testing code.

@codecov-io
Copy link

codecov-io commented Feb 28, 2019

Codecov Report

Merging #65 into master will increase coverage by 0.04%.
The diff coverage is 93.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   77.54%   77.59%   +0.04%     
==========================================
  Files          15       13       -2     
  Lines        1287     1263      -24     
==========================================
- Hits          998      980      -18     
+ Misses        289      283       -6
Impacted Files Coverage Δ
src/bin/flamegraph.rs 0% <0%> (ø) ⬆️
src/flamegraph/mod.rs 84.34% <100%> (ø) ⬆️
tests/flamegraph.rs 95.23% <100%> (ø)
src/flamegraph/merge.rs 92.13% <84.61%> (+0.46%) ⬆️
src/collapse/perf.rs 82.41% <0%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4863e01...b70479b. Read the comment docs.

src/bin/flamegraph.rs Outdated Show resolved Hide resolved
src/flamegraph/merge.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Feb 28, 2019

Looks good beyond my comment in #19 (comment). Specifically, I think it's not actually okay for us to round and truncate the result to an integer -- it may be fractional after multiplying by the factor.

@jonhoo
Copy link
Owner

jonhoo commented Feb 28, 2019

Thinking some more, I think we specifically want f64::trunc, since f64::floor would give the next lower negative number. Not that we expect any of these to be negative, but 🤷‍♂️ See also rust-lang/rust#58812.

@jasonrhansen
Copy link
Collaborator Author

Using f64::trunc doesn't match the Perl version either. It's more complicated than that because of the weirdness of formatting floating point numbers. round actually works most of the time, but it's in the middle at .5 where we have issues.

printf "%.0f", 1.5 outputs 2
printf "%.0f", 2.5 outputs 2
printf "%.0f", 3.5 outputs 4

If we round we get 2, 3, 4.
If we truncate we get 1, 2, 3.

If we didn't care about thousands separators we could just format it the same way as the Perl version.

@jonhoo
Copy link
Owner

jonhoo commented Feb 28, 2019

Wow, that's bizarre. Is that the case in Rust too? That makes no sense to me. But then yes, I think round() is fine!

@jasonrhansen
Copy link
Collaborator Author

Rust formats exactly the same way when using "{:.0}". I'll keep using round, but add a comment about how this slightly differs from the Perl version.

@jonhoo
Copy link
Owner

jonhoo commented Feb 28, 2019

Sounds good! I wonder where that behavior comes from...

src/flamegraph/merge.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Feb 28, 2019

🎉 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants