664486 - Add page fault count to about:memory on Linux and Mac

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

As part of bug 664291 (and just in general), it would be helpful to report the page fault count in about:memory.

This is pretty simple to do on *NIX, and appears to be tricky on Windows.

Comment 1

[Justin Lebar (not reading bugmail)]

njn, do you think we should report both soft and hard page faults, or just hard faults?

Comment 2

[Nicholas Nethercote [inactive]]

Hard faults are more important.  But if you're getting one it's probably easy to get the other.  I'm not that fussed either way.

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: WIP v1 - Works on Linux and Mac.

Comment 4

[(dormant account)]

Is there any value in measuring soft faults?

Comment 5

[Justin Lebar (not reading bugmail)]

(In reply to comment #4)
> Is there any value in measuring soft faults?

I can't think of any, but it's trivial to do, and I figure if we don't add it, there will come some point when we have a bug which would be easy to fix, if only about:memory had reported soft faults.  :)

Comment 6

[Justin Lebar (not reading bugmail)]

sfink, do you have any pointers as to how to do this on Windows with ETW?  I'm kind of lost in the morass that is the WinAPI...

Comment 7

[Steve Fink [:sfink] [:s:]]

Mike Hommey has attempted this in the past, but didn't find anything satisfactory: http://glandium.org/blog/?p=1963

I think it *should* be possible, since the kernel event provider is always running and collects some basic stats, so needing history isn't a problem. I'd be surprised if hard page faults weren't in those stats.

It might require admin privileges to access, though. But if so, there's probably some unprivileged command/tool/library that is at least allowed to see it, though perhaps not dump it out in a usable form. (I'd have to spin up a Windows VM to try to figure it out.)

CC'ing Kevin Gadd, since he has foolishly revealed Windows expertise.

Comment 8

[Justin Lebar (not reading bugmail)]

Some amount of documentation is here:

http://msdn.microsoft.com/en-us/library/bb968803%28v=VS.85%29.aspx

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 539826 [details] [diff] [review]
WIP v1 - Works on Linux and Mac.

Review of attachment 539826 [details] [diff] [review]:
-----------------------------------------------------------------

You'll need to modify toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul as well.  From a quick look through, everything else about this patch looks pretty good.

