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

Process memory #16

Merged
merged 3 commits into from
Jan 28, 2016
Merged

Process memory #16

merged 3 commits into from
Jan 28, 2016

Conversation

thijsc
Copy link
Member

@thijsc thijsc commented Nov 28, 2015

Measurements of current and max memory usage for processes. You can get current for all processes. Max seems to be only available for the current process, at least in Linux.

Fixes #12

@thijsc
Copy link
Member Author

thijsc commented Nov 29, 2015

I haven't done this in the coding style of #14 by the way. Will try to convert it and see how I like that :-).

@thijsc
Copy link
Member Author

thijsc commented Nov 29, 2015

I think this is the nicest we can get. It's not possible to combine raw_data and segments because segments is a Vector of references to raw_data. Therefore we do need to keep that variable around for the entire scope of the function.

@thijsc
Copy link
Member Author

thijsc commented Nov 29, 2015

I tried a slightly different approach, something like this:

file_to_string(path).
    and_then(|raw_data| {
        let segments: Vec<&str> = raw_data.split_whitespace().collect();

        if segments.len() < 2 {
            return Err(ProbeError::UnexpectedContent("Incorrect number of segments".to_owned()))
        }

This isn't very ergonomic because the errors within the block are not of the same type as the IO error from file_to_string. We'd have to add try!s around them. I think I now understand why you added these manual error conversions in #14 @timonv.

Overall I feel that a more functional style in the implementation of these functions causes more hassle than the benefit it delivers.

@timonv
Copy link
Contributor

timonv commented Nov 29, 2015

It's a balancing act 🐵


// Value is in pages, needs to be multiplied by the page size to get a value in KB. We ask the OS
// for this information using sysconf.
let pagesize = unsafe { libc::sysconf(libc::_SC_PAGESIZE) } as u64;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but why is it unsafe? Or is just whole libc unsafe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using libc is almost always unsafe since its a C library that doesn't offer the guarantees Rust needs. Therefore we have to check things manually and make sure they are correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

The compiler will emit a warning if you use an unnecessary unsafe block by the way, so we'd know about it if this was unnecessary.

@thijsc thijsc mentioned this pull request Nov 29, 2015
@thijsc
Copy link
Member Author

thijsc commented Dec 2, 2015

Yeah we'll plan that evening after its done, didn't mean to imply that I wanted to impose deadlines :-).

@thijsc
Copy link
Member Author

thijsc commented Dec 2, 2015

Any objections to merging this?

@timonv
Copy link
Contributor

timonv commented Dec 2, 2015

None at all! +1

thijsc added a commit that referenced this pull request Jan 28, 2016
@thijsc thijsc merged commit b4aa3e6 into master Jan 28, 2016
@thijsc thijsc deleted the process_memory branch January 28, 2016 20:35
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.

2 participants