Closed Bug 1517409 Opened 11 months ago Closed 11 months ago

Provide heap size info to the gecko profiler's GCMajor markers

Categories

(Core :: JavaScript: GC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 8 obsolete files)

1.24 KB, patch
jonco
: review+
Details | Diff | Splinter Review
10.71 KB, patch
jonco
: review+
Details | Diff | Splinter Review
3.02 KB, patch
jonco
: review+
Details | Diff | Splinter Review
3.24 KB, patch
jonco
: review+
Details | Diff | Splinter Review
The gecko profiler currently has the size of the JS heap reported in each GCMajor event.  However we want to add:

 + Heap usage immediately after sweeping (not capacity).
 + Allocated memory as a profiler counter.
 + Heap capacity as a profiler counter.

We need to correlate the first two to determine the heap usage at any given time.
Note also that since we do snapshot-at-the-beginning the freed memory is only since the start of the collection.

Note that:

 + We need to increment the allocation counter for each non-nursery allocation.
 + We could delay incrementing the allocation counter during tenuring, since we 
   already count tenured memory and can just re-use that value.
 + Memory allocated by the mutator during a gc, including nursery evictions may 
   make it seem like we freed more memory than we did.  We can only ever free
   the memory allocated before the GC started, since we use
   snapshot-at-the-beginning.
Hi Jesup.

Regarding my messages in devtools slack.  I want to add some counters to the profiler so that with each sample we can see how much memory the GC is using.

Since SpiderMonkey can only call to gecko via callbacks (as it needs to be built separately also) what's the best way to do this?  Can I give gecko the address of an integer (an atomic integer?) and let it read it for each sample?

Can you point me at the code within gecko where this is handled so I know where I should be looking.

Thanks.
Flags: needinfo?(rjesup)
Depends on: 1505589
Flags: needinfo?(rjesup)
Let's do this portion of the work now, and the counters as a seperate bug later.
No longer depends on: 1505589
Summary: Provide heap usage info to the gecko profiler → Provide heap usage info to the gecko profiler's GCMajor markers
Blocks: 1518061
I think that HeapUsage::gcBytes should be renamed so that it's clear it measures the size of the GC heap, not the size of allocated things.  We may have to rename some other things to make this more uniform.
Attachment #9034643 - Flags: review?(jcoppeard)
Attachment #9034644 - Flags: review?(jcoppeard)
Attachment #9034645 - Flags: review?(jcoppeard)
Attachment #9034645 - Attachment is obsolete: true
Attachment #9034645 - Flags: review?(jcoppeard)
Attachment #9034659 - Flags: review?(jcoppeard)
Comment on attachment 9034643 [details] [diff] [review]
Bug 1517409 - (part 1) Rename HeapUsage::gcBytes to heapCapacity

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

The structure is called HeapUsage and I think this makes it clear that this relates to the heap as a whole rather than the cells allocated within it.  I don't think capacity is really right either because that does imply to me that it's in terms of allocated cells (and this number also includes the overhead of arena headers).  I think we should leave this as it is.
Attachment #9034643 - Flags: review?(jcoppeard)

(In reply to Jon Coppeard (:jonco) from comment #7)

Comment on attachment 9034643 [details] [diff] [review]
Bug 1517409 - (part 1) Rename HeapUsage::gcBytes to heapCapacity

Review of attachment 9034643 [details] [diff] [review]:

The structure is called HeapUsage and I think this makes it clear that this
relates to the heap as a whole rather than the cells allocated within it. I
don't think capacity is really right either because that does imply to me
that it's in terms of allocated cells (and this number also includes the
overhead of arena headers). I think we should leave this as it is.

Okay, I wasn't sure about this anyway so I'm happy to drop this patch. I wasn't sure because capacity makes it sound like some kind of limit, which is isn't.

These names make me think of different things. HeapUsage reads as "usage of the heap".

Attachment #9034643 - Attachment is obsolete: true

By refactoring this test code we can more easily adjust the number of fields there are supposed to be in these objects.

Attachment #9034889 - Flags: review?(florian)

Update this path in these comments since the file it refers to was moved.

Attachment #9034890 - Flags: review?(jcoppeard)

Count and the used amount of memory in the GC, and report it to the profiler
in the GCMajor marker.

