Closed Bug 1223927 Opened 4 years ago Closed 4 years ago

Add |resident-unique| measurement to OS X

Categories

(Core :: XPCOM, defect)

Unspecified
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(1 file)

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
Attachment #8686266 - Flags: review?(n.nethercote)
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 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.
Attachment #8686266 - Flags: review?(n.nethercote) → review+
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.
Blocks: 1198209
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/fe0b3badc236
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
Flags: needinfo?(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).
Flags: needinfo?(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.
Attachment #8686266 - Flags: approval-mozilla-aurora?
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+
Attachment #8686266 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.