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

Add Process disk usage (bytes read/written) #248

Closed
wants to merge 12 commits into from

Conversation

bvaisvil
Copy link
Contributor

@bvaisvil bvaisvil commented Jan 28, 2020

This PR adds two new fields to process, read_bytes and written_bytes and accessor methods for ProcessExt by the same name for MacOS, Linux, and Windows. In addition, I've added a test for the new functionality.

Please note: For windows, this adds a new dependency with winrt. This was the only way I could find to get the information.

tests/process.rs Outdated
system.refresh_processes();
let process_list = system.get_process_list();
let mut write_bytes: u64 = 0;
for p in process_list.values(){
Copy link
Owner

@GuillaumeGomez GuillaumeGomez Jan 28, 2020

Choose a reason for hiding this comment

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

In here, if any process wrote any data, it'll be > 0. I assume this is not what you want to test. So instead, just get the current pid and do:

let p = system.get_process(sysinfo::get_current_pid().expect("failed to get current pid")).expect("no such process");
assert!(p.written_bytes() > 0);

pub(crate) fn get_disk_usage(p: &mut Process){
let r = ProcessDiagnosticInfo::try_get_for_process_id(p.pid as u32).ok();
if r.is_some(){
let r = r.unwrap();
Copy link
Owner

@GuillaumeGomez GuillaumeGomez Jan 28, 2020

Choose a reason for hiding this comment

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

Please never use unwrap. If needed, create an inner function expecting a result and then chain ? calls.

Copy link
Contributor Author

@bvaisvil bvaisvil Jan 30, 2020

Choose a reason for hiding this comment

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

sure, I can do that. Thought if I converted it to an option and then check if it was Some that unwrap won't cause a panic, perhaps I am wrong about that.

Copy link
Owner

@GuillaumeGomez GuillaumeGomez Jan 30, 2020

Choose a reason for hiding this comment

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

In that case, you can use if let.

@GuillaumeGomez
Copy link
Owner

@GuillaumeGomez GuillaumeGomez commented Jan 28, 2020

Thanks for the PR! This looks promising. A few things though: please run cargo fmt and your code. Please add the results from cargo bench on each platform before and after these changes (I'm very worried about windows performance after looking at the modifications).

@bvaisvil
Copy link
Contributor Author

@bvaisvil bvaisvil commented Jan 30, 2020

Benches for Linux and Mac seem okay.
However, the windows bench is vastly different. So you're right, adding the winrt call just kills performance. I'm going to look into another way to get the disk information for processes on windows.

mac before

test bench_new                  ... bench:   9,966,930 ns/iter (+/- 832,204)
test bench_refresh_all          ... bench:   3,468,708 ns/iter (+/- 345,394)
test bench_refresh_cpu          ... bench:      12,173 ns/iter (+/- 2,850)
test bench_refresh_disk_lists   ... bench:      55,104 ns/iter (+/- 17,001)
test bench_refresh_disks        ... bench:       1,027 ns/iter (+/- 179)
test bench_refresh_memory       ... bench:       3,698 ns/iter (+/- 1,174)
test bench_refresh_network      ... bench:     166,235 ns/iter (+/- 24,084)
test bench_refresh_process      ... bench:       4,473 ns/iter (+/- 778)
test bench_refresh_processes    ... bench:   1,300,710 ns/iter (+/- 244,148)
test bench_refresh_system       ... bench:   1,135,419 ns/iter (+/- 414,902)
test bench_refresh_temperatures ... bench:   1,129,658 ns/iter (+/- 321,340)

mac after

test bench_new                  ... bench:  10,214,816 ns/iter (+/- 1,776,555)
test bench_refresh_all          ... bench:   3,663,618 ns/iter (+/- 491,591)
test bench_refresh_cpu          ... bench:      12,838 ns/iter (+/- 1,808)
test bench_refresh_disk_lists   ... bench:      53,989 ns/iter (+/- 10,163)
test bench_refresh_disks        ... bench:       1,076 ns/iter (+/- 297)
test bench_refresh_memory       ... bench:       4,019 ns/iter (+/- 347)
test bench_refresh_network      ... bench:     167,138 ns/iter (+/- 30,022)
test bench_refresh_process      ... bench:       6,745 ns/iter (+/- 683)
test bench_refresh_processes    ... bench:   1,363,636 ns/iter (+/- 259,613)
test bench_refresh_system       ... bench:   1,400,767 ns/iter (+/- 403,694)
test bench_refresh_temperatures ... bench:   1,120,545 ns/iter (+/- 354,143)

Linux Before

test bench_new                  ... bench:   1,933,482 ns/iter (+/- 176,542)
test bench_refresh_all          ... bench:   1,215,275 ns/iter (+/- 235,559)
test bench_refresh_cpu          ... bench:      75,312 ns/iter (+/- 44,793)
test bench_refresh_disk_lists   ... bench:      32,832 ns/iter (+/- 828)
test bench_refresh_disks        ... bench:           2 ns/iter (+/- 0)
test bench_refresh_memory       ... bench:      39,376 ns/iter (+/- 30,142)
test bench_refresh_network      ... bench:      65,792 ns/iter (+/- 789)
test bench_refresh_process      ... bench:     101,482 ns/iter (+/- 39,634)
test bench_refresh_processes    ... bench:     784,978 ns/iter (+/- 43,152)
test bench_refresh_system       ... bench:     104,575 ns/iter (+/- 39,840)
test bench_refresh_temperatures ... bench:           2 ns/iter (+/- 0)

Linux After

test bench_new                  ... bench:   2,040,567 ns/iter (+/- 200,973)
test bench_refresh_all          ... bench:   1,315,070 ns/iter (+/- 89,428)
test bench_refresh_cpu          ... bench:      68,797 ns/iter (+/- 25,474)
test bench_refresh_disk_lists   ... bench:      33,279 ns/iter (+/- 1,359)
test bench_refresh_disks        ... bench:           2 ns/iter (+/- 0)
test bench_refresh_memory       ... bench:      38,745 ns/iter (+/- 10,128)
test bench_refresh_network      ... bench:      65,643 ns/iter (+/- 982)
test bench_refresh_process      ... bench:     386,920 ns/iter (+/- 37,392)
test bench_refresh_processes    ... bench:     935,717 ns/iter (+/- 54,157)
test bench_refresh_system       ... bench:     107,758 ns/iter (+/- 69,638)
test bench_refresh_temperatures ... bench:           2 ns/iter (+/- 0)

Windows Before:

test bench_new                  ... bench:  66,243,970 ns/iter (+/- 232,121,065)
test bench_refresh_all          ... bench:  10,690,560 ns/iter (+/- 998,126)
test bench_refresh_cpu          ... bench:         776 ns/iter (+/- 8)
test bench_refresh_disk_lists   ... bench:     155,747 ns/iter (+/- 5,528)
test bench_refresh_disks        ... bench:      60,459 ns/iter (+/- 11,847)
test bench_refresh_memory       ... bench:         796 ns/iter (+/- 7)
test bench_refresh_network      ... bench:         206 ns/iter (+/- 12)
test bench_refresh_process      ... bench:       1,543 ns/iter (+/- 17)
test bench_refresh_processes    ... bench:  10,552,740 ns/iter (+/- 916,618)
test bench_refresh_system       ... bench:       1,604 ns/iter (+/- 15)
test bench_refresh_temperatures ... bench:           2 ns/iter (+/- 0)

test result: ok. 0 passed; 0 failed; 0 ignored; 11 measured; 0 filtered out

And After:

test bench_new                  ... bench: 860,878,860 ns/iter (+/- 1,116,243,809)
test bench_refresh_all          ... bench: 527,567,100 ns/iter (+/- 72,235,947)
test bench_refresh_cpu          ... bench:         792 ns/iter (+/- 13)
test bench_refresh_disk_lists   ... bench:     161,829 ns/iter (+/- 11,966)
test bench_refresh_disks        ... bench:      57,842 ns/iter (+/- 2,782)
test bench_refresh_memory       ... bench:         811 ns/iter (+/- 20)
test bench_refresh_network      ... bench:         224 ns/iter (+/- 39)
test bench_refresh_process      ... bench:          38 ns/iter (+/- 1)
test bench_refresh_processes    ... bench: 502,094,830 ns/iter (+/- 44,655,170)
test bench_refresh_system       ... bench:       1,614 ns/iter (+/- 110)
test bench_refresh_temperatures ... bench:           2 ns/iter (+/- 0)

}
let diag_info = ProcessDiagnosticInfo::try_get_for_process_id(p.pid as u32).ok();
match diag_info{
Some(diag_info) => match diag_info{
Copy link
Owner

@GuillaumeGomez GuillaumeGomez Jan 30, 2020

Choose a reason for hiding this comment

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

Isn't of this match cascade, you can add a new macro:

macro_rules! safe_unwrap {
    ($x:expr) => {
        match $x {
             Some(x) => x,
             None => return,
        }
    }

And you can use it like this:

let diag_info = safe_unwrap!(diag_info.get_disk_usage().ok());

src/sysinfo.rs Outdated
@@ -44,7 +44,7 @@
#![crate_name = "sysinfo"]
#![crate_type = "lib"]
#![crate_type = "rlib"]
#![deny(missing_docs)]
#![warn(missing_docs)]
Copy link
Owner

@GuillaumeGomez GuillaumeGomez Jan 30, 2020

Choose a reason for hiding this comment

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

Please don't change this one.

@bvaisvil
Copy link
Contributor Author

@bvaisvil bvaisvil commented Jan 31, 2020

Latest commits restore the deny for missing_docs. Thanks for the macro suggestion. I used it to refactor get_disk_usage. I wasn't sure where I should place the macros, they're in process.rs, but let me know if you'd like them moved elsewhere.

@GuillaumeGomez
Copy link
Owner

@GuillaumeGomez GuillaumeGomez commented Jan 31, 2020

I wasn't sure where I should place the macros, they're in process.rs, but let me know if you'd like them moved elsewhere.

Put them in the same file where they're used. :) (unless they're used in multiple files, then create a macro.rs file)

@bvaisvil
Copy link
Contributor Author

@bvaisvil bvaisvil commented Feb 9, 2020

I looked into using a WMI query. I can't find a query that will give the number of bytes written or read by a process. Win32_PerfRawData_PerfProc_Process seems like the closest fit with IOWriteBytesPerSec. However, I believe that includes writing to/from network. It isn't clear how this value is computed.

I also looked into having the Process struct hold onto the ComPtr to the ProcessDiagInfo, but it does not implement Send/Sync. So that can't work.

I'm open to any ideas ...

@GuillaumeGomez
Copy link
Owner

@GuillaumeGomez GuillaumeGomez commented Feb 9, 2020

I also looked into having the Process struct hold onto the ComPtr to the ProcessDiagInfo, but it does not implement Send/Sync. So that can't work.

You can wrap the field into an Arc<Mutex<>> and use the field directly in the worst case. I'm not much of a Windows developer myself so I'll need to check what APIs are available to do so and it might take a while (so don't worry if you don't hear anything from me for a few weeks).

@GuillaumeGomez
Copy link
Owner

@GuillaumeGomez GuillaumeGomez commented Apr 5, 2020

I took over this PR in #287. I kept your commits but rebased and fixed the few things that were not smooth enough. Again: thanks a lot for starting this! :)

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