Add collapser for built in Visual Studio profiler #218

KnapSac · 2021-09-15T08:18:07Z

This adds a stack collapser for the built in Visual Studio profiler.

Closes #217

codecov · 2021-09-15T08:23:22Z

Codecov Report

Merging #218 (58818b6) into master (5ec68ae) will increase coverage by 0.29%.
The diff coverage is 92.85%.

@@            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

Impacted Files	Coverage Δ
src/collapse/mod.rs	`52.63% <ø> (ø)`
src/collapse/vsprof.rs	`92.68% <92.68%> (ø)`
src/collapse/guess.rs	`70.73% <100.00%> (+1.50%)`	⬆️

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 5ec68ae...58818b6. Read the comment docs.

src/collapse/vsprof.rs

KnapSac · 2021-09-18T17:02:21Z

@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?

jonhoo · 2021-09-19T01:46:21Z

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 svg in tests/data/collapse-vsprof/results/sample-default.svg doesn't appear to be a valid SVG file though, which doesn't seem right?

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 ", strip all ,, and then parse as an integer?

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!

KnapSac · 2021-09-20T19:44:50Z

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 ;).

jonhoo

Excellent! Left some more comments :)

src/collapse/mod.rs

tests/collapse-vsprof.rs

jonhoo · 2021-10-03T19:22:54Z

src/collapse/vsprof.rs

+    stack: Vec<(String, usize)>,
+}
+
+impl Collapse for Folder {


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.

This point still stands :)

src/collapse/vsprof.rs

KnapSac · 2021-10-13T09:58:07Z

@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...

jonhoo · 2021-10-18T00:03:09Z

Hmm, that's weird. It seems like it thinks there are no new commits on your master branch, which is what this PR was opened for. So either you'd need to push your commits to your master branch again (and then we should be able to reopen), or just open a new PR from a different branch :)

KnapSac · 2021-10-23T20:06:24Z

@jonhoo I've addressed some of the review comments, I hope to get to the last few some time next week.

jonhoo · 2022-01-11T02:14:08Z

Given that we've now bumped the MSRV (in #227), you can use split_once again!

KnapSac · 2022-03-17T20:35:38Z

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 master quite a bit, it's probably wise to update it. I feel like the PR would benefit from rebasing onto master and then squashing most of the commits, but I just wanted to know whether you're ok with that, or whether you'd rather I merge with master. Especially the commits regarding the MSRV change create a bit of unnecessary noise, which I'd like to get rid of.

Updating will also fix the build I believe.

jonhoo · 2022-03-19T03:56:27Z

All good, thanks for coming back to it! Yes, please do squash and rebase onto master and I'll do another review 👍

jonhoo

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 👍

jonhoo · 2022-04-02T16:55:48Z

src/bin/collapse-vsprof.rs

+use std::path::PathBuf;
+
+use clap::Parser;
+use env_logger::Env;


Note to self: we should really move over to tracing.

src/collapse/mod.rs

src/collapse/vsprof.rs

jonhoo · 2022-04-02T16:59:06Z

src/collapse/vsprof.rs

+    stack: Vec<(String, usize)>,
+}
+
+impl Collapse for Folder {


This point still stands :)

src/collapse/vsprof.rs

jonhoo

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!

src/collapse/vsprof.rs

jonhoo

Excellent, thank you for sticking with it!

jonhoo · 2022-04-24T17:42:19Z

Published as 0.11.2 🎉

jonhoo reviewed Sep 17, 2021

View reviewed changes

src/collapse/vsprof.rs Outdated Show resolved Hide resolved

KnapSac force-pushed the master branch from e1760f9 to b6009a6 Compare September 18, 2021 14:24

KnapSac marked this pull request as ready for review September 18, 2021 17:02

KnapSac marked this pull request as draft September 20, 2021 19:44

KnapSac marked this pull request as ready for review October 1, 2021 22:21

KnapSac requested a review from jonhoo October 1, 2021 22:22

