Closed saethlin closed 1 year ago
Base: 90.28% // Head: 90.79% // Increases project coverage by +0.51%
:tada:
Coverage data is based on head (
5cac2b1
) compared to base (198a6a3
). Patch coverage: 16.66% of modified lines in pull request are covered.:exclamation: Current head 5cac2b1 differs from pull request most recent head 765760d. Consider uploading reports for the commit 765760d to get more accurate results
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Hmm, this feels very similar to the --skip-after
argument to collapse-perf
(#222 + #224, #231, cc @romange) — I wonder if it still makes sense to have both?
With something like this I also worry about it giving an inconsistent view since it doesn't trim things to a particular stack, it picks things from potentially unrelated stacks and combines them (which, to be fair, --skip-after
also does).
it picks things from potentially unrelated stacks and combines them
Yeah, that's what I'm going for here. The idea is that when you're using this flag you don't care how control flow arrived at the function of interest. Of course the path taken to the function might matter, but for the cases I'm interested in the alternative is having a number of low-signal versions of the flames scattered all over, which in my experience is much worse because people tend to just search for the symbol and only look at the biggest pink rectangle. Maybe the biggest two.
I'll admit I didn't notice that --skip-after
exists. Unfortunately I'd rather not use it :/ . I'm using the folded stacks files as a portable store of profile data. perf.data
files aren't portable, the final SVGs are too big, and the simple text nature of the folded stacks format lends itself to ad-hoc analysis with command-line tools when the SVGs aren't up to the task. So a flag on inferno-collapse-perf
is at the wrong stage of the analysis. I can't take one perf
recording and render two --skip-after
flamegraphs from it without running the collapser twice. And if I all I have is the folded stacks (as opposed to the output of perf script
, or perf.data
in the requisite environment), --skip-after
is no help.
I also want filtering behavior which --skip-after
does not provide, so that my browser doesn't crash. For whatever reason chromium gives up on SVGs over too many MB in size. Though of course I can put grep
between the collapser and inferno-flamegraph
to get that effect.
That's a good point. Makes me wonder if we should mark --skip-after
as deprecated and encourage the use of this new flag instead. What do you think? Also cc @bsilver8192.
Another option here is to provide this as a separate binary, like inferno-diff-folded
, where you pass in one folded file and it produces another one. That way the tool would actually be useful even for folks who use "standard" flamegraph.
That's a good point. Makes me wonder if we should mark
--skip-after
as deprecated and encourage the use of this new flag instead. What do you think? Also cc @bsilver8192.
I'd be fine doing the filtering at either point. I do need the ability to pass multiple strings though, not just one like this PR currently supports. I end up using --skip-after
with the top-level function for each thread.
Specifically, I run into issues with stack unwinding from some places not making it all the way through the thread startup code, while it does from other places. That means what is in fact the same stack looks different because some of them are missing a few of the highest levels. The resulting stack traces look like the ones mentioned in this PR, but it's not actually merging unrelated stacks, it's merging the same stack that looks different.
@saethlin How do you feel about adjusting this so that it can handle more than one filter to also cater to @bsilver8192's use-case?
I'm not entirely sure how this should be tested. I could factor out the filtering logic into a helper function and unit test it? I couldn't find any tests for skip_after
, I was thinking the easiest thing would be to verify that it works exactly the same.
I would be happy with just integration here that specifically check that with a given input file, we produce the right collapse when 1/n bases are provided. Should be a pretty straightforward variant of the existing collapse integration tests I think.
Released as 0.11.14 :tada:
I'm not really sure what to call this, but I've been using this out of my fork for a while and I think it's pretty useful. Especially when working on a codebase that is calling the function of interest inside a rayon parallel iterator.