Fix #173 Templated C++ functions cut off after opening parenthesis in template arguments.

[SoftwareApe]

Fix too eager truncation of template parameters on opening parentheses in C++

• Add failing tests for C++ with std::function in template arguments.
• Use rfind to find the last opening parenthesis, which is most likely the function aruguments
• Check if the opening parenthesis is inside a C++ function template, by counting opening vs closing angle brackets.

[SoftwareApe]

Fixes #173

[jonhoo]

I think the failures here are legitimate (you can see the actual error by clicking "Details", then "View more details on Azure Pipelines", then clicking the failing build step in the list on the left.

The style check fails because you haven't run rustfmt (cargo fmt should fix it for you).

The main tests fail because this changes the output of an existing test:

-"java;start_thread;java_start;GCTaskThread::run;ThreadRootsTask::do_it;JavaThread::oops_do;frame::oops_do_internal;OopMapSet::all_do;PSRootsClosure<false>::do_oop;oopDesc* PSPromotionManager::copy_to_survivor_space<false>;InstanceKlass::oop_push_contents 1"
+"java;start_thread;java_start;GCTaskThread::run;ThreadRootsTask::do_it;JavaThread::oops_do;frame::oops_do_internal;OopMapSet::all_do(frame const*, RegisterMap const*, OopClosure*, void (*);PSRootsClosure<false>::do_oop;oopDesc* PSPromotionManager::copy_to_survivor_space<false>;InstanceKlass::oop_push_contents 1"

Specifically:

-"OopMapSet::all_do;
+"OopMapSet::all_do(frame const*, RegisterMap const*, OopClosure*, void (*);

I think what's going wrong is that your new code fails to take into account when there are parentheses inside the function arguments too. In that case, your code will only remove what trails the last (, whereas the old code would strip entire argument list (which is what should happen).

[jonhoo]

You should also be able to reproduce the test failure locally by running cargo test?

[SoftwareApe]

Was missing the submodule. Since the code compiled I didn't think there were any.

[jonhoo]

Yeah, the submodule is only needed for tests. Sorry! I wish there was a way to tell git to always check out the submodule when cloning a repo :'(

[SoftwareApe]

So I found the Rust stacks were also mangled due to, e.g. ...FromIterator<(K,V)>>::from_iter being cut off at the last parenthesis.
So it's not only C++ templates that are an issue here.

However the js expectations are also "wrong" now. I'm saying wrong, since they don't get cut off when they have template parameters. My basic question would be why we even want to get rid of the parameter list? After all if it just says Call or Invoke, you know very little about the call stack. With the parameters it's much more informative.

Wouldn't it be better to just leave the parameter lists inside?

[SoftwareApe]

For example, without the parentheses this wouldn't be informative at all:
v8::Function::Call(v8::Handlev8::Value, int, v8::Handlev8::Value*)

[jonhoo]

The answer is (perhaps unhelpfully) that we do it because that's what flamegraph did. Now, that's not a very good reason in and of itself, but my guess was that they decided that the argument lists in general only contain redundant information once you have the function name. And I think in general that observation is correct. For the v8 example you give above, for example, what information does the arguments give that you can't get from the source definition of v8::Function::Call?

My preference would be to stick with pruning argument lists for now at least.

[jonhoo]

I don't think this will quite work either. Consider, for example:

foo(Vec<usize>)

[SoftwareApe]

True, probably actually need to do some recursive descent parsing here.

[jonhoo]

Oh, by the way, we should probably add all these interesting weird cases we come across now as unit tests for that function!

[SoftwareApe]

The answer is (perhaps unhelpfully) that we do it because that's what flamegraph did. Now, that's not a very good reason in and of itself, but my guess was that they decided that the argument lists in general only contain redundant information once you have the function name. And I think in general that observation is correct. For the v8 example you give above, for example, what information does the arguments give that you can't get from the source definition of v8::Function::Call?

My preference would be to stick with pruning argument lists for now at least.

I'd say you don't know anything about which function is called here, if there's no argument list.

Oh, by the way, we should probably add all these interesting weird cases we come across now as unit tests for that function!

I already fixed the broken test cases on the Rust example.

[jonhoo]

Right, but the argument list that you printed also tells you nothing more, since those are just the arguments that Call takes, not the values that were passed. Unless you're saying that v8::Function::Call is special, and the arguments that get printed are the arguments of the underlying JavaScript function that was called? If so, it feels like something we should special-case. In general, the function arguments are at least deterministic from the function name.

I already fixed the broken test cases on the Rust example.

Ah, yes, what I meant was more that we should add these as unit tests for tidy_generic so that a future change doesn't accidentally regress on parsing these things. And it's easier to figure these things out from a unit test than a giant folded file diff.

[codecov]

Codecov Report

Merging #174 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #174   +/-   ##
=======================================
  Coverage   90.23%   90.24%           
=======================================
  Files          16       16           
  Lines        2213     2224   +11     
=======================================
+ Hits         1997     2007   +10     
- Misses        216      217    +1     

Impacted Files	Coverage Δ
src/collapse/perf.rs	97.37% <100.00%> (-0.22%)	⬇️
src/flamegraph/color/palette_map.rs	95.57% <0.00%> (-0.89%)	⬇️
src/flamegraph/color/mod.rs	81.11% <0.00%> (-0.70%)	⬇️
src/flamegraph/svg.rs	97.82% <0.00%> (-0.10%)	⬇️
src/collapse/common.rs	62.87% <0.00%> (ø)
src/flamegraph/mod.rs	96.30% <0.00%> (+0.35%)	⬆️
src/flamegraph/merge.rs	94.73% <0.00%> (+1.05%)	⬆️

---

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 23df1d4...615bd38. Read the comment docs.

[SoftwareApe]

I now went with the matching of parentheses. The tidying now doesn't break the old tests (except the faulty Rust tests, which I updated), I added the small unit test for tidy_generic with some edge cases we found here.

[jonhoo]

This looks great now, thank you!

[SoftwareApe]

Thank you for your quick response and helpful comments.

[jonhoo]

Thank you for sticking with it even with all my comments 😅
Released as 0.9.7!

[jonhoo]

Oh, I also took the liberty of squashing your commit and cleaning up the commit message a little. Hope you don't mind. See 2230ac7.