-
Notifications
You must be signed in to change notification settings - Fork 125
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
Add collapser for built in Visual Studio profiler #218
Conversation
Codecov Report
@@ Coverage Diff @@
## master #218 +/- ##
==========================================
+ Coverage 86.88% 87.17% +0.29%
==========================================
Files 17 18 +1
Lines 2401 2526 +125
==========================================
+ Hits 2086 2202 +116
- Misses 315 324 +9
Continue to review full report at Codecov.
|
@jonhoo I'm not entirely sure what to do about the code coverage test failing, looking at the diff it seems to me as if they're erroneously marking some lines as not covered. Do you have any ideas? I only tested the output of a C# program, as I don't have any C++ projects to profile. Additionally, I don't know what other languages the built in Visual Studio profiler supports, which there are probably quite a few of. I suspect that the output for all of them is the same, and as such they shouldn't have any issues with this implementation. In C# the namespace is included in the function name output (so the function name is actually the fully qualified name of the function), this makes some function names incredibly long, especially if the function also has several arguments with long fully qualified names. One idea I had to make the output more readable was to introduce an option to specify the number of namespaces which should be kept and trim any extra namespaces. Another idea could be to add an option to keep the fewest namespaces possible, while still being unique. Both might also not be a bad idea. Is this something you'd like to discuss/incorporate into this PR, or should a discussion about that be moved to a new issue? The latter would have my preference, as this is, although related, not a requirement for a working implementation. What are your thoughts on this? |
I wouldn't worry too much about the code coverage — it tends to be overly sensitive. Usually it's because error paths aren't covered, but I'm okay with that for a first PR :) I did notice that the output I'm also not entirely sure why you chose to not support lines with a depth deeper than 1000? It seems fairly simple to parse the contained numbers? Just find the next For namespaces, I agree with you that such trimming functionality is something we can discuss separately. No need to implement that here, even if it may be a good idea to add later! I'm a bit short on time, so may take a bit until I have a chance to give this a thorough review, but hopefully the above unblocks you for a little while at least. Thanks for taking the time to implement this! |
The svg output indeed does not appear to be correct, when I have some time later this week I'll look into it. As to not supporting lines with a depth over 1000, I don't have any good reason for that. On second thought, whether or not it is actually possible to have a call stack over 1000 deep, we shouldn't introduce an arbitrary restriction on that. I'll also fix that. For now I'll mark this PR as a draft, when I've resolved these issues I'll mark it as ready. Don't worry about being short on time, as a student I know the feeling ;). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! Left some more comments :)
stack: Vec<(String, usize)>, | ||
} | ||
|
||
impl Collapse for Folder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want a slight challenge (definitely not required for the PR to land!), try implementing the CollapsePrivate
trait instead:
inferno/src/collapse/common.rs
Lines 53 to 59 in 410018b
/// Private trait for internal library authors. | |
/// | |
/// If you implement this trait, your type will implement the public-facing | |
/// `Collapse` trait as well. Implementing this trait gives you parallelism | |
/// for free as long as you adhere to the requirements described in the | |
/// comments below. | |
pub trait CollapsePrivate: Send + Sized { |
That will give you multi-core file processing, which can speed up collapsing quite a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This point still stands :)
@jonhoo it seems I made a bit of a mess while preparing #220, and now Github won't let me reopen this PR :(. Do you have any ideas on how to reopen this PR, or do I have to make a new one? It seems Github is a bit confused, because it says that there are no new commits on my master branch, despite the fact that I added one this morning... |
Hmm, that's weird. It seems like it thinks there are no new commits on your |
@jonhoo I've addressed some of the review comments, I hope to get to the last few some time next week. |
Given that we've now bumped the MSRV (in #227), you can use |
Wow, this has taken me a lot longer to get back to than I would have liked, my apologies. I believe I have addressed all the review comments you left during your first review, and all my tests pass. The produced flamegraphs also look correct to me visually (I just created a simple program with only a few functions to make it easier to check correctness visually). Since this branch has fallen behind Updating will also fix the build I believe. |
All good, thanks for coming back to it! Yes, please do squash and rebase onto master and I'll do another review 👍 |
be479d4
to
21c7913
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew, finally found some time to review! Thanks again for coming back to this. I've left some more notes inline, though I think they're all fairly minor 👍
use std::path::PathBuf; | ||
|
||
use clap::Parser; | ||
use env_logger::Env; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: we should really move over to tracing
.
stack: Vec<(String, usize)>, | ||
} | ||
|
||
impl Collapse for Folder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This point still stands :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something definitely still tugs at me about the A calls B case, but I don't think it's worth blocking this on. It may just be me failing to read in enough detail. And if someone finds an issue later, we'll find out about it then!
I found two more nits, and with those fixed I think we're finally ready to merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thank you for sticking with it!
Published as 0.11.2 🎉 |
This adds a stack collapser for the built in Visual Studio profiler.
Closes #217