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 a basic memory profiler, invoked with -m. #2720

Merged
merged 2 commits into from
Jun 27, 2014

Conversation

nnethercote
Copy link
Contributor

This adds a basic memory profiler. It's invoked with the -m option. Sample
output:

category : size (MiB)
vsize : 1355.47
resident : 50.85

It currently only works on Linux. On other platforms, "???" will be printed
instead.

I chose to make the memory profiler entirely separate from the existing
profiler, as opposed to augmenting it. They are/will be different enough beasts
that this seemed the right thing to do. But it did result in a bit of copy &
pasting. And if you run with both -p and -m, the output can be interleaved,
which isn't ideal.

Something I didn't do, which might make sense, is to rename Profiler (and all
related names) as TimeProfiler. That name would nicely match the |time| module
name.

The option_try! macro might be a useful thing to add to Rust's std::macros.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/1908

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

@pcwalton
Copy link
Contributor

Sweet! r=me with nits addressed.

@nnethercote
Copy link
Contributor Author

I addressed the minor nits except for the |Instrumentation| suggestion, because I couldn't work out how to create a new module, and it also felt like overkill. I also added a second patch that renames Profiler as TimeProfiler.

pcwalton, can you please re-review?

The Mac Travis CI run failed, but it looked like an infrastructure problem?

@larsbergstrom
Copy link
Contributor

@nnethercote I'm looking into the travisci failure - it appears to be a network timeout installing autoconf213, which is required by our ancient version of SpiderMonkey.

@larsbergstrom
Copy link
Contributor

On TravisCI, I get:

/libmacros.dummy

/Users/travis/build/mozilla/servo/src/components/util/memory.rs:8:5: 8:18 error: unused import [-D unused-imports]

/Users/travis/build/mozilla/servo/src/components/util/memory.rs:8 use std::io::File;

^~~~~~~~~~~~~

/Users/travis/build/mozilla/servo/src/components/util/memory.rs:9:5: 9:23 error: unused import [-D unused-imports]

/Users/travis/build/mozilla/servo/src/components/util/memory.rs:9 use std::os::page_size;

And likewise for |ProfilerChan|, |profiler_chan|, and so on.  This
contrasts nicely with the newly added |MemoryProfiler|.
@nnethercote
Copy link
Contributor Author

I fixed the Mac-only travis errors and rebased. Should be good for re-review now.

nnethercote added a commit that referenced this pull request Jun 27, 2014
Add a basic memory profiler, invoked with -m. r=pcwalton.
@nnethercote nnethercote merged commit 491cc03 into servo:master Jun 27, 2014
@nnethercote nnethercote deleted the memprof branch March 24, 2015 11:22
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.

5 participants