I'm wondering what the use case for these counts is.  I guess if a user feels like Firefox is really crawling, they can consult the page fault count?  The hard part is knowing what a normal value is.  I guess if you look at about:memory multiple times you'd get a sense for your particular machine and browsing habits.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +794,5 @@
>        };
>        rArray.push(elem);
> +      var thisAmntLength = formatReporterUsage(elem).length;
> +      if (thisAmntLength > maxAmntLength) {
> +        maxAmntLength = thisAmntLength;

Saving two chars doesn't seem worth it... I'd use "Amount" instead of "Amnt".

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +193,5 @@
> +NS_MEMORY_REPORTER_IMPLEMENT(SoftPageFaults,
> +    "soft page faults",
> +    MR_COUNT,
> +    "Also known as a \"minor page fault\", a soft page fault occurs when a "
> +    "process tries to access a page which is present in physical memory but is "

I wonder if it's worth augmenting the description with some stuff about how to interpret the number.  Eg. that it's a function of how much memory Firefox is using and how much physical RAM the machine has, and that the number is best used as a relative measure against itself at other times.

The descriptions for "vsize" and "resident" might be worth looking at, they have some info about how useful they are as metrics, and how they are best interpreted.

Comment 10

[Justin Lebar (not reading bugmail)]

> I'm wondering what the use case for these counts is.  I guess if a user feels 
> like Firefox is really crawling, they can consult the page fault count?  The 
> hard part is knowing what a normal value is.

In my very limited testing, I saw fewer than 10 faults after browsing around for a while.  But as soon as I started paging, I saw tens of thousands.

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.1

s/Amnt/Amount

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 541398 [details] [diff] [review]
Patch v1.1

Review of attachment 541398 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is very clean and good overall.  But it has one problem -- adding MR_COUNT to handle non-byte reporters is a hack.  The right way to do this, which I'd like you to do, is to add a new "unit" field to nsIMemoryReporter, rather than adding MR_COUNT.  (I'm sorry I didn't think of this when I gave you feedback earlier!)

This new field would be a PRInt32 faking an enum like this:

  const PRInt32 MR_BYTES = 0;
  const PRInt32 MR_COUNTS = 1;
  const PRInt32 MR_COUNTS_PER_SECOND = 2;
  // This list can grow indefinitely. Just don't ever change an existing item.

"explicit" reporters can only sensibly use MR_BYTES, and we could check for that if we felt inclined.

This would allow things like counts of page faults per second to be reported cleanly, if we wanted, and anything else we can think of in the future.

BTW, I'll save you some trouble:  as part of this, you'll have to modify struct MemoryReport in dom/ipc/PMemoryReportRequest.ipdl.  That one took me a while to find the first time I tried something similar :)

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +153,5 @@
>    //     interface Reporter {
>    //       _path:        string;
>    //       _kind:        number;
>    //       _description: string;
> +  //       _amount:  number;

Make indentation consistent.

@@ +172,5 @@
>      var r = {
>        _path:        rOrig.path,
>        _kind:        rOrig.kind,
>        _description: rOrig.description,
> +      _amount:  rOrig.amount

Indentation again.

@@ +774,5 @@
>  {
>    // Generate an array of Reporter-like elements, stripping out all the
>    // reporters that have already been handled.  Also find the width of the
>    // widest element, so we can format things nicely.
> +  var maxAmntLength = 0;

s/Amnt/Amount/, here and everywhere else.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +192,5 @@
> +
> +NS_MEMORY_REPORTER_IMPLEMENT(SoftPageFaults,
> +    "soft-page-faults",
> +    MR_COUNT,
> +    "Also known as a \"minor page fault\", a soft page fault occurs when a "

I'd add "The number of soft page faults that have occurred during the lifetime of the process." at the start.

@@ +198,5 @@
> +    "not mapped into the process's address space.  For instance, a process might "
> +    "observe soft page faults when it loads a shared library which is already "
> +    "present in physical memory. A large number in this field does not "
> +    "necessarily indicate that the process is trying to use more memory than you "
> +    "have available.",

The last sentence reads a little strangely.  How about:  "For this reason, 'hard-page-faults' is a better indicator of whether the process is using more memory than you have available."

@@ +205,5 @@
> +
> +NS_MEMORY_REPORTER_IMPLEMENT(HardPageFaults,
> +    "hard-page-faults",
> +    MR_COUNT,
> +    "Also known as a \"major page fault\", a hard page fault occurs when a "

I'd add "The number of hard page faults that have occurred during the lifetime of the process." at the start.

@@ +210,5 @@
> +    "process tries to access a page which is not present in physical memory. "
> +    "The operating system must access the disk in order to fulfill a hard page "
> +    "fault. When memory is plentiful, you should see very few hard page faults. "
> +    "But if the process tries to use more memory than your machine has available, "
> +    "you may see many thousands of hard page faults.",

Add something like "This can greatly degrade performance." or similar at the end.

@@ +396,5 @@
>  
>  #if defined(XP_LINUX) || defined(XP_MACOSX)
>      REGISTER(Vsize);
> +    REGISTER(SoftPageFaults);
> +    REGISTER(HardPageFaults);

Is it worth doing hard page faults per second on Windows?  Not sure.

Comment 14

[Justin Lebar (not reading bugmail)]

Thanks for the comments!  I'll get to work on fixing it up.

> Is it worth doing hard page faults per second on Windows?  Not sure.

I can only get hard page faults per second for the whole system on Windows.  (AFAICT, you can get soft+hard PF per process or aggregated hard PF for the system, but not hard PF per process.)

That doesn't seem to belong in about:memory to me; what do you think?

Comment 15

[Justin Lebar (not reading bugmail)]

>> +    "not mapped into the process's address space.  For instance, a process might "
>> +    "observe soft page faults when it loads a shared library which is already "
>> +    "present in physical memory. A large number in this field does not "
>> +    "necessarily indicate that the process is trying to use more memory than you "
>> +    "have available.",

> The last sentence reads a little strangely.  How about:  "For this reason, 
> 'hard-page-faults' is a better indicator of whether the process is using more 
> memory than you have available."

That implies that soft page faults might be somehow correlated with using more memory than is available (i.e. hard faults are a better indicator, but are soft faults still decent?), and I'm not sure that's true.

I feel like we should indicate what the number is good for, rather than what it's not good for.  But I honestly don't know what it's good for; I just figure it doesn't hurt to have it there.  How about

  A process may experience many thousands of soft page faults even when the machine has plenty of available physical memory, and because the OS services a soft page fault without accessing the disk, they impact performance much less than hard page faults.

?

(In other news, I like that Firefox contains a lesson on systems vocabulary, but some of these are becoming a bit long for tooltips...)

Comment 16

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 17

[Nicholas Nethercote [inactive]]

(In reply to comment #14)
> 
> I can only get hard page faults per second for the whole system on Windows. 
> (AFAICT, you can get soft+hard PF per process or aggregated hard PF for the
> system, but not hard PF per process.)
> 
> That doesn't seem to belong in about:memory to me; what do you think?

Oh right!  No, it doesn't belong.

> How about
> 
>   A process may experience many thousands of soft page faults even when the
> machine has plenty of available physical memory, and because the OS services
> a soft page fault without accessing the disk, they impact performance much
> less than hard page faults.

Sounds good!

> (In other news, I like that Firefox contains a lesson on systems vocabulary,
> but some of these are becoming a bit long for tooltips...)

Eh, plenty of them are already very long.   A pet peeve of mine is that people don't know what all these measurements mean, so I'm trying to remedy that :)

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 541781 [details] [diff] [review]
Patch v2

Review of attachment 541781 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nits below addressed.  Thanks for the code clean-ups, like making the field ordering consistent and getting rid of the void* argument to GetAmount().

This also needs a super-review;  bz recently did one for me that involved memory reporters.

You should add the "dev-doc-needed" keyword once this lands.

It'd be good if you could land this quickly;  I have a patch in bug 666075 that'll have some conflicts, which I'm happy to resolve, but the sooner I can do that the better!

::: gfx/harfbuzz/src/hb-icu.c
@@ +68,5 @@
>    case U_UPPERCASE_LETTER:		return HB_CATEGORY_UPPERCASE_LETTER;	/* Lu */
>    case U_LOWERCASE_LETTER:		return HB_CATEGORY_LOWERCASE_LETTER;	/* Ll */
>    case U_TITLECASE_LETTER:		return HB_CATEGORY_TITLECASE_LETTER;	/* Lt */
>    case U_MODIFIER_LETTER:		return HB_CATEGORY_MODIFIER_LETTER;	/* Lm */
> +  case U_OTHER_LETTER:			return HB_KIND_OTHER_LETTER;	/* Lo */

You have a bunch of harfbuzz changes in here.  Is that an accident?  They don't seem to belong.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +236,5 @@
>    content.appendChild(div);
>  }
>  
> +// Compare two memory reporter nodes.  Sort by amount, but put nodes with units
> +// UNITS_COUNT below all other nodes.

Can you future-proof this comment by saying something like "measurements of a single unit type are kept together in a group, and the groups are ordered according to the UNITS_* values".

@@ +239,5 @@
> +// Compare two memory reporter nodes.  Sort by amount, but put nodes with units
> +// UNITS_COUNT below all other nodes.
> +function cmp_amount(a, b)
> +{
> +  if ((a._units == UNITS_COUNT) == (b._units == UNITS_COUNT))

And then this function could instead work like this:
- if the units aren't equal, compare the _units field
- otherwise, compare the _amount field

That way it wouldn't need changing if more units are added later.

@@ +267,5 @@
>     *         interface Node {
>     *           _name: string;
>     *           _kind:        number;
>     *           _description: string;
> +   *           _amount:      number;    (non-negative or 'kUnknown')

You made the field order consistent everywhere else (eg. description comes after amount), might as well do it here too :)

@@ +486,5 @@
> + * Returns the amount used by a reporter as a human-readable string.
> + *
> + * @param aReporter
> + *        The reporter whose usage we're formatting
> + * @return The reporter's usage formatted as a human-readable string

Change to "The reporter's amount", and mention that the unit is printed, if appropriate.

@@ +488,5 @@
> + * @param aReporter
> + *        The reporter whose usage we're formatting
> + * @return The reporter's usage formatted as a human-readable string
> + */
> +function formatReporterUsage(aReporter)

formatReporterAmount or just formatAmount would be a better name.

@@ +494,5 @@
> +  if (aReporter._units == UNITS_BYTES) {
> +    return formatBytes(aReporter._amount);
> +  }
> +  else {
> +    return formatInt(aReporter._amount);

I'd be inclined to partially future-proof this against new units being added, like this:

  else if (aReporter._units == UNITS_COUNT) {
    return formatInt(aReporter._amount);
  else
    return "???";

Also, return after else is usually frowned upon.  You could use a switch instead.  Eg. see kindToString() below.

::: xpcom/base/nsIMemoryReporter.idl
@@ +90,5 @@
> +   * There are three categories of memory reporters:
> +   *
> +   *  - MAPPED: memory allocated directly by the OS, e.g. by calling mmap,
> +   *    VirtualAlloc, or vm_allocate.  Reporters in this category must have
> +   *    units UNITS_BYTES.

...and must start with "explicit".  You said it above, doesn't hurt to repeat it here.  Likewise for HEAP.

@@ +110,5 @@
>     */
>    readonly attribute PRInt32 kind;
>  
>    /*
> +   * The amount reported by a memory reporter may have one of two units.

Can you say "the following units." and indicate below that new units can be added as necessary.

@@ +125,5 @@
> +
> +  /*
> +   * The units on the reporter's amount.  See UNITS_* above.
> +   */
> +  readonly attribute PRInt32 units;

This is really picky, but I was imagining this field would come after the amount, because units are normally written that way.  Can you change, here and everywhere else? :)

@@ +178,5 @@
> +      NS_IMETHOD GetProcess(char **process) { *process = strdup(""); return NS_OK; }          \
> +      NS_IMETHOD GetPath(char **memoryPath) { *memoryPath = strdup(_path); return NS_OK; }    \
> +      NS_IMETHOD GetKind(int *kind) { *kind = _kind; return NS_OK; }                          \
> +      NS_IMETHOD GetUnits(int *units) { *units = _units; return NS_OK; }                      \
> +      NS_IMETHOD GetAmount(PRInt64 *amount) { *amount = _usageFunction(); return NS_OK; }     \

Oh, you managed to get rid of the dataptr to the usage function.  Cool.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +216,5 @@
> +    "The operating system must access the disk in order to fulfill a hard page "
> +    "fault. When memory is plentiful, you should see very few hard page faults. "
> +    "But if the process tries to use more memory than your machine has "
> +    "available, you may see many thousands of hard page faults. Because "
> +    "accessing the disk is a million times slower than accessing RAM, this can "

Hmm, too precise;  I'd say "can be up to a million times slower", or "is thousands or even millions times slower".

Comment 19

[Justin Lebar (not reading bugmail)]

> You have a bunch of harfbuzz changes in here.  Is that an accident?

Yes!  That was the result of overzealous sed.  Thanks for catching it.

> This is really picky, but I was imagining this field would come after the 
> amount, because units are normally written that way.  Can you change, here and 
> everywhere else? :)

I think there's an equally compelling reason to use the current order: It groups all the metadata together.  The amount is different from the rest of the fields because it can change every time you read it.

How wed are you to this particular color for the bikeshed?  :)

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to comment #19)
> 
> How wed are you to this particular color for the bikeshed?  :)

Not very.  Don't worry about it :)

Comment 21

[Mike Hommey [:glandium]]

As a side note I'm not sure the various explanations about terminology, while nice and interesting, have their place in C code. They'd probably be better as localizable .properties or .dtd entities.

Comment 22

[Nicholas Nethercote [inactive]]

(In reply to comment #21)
> As a side note I'm not sure the various explanations about terminology,
> while nice and interesting, have their place in C code. They'd probably be
> better as localizable .properties or .dtd entities.

That's bug 649881.  Nobody's very keen to do it.

Comment 23

[Justin Lebar (not reading bugmail)]

Do you think this is too precise?

> Because 
> accessing the disk is up to a million times slower than accessing RAM,
> the program may run very slowly when it is experiencing more than 100 or so
> hard page faults a second.

I didn't like "this can cause the program to run very slowly" -- *what* can cause the program to run slowly?

At 100 faults per second and 5ms to service a fault, you'd be spending half your time waiting for pages to be faulted in, so 100 seems like a reasonable number to posit.

Comment 24

[Justin Lebar (not reading bugmail)]

> indicate below that new units can be added as necessary.

I always feel silly doing this.  Of course new units can be added as necessary, just as any of the other code in the tree can be modified as necessary.  I think it would be worth noting if consumers (e.g. about:memory) were expected to handle new units with no modifications, but that's not the case.

Anyway, it's just a comment; I'm happy to add it there, if you still want.  Do you also want a comment saying new kinds can be added?

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Comment 26

[Justin Lebar (not reading bugmail)]

Comment on attachment 542156 [details] [diff] [review]
Patch v3

This is ready for sr; we can fix up the few outstanding comment issues in parallel.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 542156 [details] [diff] [review]
Patch v3

>+   * The reporter kind.  See CATEGORY_* above.

s/CATEGORY/KIND/

sr=me

Comment 28

[Nicholas Nethercote [inactive]]

(In reply to comment #23)
> Do you think this is too precise?
> 
> > Because 
> > accessing the disk is up to a million times slower than accessing RAM,
> > the program may run very slowly when it is experiencing more than 100 or so
> > hard page faults a second.

That's fine.

> > indicate below that new units can be added as necessary.
> 
> I always feel silly doing this.  Of course new units can be added as
> necessary, just as any of the other code in the tree can be modified as
> necessary.  I think it would be worth noting if consumers (e.g.
> about:memory) were expected to handle new units with no modifications, but
> that's not the case.
> 
> Anyway, it's just a comment; I'm happy to add it there, if you still want. 
> Do you also want a comment saying new kinds can be added?

I figure it's better to over-comment than under-comment.  Something brief is fine, eg "add new units as necessary".

With super-review in, would you mind landing this ASAP?  I have three patches that will need to be reworked as a result of this change, which is not a problem, but it would be good to get it done soon.

Comment 29

[Justin Lebar (not reading bugmail)]

I'll land on inbound first thing in the morning.

Comment 30

[Justin Lebar (not reading bugmail)]

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/012e21b57057

Comment 31

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/012e21b57057

Comment 32

[Nicholas Nethercote [inactive]]

A follow-up to add some testing of the new code:

http://hg.mozilla.org/integration/mozilla-inbound/rev/152c57ad8825

Comment 33

[Nicholas Nethercote [inactive]]

This is dev-doc-needed for the nsIMemoryReporter changes.

Comment 34

[Marco Bonardo [:mak]]

test merge http://hg.mozilla.org/mozilla-central/rev/152c57ad8825

Comment 35

[Eric Shepherd [:sheppy]]

Docs updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporter

Changes mentioned on Firefox 7 for developers.