939385 - Report vsizeMaxContiguous in about:memory and telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 939137 added nsIMemoryReporter::largestContiguousMemory.  We should write a memory reporter for it so it shows up in about:memory, and also report it via telemtry if it's not too expensive.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Rename largestContiguousVMBlock as vsizeMaxContiguous.

Apologies for the bike-shedding, but I'd really like "vsize" and
"vsize-max-contiguous" to be next to each other in about:memory.

Shu, this will affect your patch where you move the test code around.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Expose vsizeMaxContiguous to telemetry.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Expose vsizeMaxContiguous to about:memory.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Rename some distinguished amount functions.

When I introduced "distinguished amounts" in the memory reporter manager, I
used a naming convention of FooBarDistinguishedAmount() for the function that
computes the |fooBar| value.  But I forget to change GetVsize(), GetResident(),
and GetResidentFast().  This patch renames them.

Comment 5

[Nicholas Nethercote [inactive]]

> (part 2) - Expose vsizeMaxContiguous to telemetry.

Oh, do we know that this measurement is reliably fast?  If not, we should not do this.  We've had problems in the past with slow memory measurements done by telemetry.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> > (part 2) - Expose vsizeMaxContiguous to telemetry.
> 
> Oh, do we know that this measurement is reliably fast?  If not, we should
> not do this.  We've had problems in the past with slow memory measurements
> done by telemetry.

It certainly appears to be pretty fast.  The worst case is when you have a pattern of virtual memory pages that look like this: ...free,allocated,free,allocated,....  Then you're making a separate VirtualQuery call for every single page.

Given that jemalloc VirtualAllocs much bigger regions, I think we are unlikely to hit anything close to the worst case here.

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a06dd3421421
https://hg.mozilla.org/integration/mozilla-inbound/rev/a045549217e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/65048ac4ae6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/027fced104dc

Comment 8

[Carsten Book [:Tomcat]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a06dd3421421
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a045549217e2
> https://hg.mozilla.org/integration/mozilla-inbound/rev/65048ac4ae6f
> https://hg.mozilla.org/integration/mozilla-inbound/rev/027fced104dc

Hi Nick, sorry had to backout this changes in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5ef410bdf303 because of perma failures/oranges in Linux and OSX Debug Builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=30755796&tree=Mozilla-Inbound

Comment 9

[Nicholas Nethercote [inactive]]

Betcha $5 they were false positives due to lack of clobbering.

I see these lines:

02:19:57     INFO -  ASSERT: telemetry accessed an unknown distinguished amount
02:19:57     INFO -  Stack Trace:
02:19:57     INFO -  0:h(MEMORY_VSIZE_MAX_CONTIGUOUS,0,vsizeMaxContiguous)

which indicate that the changes to nsIMemoryReporter.idl weren't processed somehow.  Admittedly, I did forget to update a UUID that I should have.  Perhaps that was the problem?

I'll do a try run.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Betcha $5 they were false positives due to lack of clobbering.
> 
> I see these lines:
> 
> 02:19:57     INFO -  ASSERT: telemetry accessed an unknown distinguished
> amount
> 02:19:57     INFO -  Stack Trace:
> 02:19:57     INFO -  0:h(MEMORY_VSIZE_MAX_CONTIGUOUS,0,vsizeMaxContiguous)
> 
> which indicate that the changes to nsIMemoryReporter.idl weren't processed
> somehow.  Admittedly, I did forget to update a UUID that I should have. 
> Perhaps that was the problem?

Lack of UUID updating always causes weird problems like that.  My fault too for not catching that.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Betcha $5 they were false positives due to lack of clobbering.
 
Yep.  Try run was green:
https://tbpl.mozilla.org/?tree=Try&rev=e0b35fa20620

Comment 12

[Nicholas Nethercote [inactive]]

Re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f3c8ef908f
https://hg.mozilla.org/integration/mozilla-inbound/rev/45dd270fa5e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a636fa7bc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc781a9dd992

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Test vsizeMaxContiguous.

Comment 14

[Nicholas Nethercote [inactive]]

Part 5:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e2cb0a838d

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8f3c8ef908f
https://hg.mozilla.org/mozilla-central/rev/45dd270fa5e4
https://hg.mozilla.org/mozilla-central/rev/d6a636fa7bc6
https://hg.mozilla.org/mozilla-central/rev/bc781a9dd992
https://hg.mozilla.org/mozilla-central/rev/d9e2cb0a838d

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: Omnibus patch for bug 939137 and bug 939385

This patch combines all three patches from bug 939137 (which introduced this measurement and made mochitests use it) and this bug (which renamed it and exposed it to about:memory and telemetry).

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8338235 [details] [diff] [review]
Omnibus patch for bug 939137 and bug 939385

[Approval Request Comment]

Bug caused by (feature/regressing bug #): N/A;  this is a new feature.

User impact if declined: this measurement might provide important insight into our OOMs on Windows.

Testing completed (on m-c, etc.): Been on m-c for over a week, working well.

Risk to taking this patch (and alternatives if risky): The main risk is that this measurement is performed on every telemetry ping.  If it's sometimes slow -- and froydnj thinks it shouldn't be -- that would hurt users with telemetry enabled.

String or IDL/UUID changes made by this patch: nsIMemoryReporterManager changed (from 4db7040a-16f9-4249-879b-fe72729c7ef5 to b8fbab52-cdc0-424d-ab18-a5d9fc4b98c8) due to the addition of the read-only attribute |vsizeMaxContiguous|.

Comment 18

[Shu-yu Guo [:shu]]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> Created attachment 8338235 [details] [diff] [review]
> Omnibus patch for bug 939137 and bug 939385
> 
> This patch combines all three patches from bug 939137 (which introduced this
> measurement and made mochitests use it) and this bug (which renamed it and
> exposed it to about:memory and telemetry).

Do you perhaps want to roll up bug 939414 as well?

Comment 19

[Nicholas Nethercote [inactive]]

> Do you perhaps want to roll up bug 939414 as well?

Having it in about:memory and telemetry seems more important to me than having it in the test runs, though others may disagree.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8338235 [details] [diff] [review]
Omnibus patch for bug 939137 and bug 939385

With only one beta left before ship that's not enough bake time to test this potential slow down for telemetry users with all the pinging so let's only take this to FF27 and see how it fares on an entire Beta cycle.

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 8338235 [details] [diff] [review]
Omnibus patch for bug 939137 and bug 939385

Fair enough!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d364517977a7

Comment 23

[svbabukumar]

[bugday-20140122] Could you please add the exact STR(Steps to Reproduce) to check this  bug

Comment 24

[Nicholas Nethercote [inactive]]

> [bugday-20140122] Could you please add the exact STR(Steps to Reproduce) to
> check this  bug

This wasn't a defect, but the addition of a new feature, so it's a strange choice to verify.

Nonetheless, here are steps to check it's working.

- First, it only works on Windows.

- Open about:memory, look for the presence of a "vsize-max-contiguous" measurement. It should be near the bottom.

- After the browser has been running for a few minutes, open about:telemtry, and click on the "Histograms" section. There should be a MEMORY_VSIZE_MAX_CONTIGUOUS histogram, and the measurements in it should be similar to the measurement you saw in about:memory.