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
Merged

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jun 26, 2014

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

@hoppipolla-critic-bot hoppipolla-critic-bot commented Jun 26, 2014

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

@highfive highfive commented Jun 26, 2014

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

@pcwalton pcwalton commented Jun 26, 2014

Sweet! r=me with nits addressed.

@nnethercote
Copy link
Contributor Author

@nnethercote nnethercote commented Jun 26, 2014

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

@larsbergstrom larsbergstrom commented Jun 26, 2014

@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

@larsbergstrom larsbergstrom commented Jun 26, 2014

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;

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

@nnethercote nnethercote commented Jun 26, 2014

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

@pcwalton

This comment has been minimized.

Copy link

@pcwalton pcwalton commented on 56dd5b9 Jun 26, 2014

r+

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
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@nnethercote nnethercote deleted the nnethercote:memprof branch Mar 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.