Open Jardynq opened 2 years ago
Good! Please fix the CI and sign-off. (You could sign off a commit through git commit -s
, or add a sign-off for previous commit with git commit -s --amend
)
Thanks for your contribution again. You've done a really good job :beers: . I was also thinking about adding more sampling methods (like add sampling methods based on perf_event_open
for Linux), and this PR also helps it a lot :heart: .
I'm not sure what to do with ReportTiming since it currently just stores some info. I've made a trait for Timer instead that has a start and stop. I have simply created a trait for each "thing" that makes sense should be implemented per platform. Then each platform module impl this onto Profiler, Timer, etc. To see the basic idea for the traits look at the different files under windows_impl/
let me know if anything should change.
I run the unit tests and examples on macos aarch64. Everything works fine as usual under both the dwarf and fp modes.
There are some unimportant warnings about unused functions in tests. I can fix them for you later.
I also tested
pprof = { git = "https://github.com/Jardynq/pprof-rs", branch = "seperate-platform-impl", features = ["flamegraph"] }
as a dependency with our project and it compiles just fine. The target was Windows
.
Didn't have time to actually generate graph, but think it shall be fine as well. Will try tomorrow.
Ah... actually flamegraphs functionality is still unimplemented on win :) Any plans on when may it appear?
@maksymsur I have been working on a simple implementation for windows. You can try it with:
pprof = { git = "https://github.com/Jardynq/pprof-rs/", branch = "windows-support", features = [
"flamegraph",
] }
Though it's probably going to be a while before an actual release with windows support. If you get a stack overflow while building a report, then try running in release or switching to a stable toolchain. Related issue: rust-lang/rust#99693
Hi @maksymsur, @Jardynq is this now pending on Windows support for flamegraph? Would it be possible to move this along before that's fully resolved to get a fix for #151 ?
Hi @maksymsur, @Jardynq, is there anything I can do to help this move along?
I would also be interested in helping if someone could explain me the basics :)
Hi @dveeden @AntoniosBarotsis sorry for late reply! I think @YangKeao & @Jardynq are more appropriate persons to follow up with this issue. Personally me shifted to a Linux distro while working on my project as windows blank implementation wasn't smth I could consume. But it was enough to pass all tests without errors at least.
Hi @YangKeao, @Jardynq any updates on this? Anything I can do to help?
Separated platform specific implementation into separate files. This is needed for windows support. Though the windows implementation is currently blank.
It compiles and runs on windows and linux, but I haven't tested macos.