tikv / pprof-rs

A Rust CPU profiler implemented with the help of backtrace-rs
Apache License 2.0
1.29k stars 99 forks source link

fix: increase stack max depth at 128 #159

Closed viglia closed 2 years ago

viglia commented 2 years ago

At the moment MAX_DEPTH is set to 32.

This is a bit low as a value and in some cases important frames are cut out of the final result.

viglia commented 2 years ago

@YangKeao if this looks good to you and it's merged, could you please create a new release after?

Thank you

YangKeao commented 2 years ago

I'd fix some clippy before merge this PR :smile_cat: .

viglia commented 2 years ago

@YangKeao thanks for taking such a quick look at this 😀

I'd fix some clippy before merge this PR 😸 .

I'm a bit confused about this. Do you mean making clippy less strict about warnings or fixing the warnings themselves?

I see most of the warnings come from the prost create but in that repo the clippy job was totally removed from CI (link) so each time some clippy-unfriendly syntax is merged there it'll have repercussion here in pprof.

I can only see a few warnings coming from pprof.

And sometimes those warnings seems very stricts (see example below) and I'm not sure if they should be treated as an error

error: very complex type used. Consider factoring parts into `type` definitions
Error:   --> src/report.rs:34:28
   |
34 |     frames_post_processor: Option<Box<dyn Fn(&mut Frames)>>,
   |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
YangKeao commented 2 years ago

I'm a bit confused about this. Do you mean making clippy less strict about warnings or fixing the warnings themselves?

See https://github.com/tikv/pprof-rs/pull/160 . I fixed some clippy warnings themselves, and allow all inside the prost mod.

Swatinem commented 2 years ago

I think you can just #[allow(...)] a bunch of clippy rules. Suggesting/forcing you to use a type alias for an Option<Box<dyn Fn>> is a bit too annoying I would argue.

YangKeao commented 2 years ago

@viglia Please fix the DCO (by git commit --amend -s), and rebase with the newest master (which fixes the clippy), then we can merge this PR :smiley: