Skip to content

Add flamegraph profiling using flame and flamer #1120

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

Merged
merged 2 commits into from
Jul 9, 2019

Conversation

coolreader18
Copy link
Member

@coolreader18 coolreader18 commented Jul 8, 2019

This is just an initial change to get the ball rolling for flame profiling, we can add more scopes later.

@coolreader18
Copy link
Member Author

There's not a ton of information here that would be helpful to optimization (yet), but it's really interesting to see!

image

@coolreader18 coolreader18 force-pushed the coolreader18/flame-profiling branch from d662bfa to f585f7e Compare July 9, 2019 05:10
@windelbouwman
Copy link
Contributor

This is pretty cool! Is it mandatory to add the flame-it attribute to each function you wish to track? Can we just follow all functions?

// flame doesn't include notes in the html graph :(
flame_note!(
flame::StrCow::from("CodeObj name"),
self.code.obj_name.clone().into()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Now we can follow python objects as well!

@@ -334,6 +342,7 @@ impl Frame {

/// Execute a single instruction.
#[allow(clippy::cognitive_complexity)]
#[cfg_attr(feature = "flame-it", flame("Frame"))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to explicitly enable all trace points?

@windelbouwman
Copy link
Contributor

Off topic: why does the importlib loading take so long? Is it a whole lot of code?

@coolreader18
Copy link
Member Author

re: importlib: The main problem is that it is a fair bit of code, and RustPython isn't all that optimized. See #1047.

re: automatically tagging all fns: see llogiq/flamer#45. I tried to do that initially, and there were a lot of issues with it. Even so, manually tagging all fns in frame.rs with #[flame] gave way too much irrelevant data, and the file size was huge as well (36MB compared to the 1MB with the current version here). e.g., do we really need to see how long each individual exception creation function takes?

@windelbouwman windelbouwman merged commit 53ce362 into master Jul 9, 2019
@coolreader18
Copy link
Member Author

I had to use a modified local clone of flame to get this flamegraph due to a bug (llogiq/flame#47), but something interesting that you can see is that in _install_external_importers, when it imports _frozen_importlib_external, a fairly large portion of the time spent in __import__ is before it even starts executing the module (Frame::run(<module>)). Maybe we can detect when it's importing _frozen_importlib_external from _frozen_importlib and use Rust to import it?

@coolreader18 coolreader18 deleted the coolreader18/flame-profiling branch July 9, 2019 07:09
@coolreader18
Copy link
Member Author

Here it is with a simple optimization to skip __import__ in that case: link. Shaved off 10ms, not bad!

@palaviv
Copy link
Contributor

palaviv commented Jul 9, 2019

CPython actually have large parts of the __import__ function implemented in C. We can do the same in Rust for optimization.

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.

3 participants