1223927 - Add |resident-unique| measurement to OS X

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

Reporting |resident-unique| (aka USS, private bytes) is needed to determine overall memory usage with multiple content processes on OS X.

It looks like |libtop_update_vm_regions| in the OS X top implementation [1] should be a good starting point.

[1] http://www.opensource.apple.com/source/top/top-100.1.2/libtop.c

Comment 1

[Eric Rahm [:erahm]]

Attachment: Add resident-unique measurement to OS X

Comment 2

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1225acd269e

Comment 3

[Eric Rahm [:erahm]]

Comment on attachment 8686266 [details] [diff] [review]
Add resident-unique measurement to OS X

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

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +575,5 @@
> +        break;
> +    }
> +  }
> +
> +  *aN = privatePages * vm_kernel_page_size;

The build is failing on 10.7 due to |vm_kernel_page_size| not being defined, I guess we'll need to go w/ PAGE_SIZE instead.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8686266 [details] [diff] [review]
Add resident-unique measurement to OS X

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

Seems reasonable, though I haven't seen this API before.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +512,5 @@
> +    default:
> +      return false;
> +  }
> +
> +  return aAddr >= base && aAddr < (base + size);

I find comparisons like this easier to read when written with the repeated value in the middle:

> return base <= aAddr && aAddr < (base + size);

@@ +535,5 @@
> +  mach_vm_size_t size = 0;
> +  for (mach_vm_address_t addr = MACH_VM_MIN_ADDRESS; ; addr += size) {
> +    vm_region_top_info_data_t info;
> +    mach_msg_type_number_t infoCount = VM_REGION_TOP_INFO_COUNT;
> +    mach_port_t object_name;

Nit: |objectName|.

@@ +549,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    if (InSharedRegion(addr, cpu_type) && info.share_mode != SM_PRIVATE) {
> +      if (info.share_mode != SM_PRIVATE) {

You have |info.share_mode != SM_PRIVATE| twice, which is redundant. Is the second occurrence meant to be different?

@@ +557,5 @@
> +
> +    if (info.share_mode == SM_COW && info.ref_count == 1) {
> +      // Treat copy-on-write pages as private if they only have one reference.
> +      info.share_mode = SM_PRIVATE;
> +    }

I think it'd be clearer to move this if-block into the |SM_COW| case in the switch below.

Comment 5

[Eric Rahm [:erahm]]

Thank you for the quick review.

(In reply to Nicholas Nethercote [:njn] from comment #4)
> Comment on attachment 8686266 [details] [diff] [review]
> Add resident-unique measurement to OS X
> 
> Review of attachment 8686266 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems reasonable, though I haven't seen this API before.

Yes, I'm new to it as well. It looks like Chrome is using a similar approach (and referenced libtop as well) so I feel somewhat comfortable with this usage.

> 
> ::: xpcom/base/nsMemoryReporterManager.cpp
> @@ +512,5 @@
> > +    default:
> > +      return false;
> > +  }
> > +
> > +  return aAddr >= base && aAddr < (base + size);
> 
> I find comparisons like this easier to read when written with the repeated
> value in the middle:
> 
> > return base <= aAddr && aAddr < (base + size);

Will update.

> @@ +535,5 @@
> > +  mach_vm_size_t size = 0;
> > +  for (mach_vm_address_t addr = MACH_VM_MIN_ADDRESS; ; addr += size) {
> > +    vm_region_top_info_data_t info;
> > +    mach_msg_type_number_t infoCount = VM_REGION_TOP_INFO_COUNT;
> > +    mach_port_t object_name;
> 
> Nit: |objectName|.

Will update.

> @@ +549,5 @@
> > +      return NS_ERROR_FAILURE;
> > +    }
> > +
> > +    if (InSharedRegion(addr, cpu_type) && info.share_mode != SM_PRIVATE) {
> > +      if (info.share_mode != SM_PRIVATE) {
> 
> You have |info.share_mode != SM_PRIVATE| twice, which is redundant. Is the
> second occurrence meant to be different?

Good catch, this was a half-finished refactoring.

> @@ +557,5 @@
> > +
> > +    if (info.share_mode == SM_COW && info.ref_count == 1) {
> > +      // Treat copy-on-write pages as private if they only have one reference.
> > +      info.share_mode = SM_PRIVATE;
> > +    }
> 
> I think it'd be clearer to move this if-block into the |SM_COW| case in the
> switch below.

Will update.

Comment 6

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9328175d2a4d

Comment 7

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0b3badc236df620d51863b9f87d2222738390f
Bug 1223927 - Add resident-unique measurement to OS X. r=njn

Comment 8

[Nicholas Nethercote [inactive]]

I didn't catch the fact that a Mac-only duplicate of ResidentUniqueReporter was being added here. If you could factor out the one added here in bug 1224685's patch that would be great. Thank you.

Comment 9

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> I didn't catch the fact that a Mac-only duplicate of ResidentUniqueReporter
> was being added here. If you could factor out the one added here in bug
> 1224685's patch that would be great. Thank you.

I will update this as well in bug 1224685.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe0b3badc236

Comment 11

[Chris H-C :chutten]

How good of a candidate for uplift would this patch be? :vladan would like bug 1198209 uplifted, and it's not very useful without this one and bug 1224685.

Comment 12

[Eric Rahm [:erahm]]

It should be straightforward, we're just adding new functions. The patch will probably need to be tweaked. I'd suggest just doing rollup w/ bug 1224685 rather than individually (bug 1224685 refactored some shared code, so order is important).

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8686266 [details] [diff] [review]
Add resident-unique measurement to OS X

Approval Request Comment
[Feature/regressing bug #]: Adds resident unique reporter for OS X.
[User impact if declined]: Can't run e10s A/B test on beta 44.
[Describe test coverage new/current, TreeHerder]: Baked on central for several weeks.
[Risks and why]: None, adds new functionality for OS X.
[String/UUID change made/needed]: None.

Note: applies cleanly to aurora, needs to land before bug 1224685.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8686266 [details] [diff] [review]
Add resident-unique measurement to OS X

This has been on Nightly for 2 weeks and is needed for e10s A/B experiment. Aurora44+

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/971d0fc7eef6

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/971d0fc7eef6