jonhoo requested changes Oct 3, 2021

View reviewed changes

jonhoo mentioned this pull request Oct 13, 2021

Update MSRV and fix clippy warnings #219

Merged

KnapSac closed this Oct 13, 2021

KnapSac force-pushed the master branch from 0f965ac to f62187c Compare October 13, 2021 08:17

KnapSac reopened this Oct 23, 2021

KnapSac requested a review from jonhoo March 17, 2022 20:35

KnapSac force-pushed the master branch 3 times, most recently from be479d4 to 21c7913 Compare March 19, 2022 15:00

Add collapser for built in Visual Studio profiler

3ffc5d5

KnapSac force-pushed the master branch from 21c7913 to 3ffc5d5 Compare March 22, 2022 20:22

jonhoo requested changes Apr 2, 2022

View reviewed changes

KnapSac added 2 commits April 2, 2022 22:40

Address review comments

71c9f66

Add unit tests for get_next_number

4b4f0c3

KnapSac requested a review from jonhoo April 2, 2022 21:01

Update comment to convey intent more clearly

4a0e0f8

jonhoo requested changes Apr 10, 2022

View reviewed changes

src/collapse/vsprof.rs Outdated Show resolved Hide resolved

src/collapse/vsprof.rs Outdated Show resolved Hide resolved

jonhoo reviewed Apr 10, 2022

View reviewed changes

src/collapse/vsprof.rs Outdated Show resolved Hide resolved

Address final review comments

5a5ee55

KnapSac force-pushed the master branch from 262ba88 to 5a5ee55 Compare April 11, 2022 09:07

New implementation of get_next_number

58818b6

jonhoo approved these changes Apr 24, 2022

View reviewed changes

jonhoo merged commit 190570a into jonhoo:master Apr 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add collapser for built in Visual Studio profiler #218

Add collapser for built in Visual Studio profiler #218

KnapSac commented Sep 15, 2021 •

edited

codecov bot commented Sep 15, 2021 •

edited

KnapSac commented Sep 18, 2021

jonhoo commented Sep 19, 2021

KnapSac commented Sep 20, 2021

jonhoo left a comment

jonhoo Oct 3, 2021

jonhoo Apr 2, 2022

KnapSac commented Oct 13, 2021

jonhoo commented Oct 18, 2021

KnapSac commented Oct 23, 2021

jonhoo commented Jan 11, 2022

KnapSac commented Mar 17, 2022

jonhoo commented Mar 19, 2022

jonhoo left a comment

jonhoo Apr 2, 2022

jonhoo Apr 2, 2022

jonhoo left a comment

jonhoo left a comment

jonhoo commented Apr 24, 2022

	/// 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 {

Add collapser for built in Visual Studio profiler #218

Add collapser for built in Visual Studio profiler #218

Conversation

KnapSac commented Sep 15, 2021 • edited

codecov bot commented Sep 15, 2021 • edited

Codecov Report

KnapSac commented Sep 18, 2021

jonhoo commented Sep 19, 2021

KnapSac commented Sep 20, 2021

jonhoo left a comment

Choose a reason for hiding this comment

jonhoo Oct 3, 2021

Choose a reason for hiding this comment

jonhoo Apr 2, 2022

Choose a reason for hiding this comment

KnapSac commented Oct 13, 2021

jonhoo commented Oct 18, 2021

KnapSac commented Oct 23, 2021

jonhoo commented Jan 11, 2022

KnapSac commented Mar 17, 2022

jonhoo commented Mar 19, 2022

jonhoo left a comment

Choose a reason for hiding this comment

jonhoo Apr 2, 2022

Choose a reason for hiding this comment

jonhoo Apr 2, 2022

Choose a reason for hiding this comment

jonhoo left a comment

Choose a reason for hiding this comment

jonhoo left a comment

Choose a reason for hiding this comment

jonhoo commented Apr 24, 2022

KnapSac commented Sep 15, 2021 •

edited

codecov bot commented Sep 15, 2021 •

edited