Separate platform implementation #150

Jardynq · 2022-07-23T23:37:59Z

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.

YangKeao · 2022-07-24T15:48:37Z

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)

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. Signed-off-by: Christian Jordan <[email protected]>

Signed-off-by: Christian Jordan <[email protected]>

they were moved into nix_impl/profiler in previous commit Signed-off-by: Christian Jordan <[email protected]>

Signed-off-by: Christian Jordan <[email protected]>

To make clippy happy the .unwrap on write! shouldn't fail Signed-off-by: Christian Jordan <[email protected]>

Signed-off-by: Christian Jordan <[email protected]>

Makes clippy happy Signed-off-by: Christian Jordan <[email protected]>

YangKeao · 2022-07-25T02:06:41Z

src/platform/nix_impl/profiler.rs

+use crate::profiler::PROFILER;
+use crate::{MAX_DEPTH, MAX_THREAD_NAME};
+
+pub fn register() -> Result<()> {


How about impl register in a trait? See /src/backtrace/mod.rs: trait Trace. We'll need to tell the developers which functions are needed to add a new Profiler (for different platform, or implementation)

YangKeao · 2022-07-25T02:09:25Z

src/report.rs

@@ -6,8 +6,8 @@ use std::fmt::{Debug, Formatter};
 use parking_lot::RwLock;

 use crate::frames::{Frames, UnresolvedFrames};
+use crate::platform::timer::ReportTiming;


Also, add a trait for ReportTiming, to tell the developers that it should implement Clone, and has a .get_frequency(), .get_duration() and .get_start_time() 🤔 .

Do the same for validate.

YangKeao · 2022-07-25T02:14:35Z

Thanks for your contribution again. You've done a really good job 🍻 . 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 ❤️ .

+ moved write_thread_name_fallback into profiler, since it's platform agnostic Signed-off-by: Christian Jordan <[email protected]>

Signed-off-by: Christian Jordan <[email protected]>

Jardynq · 2022-07-26T20:06:58Z

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.

YangKeao

LGTM! I'll help you to test (and do some minor modification if I could) on freebsd in a few days to solve #151 .

@sticnarf Could you help me to test on macos?

After testing, I'd approve this PR 😸

sticnarf · 2022-07-27T05:33:36Z

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.

maksymsur · 2022-07-27T19:04:12Z

Cargo.toml

-criterion = {version = "0.3", optional = true}
+criterion = { version = "0.3", optional = true }
+
+[target.'cfg(any(target_os = "linux", target_os = "macos"))'.dependencies]


Perhaps it's better to use more generic and inclusive template unless some other UNIX targets but tested linux and macos may fail

[target.'cfg(target_family = "unix")'.dependencies]

Rest of the code looks good

maksymsur · 2022-07-27T19:05:09Z

src/error.rs

-    NixError(#[from] nix::Error),
+    OsError(i32),
+
+    #[cfg(any(target_os = "linux", target_os = "macos"))]


[target.'cfg(target_family = "unix")'.dependencies]

maksymsur · 2022-07-27T19:06:51Z

src/platform/mod.rs

@@ -0,0 +1,49 @@
+#[cfg(any(target_os = "linux", target_os = "macos"))]


[target.'cfg(target_family = "unix")'.dependencies]

maksymsur · 2022-07-27T19:07:35Z

src/platform/mod.rs

+    pub use backtrace_rs::Trace as TraceImpl;
+}
+
+#[cfg(any(target_os = "linux", target_os = "macos"))]


[target.'cfg(target_family = "unix")'.dependencies]

maksymsur · 2022-07-27T20:34:00Z

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.

maksymsur · 2022-07-28T09:31:13Z

Ah... actually flamegraphs functionality is still unimplemented on win :)
Any plans on when may it appear?

Jardynq · 2022-07-28T15:34:04Z

@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

dveeden · 2022-08-31T07:54:36Z

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 ?

dveeden · 2023-01-05T06:31:15Z

Hi @maksymsur, @Jardynq, is there anything I can do to help this move along?

AntoniosBarotsis · 2023-01-05T09:08:53Z

I would also be interested in helping if someone could explain me the basics :)

maksymsur · 2023-01-19T20:41:38Z

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.

dveeden · 2023-06-27T07:04:51Z

Hi @YangKeao, @Jardynq any updates on this? Anything I can do to help?

Jardynq force-pushed the seperate-platform-impl branch from abdc3d2 to 116668a Compare July 24, 2022 23:42

Jardynq added 6 commits July 25, 2022 01:54

Separated platform specific implementation into separate files