This is the first time seeing this data for this GC for me. Heap utilization after a collection seems to be typically 75%. It'll be interesting look at this when we add more compacting as is one of our 2019 goals.

Attachment #9034902 - Flags: review?(jcoppeard)

(In reply to Paul Bone [:pbone] from comment #8)

These names make me think of different things. HeapUsage reads as "usage of the heap".

Oh, that's interesting, that interpretation hadn't occurred to me. I read it as 'how much memory the heap uses'. I'd be happy to rename this to HeapSize or similar if you think that's better.

Attachment #9034890 - Flags: review?(jcoppeard) → review+
Comment on attachment 9034902 [details] [diff] [review]
Bug 1517409 - (part 4) Count the used memory in the GC heap during sweeping

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

The /js/src bits look fine, just one question...

::: js/src/gc/Statistics.cpp
@@ +984,5 @@
>    postCapacity = runtime->gc.usage.gcBytes();
>  
> +  postUsage = 0;
> +  for (auto zone : runtime->gc.zones()) {
> +    postUsage += zone->markedBytes();

This counts the markedBytes of all zone, including ones which were not collected.  Is that what we want here?

::: js/src/gc/Zone.h
@@ +353,5 @@
> +  void resetMarkedBytes() {
> +    markedBytes_ = 0;
> +  }
> +
> +  size_t markedBytes() const { return markedBytes_; } 

nit: looks like there's a trailing space here.
Comment on attachment 9034644 [details] [diff] [review]
Bug 1517409 - (part 2) Rename preBytes to preCapacity

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

I guess we'll call this something other than capacity now.  about:memory uses 'heap committed' and 'heap used' so that might be the way to go.
Attachment #9034644 - Flags: review?(jcoppeard)
Comment on attachment 9034659 [details] [diff] [review]
Bug 1517409 - (part 3) Add a postCapacity measurement

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

Ditto previous comment.
Attachment #9034659 - Flags: review?(jcoppeard)
Comment on attachment 9034889 [details] [diff] [review]
Bug 1517409 - Make it easier to update the GC telemetry unit test

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryGC.js
@@ +49,5 @@
>  
>    // Exactly the limit of fields.
> +  let limit = 24;
> +  let my_gc_exact = make_gc();
> +  Assert.equal(19, Object.keys(my_gc_exact).length);

nit: It seems 19 should be another constant with a name, as it's a magic value used 3 times in the code (assuming the -18 a few lines later is -19+1).

@@ +56,3 @@
>    }
> +  GCTelemetry.observeRaw(my_gc_exact);
> +  // Assert that it was recorded has all the fields.

nit: The phrasing of this comment seems incorrect, both before and after your changes. Is there a missing 'and' between 'recorded' and 'has'?
Attachment #9034889 - Flags: review?(florian) → review+

(In reply to Jon Coppeard (:jonco) from comment #12)

(In reply to Paul Bone [:pbone] from comment #8)

These names make me think of different things. HeapUsage reads as "usage of the heap".

Oh, that's interesting, that interpretation hadn't occurred to me. I read it as 'how much memory the heap uses'. I'd be happy to rename this to HeapSize or similar if you think that's better.

Yeah, I like HeapSize better.

(In reply to Jon Coppeard (:jonco) from comment #13)

Comment on attachment 9034902 [details] [diff] [review]
Bug 1517409 - (part 4) Count the used memory in the GC heap during sweeping

Review of attachment 9034902 [details] [diff] [review]:

The /js/src bits look fine, just one question...

::: js/src/gc/Statistics.cpp
@@ +984,5 @@

postCapacity = runtime->gc.usage.gcBytes();

  • postUsage = 0;
  • for (auto zone : runtime->gc.zones()) {
  • postUsage += zone->markedBytes();

This counts the markedBytes of all zone, including ones which were not
collected. Is that what we want here?

Oops. I will need to count all the zones, but this will count those zones inaccurately. I'll have to come up with something else.

Thanks.

(In reply to Jon Coppeard (:jonco) from comment #14)

Comment on attachment 9034644 [details] [diff] [review]
Bug 1517409 - (part 2) Rename preBytes to preCapacity

Review of attachment 9034644 [details] [diff] [review]:

I guess we'll call this something other than capacity now. about:memory
uses 'heap committed' and 'heap used' so that might be the way to go.

heap available / heap used?

committed means to me that we definitely know it won't page fault, but I'm not certain if that's the actual meaning.

Comment on attachment 9034889 [details] [diff] [review]
Bug 1517409 - Make it easier to update the GC telemetry unit test

Moved this patch to Bug 1518713

Attachment #9034889 - Attachment is obsolete: true
Depends on: 1518713

(In reply to Paul Bone [:pbone] from comment #19)

committed means to me that we definitely know it won't page fault, but I'm not certain if that's the actual meaning.

That is committed memory. Decommitted memory in chunks is not included in this. You can still take a page fault accessing committed memory though if the OS has paged it out.

Priority: -- → P1
Target Milestone: --- → mozilla66

I have working patches that work for zonal GCs and compute the memory usage at the beginning of a GC also. But I think premarking is throwing them off. Sometimes the memory used is greater than the heap size!

Heap usage can't be calculated accuratly because of premarking. And there's no simple way to fix that right now. So I'll postpone that work for later and only show heap size with this bug.

No longer blocks: 1518061
Summary: Provide heap usage info to the gecko profiler's GCMajor markers → Provide heap size info to the gecko profiler's GCMajor markers
Attachment #9034902 - Attachment is obsolete: true
Attachment #9034902 - Flags: review?(jcoppeard)
Attachment #9034644 - Attachment is obsolete: true
Attachment #9034659 - Attachment is obsolete: true
Attachment #9036498 - Flags: review?(jcoppeard)

Hi jonco & dexter,

This patch has changes to both js/src and toolkit/components/telemetry. Could you provide reviews for your respective domain?

Thanks.

Attachment #9036501 - Flags: review?(jcoppeard)
Attachment #9036501 - Flags: review?(alessio.placitelli)
Attachment #9036501 - Attachment is obsolete: true
Attachment #9036501 - Flags: review?(jcoppeard)
Attachment #9036501 - Flags: review?(alessio.placitelli)
Attachment #9036506 - Flags: review?(jcoppeard)
Attachment #9036506 - Flags: review?(alessio.placitelli)
Attachment #9036498 - Flags: review?(jcoppeard) → review+
Comment on attachment 9036499 [details] [diff] [review]
Bug 1517409 - (part 3) Rename Statustics::preBytes to preHeapSize

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

This is fine but in general I would prefer names that read nicely in English, e.g. 'initialHeapSize' or 'heapSizePreCollection'.
Attachment #9036499 - Flags: review?(jcoppeard) → review+
Attachment #9036506 - Flags: review?(jcoppeard) → review+
Comment on attachment 9036506 [details] [diff] [review]
Bug 1517409 - (part 4) Add a postCapacity measurement

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

This looks good wrt the telemetry changes. Please note that this needs data-review, as far as I know, as we're adding a new field.
Attachment #9036506 - Flags: review?(jcoppeard)
Attachment #9036506 - Flags: review?(alessio.placitelli)
Attachment #9036506 - Flags: review+

Chris, would adding a field to GC telemetry require a data-review?

Flags: needinfo?(chutten)
Attachment #9036506 - Flags: review?(jcoppeard) → review+

(In reply to Alessio Placitelli [:Dexter] from comment #30)

Chris, would adding a field to GC telemetry require a data-review?

It would! All new or changed data collections (except removals) require Data Collection Review.

Flags: needinfo?(chutten)
See Also: → 1520357

Before we go adding more telemetry, I want to see if it'll actually be useful and whether anyone even uses some of the data we collect now: Bug 1520357

BTW. this comes up because the same code for telemetry and profiling is shared. Of course it could have a flag about which fields to write out, but so far that was never implemented.

Depends on: 1520357

Here's an alternative version of Part 4 which does not add any new telemetry. I cannot justify adding new fields to telemetry, since my goal is just to get this data to the profiler.

Attachment #9037137 - Flags: review?(jcoppeard)
Attachment #9037137 - Flags: review?(jcoppeard) → review+
Attachment #9036506 - Attachment is obsolete: true
Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92b72891266c
(part 1) Fix path in comments r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c747052bceb
(part 2) Rename HeapUsage to HeapSize r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b29deaac54a
(part 3) Rename Statustics::preBytes to preHeapSize r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac2f47b3cc2
(part 4) Add a postCapacity measurement r=jonco
You need to log in before you can comment on or make changes to this bug.