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

Refactor "merged callstacks profiler" with new stack traces datastructure #57

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Bluezen
Copy link
Collaborator

@Bluezen Bluezen commented Mar 19, 2023

I have created ParallelStacksProfiler, a quick clone of MergedCallStacksProfiler to test the new stack trace datastructure. Instead of refactoring MergedCallStacksProfiler I wanted to compare both implementation and output side by side.

TreeNode code is very easy to understand, it will certainly be simpler to maintain and to adapt. I really like the idea of using HashMap property to aggregate call stacks, so much cleaner! Overall I think we could simply trash the old implementation. Good job!!

A few remarks though:

  • To print stacks from top to bottom I still have to do a revert (line 132) as the do_stack_snapshot method gives us methods_ids as a stack (to read from bottom to top)... if you think the prefered way to read stacks are the other way around maybe we should do the revert in the build_from_sequences tree method
  • we loose the thread_ids... i know we don't make any use of them right now, so not a big deal at the moment, but I wanted to point it out
  • depth in TreeNode can be discarded I think

@Bluezen Bluezen requested a review from ogxd March 19, 2023 13:16
@Bluezen
Copy link
Collaborator Author

Bluezen commented Mar 19, 2023

We don't have to merge this PR, I pushed it in case you wanted to have a look at how MergedCallStacksProfiler could look like with TreeNode... but something tells me you already have a more generic approach to print TreeNode and maybe already refactored both MergedCallStacksProfiler and CpuHotpathProfiler XD

@ogxd
Copy link
Collaborator

ogxd commented Mar 19, 2023

Nooo haha I just did this, so I was really happy to see your PR. Didn't had time to check it yet however

@ogxd
Copy link
Collaborator

ogxd commented Mar 19, 2023

To print stacks from top to bottom I still have to do a revert (line 132) as the do_stack_snapshot method gives us methods_ids as a stack (to read from bottom to top)... if you think the prefered way to read stacks are the other way around maybe we should do the revert in the build_from_sequences tree method

Ah yesss didn't thought of that. I think you put it in the best place. Maybe the code to rebuild all sequences but reverted can be compacted a bit, will see if I can get something

we loose the thread_ids... i know we don't make any use of them right now, so not a big deal at the moment, but I wanted to point it out

Yeah true.... Made some changes so that it is a little more flexible: #58

depth in TreeNode can be discarded I think

Ok !

@Bluezen Bluezen changed the base branch from stack-utils to master March 20, 2023 13:19
@Bluezen
Copy link
Collaborator Author

Bluezen commented Mar 22, 2023

merged call stacks profiler has been replaced by pstacks profiler that make use of the new tree structure. The output is exactly the same than before. Unfortunately I haven't yet been able to refactor the rendering of the tree and make it available to other profiler... it's a bit tricky

@Bluezen Bluezen changed the title Test of new stack traces datastructure Refactor "merged callstacks profiler" with new stack traces datastructure Mar 24, 2023
@Bluezen Bluezen self-assigned this Mar 24, 2023
@ogxd ogxd force-pushed the master branch 2 times, most recently from ab0f5b8 to 4a8a696 Compare May 27, 2023 23:02
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

2 participants