588ea16

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. Signed-off-by: Christian Jordan <[email protected]>

rename from platform_* to *_impl

d6a2d40

Signed-off-by: Christian Jordan <[email protected]>

remove linux tests from profiler

35cbaa2

they were moved into nix_impl/profiler in previous commit Signed-off-by: Christian Jordan <[email protected]>

move platform specific backtrace into ./platform

46f2c30

Signed-off-by: Christian Jordan <[email protected]>

Use fmt::write instead of push_str

45d0a6c

To make clippy happy the .unwrap on write! shouldn't fail Signed-off-by: Christian Jordan <[email protected]>

fix linux tests

20c4ad7

Signed-off-by: Christian Jordan <[email protected]>

Jardynq force-pushed the seperate-platform-impl branch from 116668a to 20c4ad7 Compare July 25, 2022 00:05

Moved trigger_lazy import into tests module

c12b87a

Makes clippy happy Signed-off-by: Christian Jordan <[email protected]>

YangKeao reviewed Jul 25, 2022

View reviewed changes

Moved platform impl into traits

c855009

+ moved write_thread_name_fallback into profiler, since it's platform agnostic Signed-off-by: Christian Jordan <[email protected]>

Jardynq force-pushed the seperate-platform-impl branch from b4cb45d to c855009 Compare July 26, 2022 18:17

fix write_thread_name_fallback and bench addr_validate

e9f8cf7

Signed-off-by: Christian Jordan <[email protected]>

Jardynq force-pushed the seperate-platform-impl branch from 5f75b2f to e9f8cf7 Compare July 26, 2022 19:22

YangKeao mentioned this pull request Jul 27, 2022

Compiling fails on FreeBSD #151

Closed

YangKeao reviewed Jul 27, 2022

View reviewed changes

maksymsur reviewed Jul 27, 2022

View reviewed changes

AntoniosBarotsis mentioned this pull request Sep 29, 2022

feat: add performance investigation AntoniosBarotsis/Rss2Email#12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate platform implementation #150

Separate platform implementation #150

Jardynq commented Jul 23, 2022

YangKeao commented Jul 24, 2022

YangKeao Jul 25, 2022

YangKeao Jul 25, 2022

YangKeao Jul 25, 2022

YangKeao commented Jul 25, 2022

Jardynq commented Jul 26, 2022

YangKeao left a comment

sticnarf commented Jul 27, 2022

maksymsur Jul 27, 2022 •

edited

Loading

maksymsur Jul 27, 2022

maksymsur Jul 27, 2022

maksymsur Jul 27, 2022

maksymsur commented Jul 27, 2022 •

edited

Loading

maksymsur commented Jul 28, 2022 •

edited

Loading

Jardynq commented Jul 28, 2022

dveeden commented Aug 31, 2022

dveeden commented Jan 5, 2023

AntoniosBarotsis commented Jan 5, 2023

maksymsur commented Jan 19, 2023 •

edited

Loading

dveeden commented Jun 27, 2023

		@@ -0,0 +1,49 @@
		#[cfg(any(target_os = "linux", target_os = "macos"))]

Separate platform implementation #150

Are you sure you want to change the base?

Separate platform implementation #150

Conversation

Jardynq commented Jul 23, 2022

YangKeao commented Jul 24, 2022

YangKeao Jul 25, 2022

Choose a reason for hiding this comment

YangKeao Jul 25, 2022

Choose a reason for hiding this comment

YangKeao Jul 25, 2022

Choose a reason for hiding this comment

YangKeao commented Jul 25, 2022

Jardynq commented Jul 26, 2022

YangKeao left a comment

Choose a reason for hiding this comment

sticnarf commented Jul 27, 2022

maksymsur Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

maksymsur Jul 27, 2022

Choose a reason for hiding this comment

maksymsur Jul 27, 2022

Choose a reason for hiding this comment

maksymsur Jul 27, 2022

Choose a reason for hiding this comment

maksymsur commented Jul 27, 2022 • edited Loading

maksymsur commented Jul 28, 2022 • edited Loading

Jardynq commented Jul 28, 2022

dveeden commented Aug 31, 2022

dveeden commented Jan 5, 2023

AntoniosBarotsis commented Jan 5, 2023

maksymsur commented Jan 19, 2023 • edited Loading

dveeden commented Jun 27, 2023

maksymsur Jul 27, 2022 •

edited

Loading

maksymsur commented Jul 27, 2022 •

edited

Loading

maksymsur commented Jul 28, 2022 •

edited

Loading

maksymsur commented Jan 19, 2023 •

edited

Loading