I think is just a matter of propagating that information in memory_metric.html [1], around line 460 where there is already some code to report the Ignition-specific code size stats.
Should be quite straightforward. Let's just wait for Petr to confirm.


[1] https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/metrics/system_health/memory_metric.html?rcl=0&l=460

Primiano is right, we simply have to dump the information in memory_metric.html.

ulan: What exactly should the number be?

Example 1 (real): V8 has a heap of size 15 MiB which is backed by 20 MiB in malloc. Should the reported value be 15 or 20?

Example 2 (possibly unreal): Blink holds an object of size 12 MiB, which is represented by 15 MiB in V8, which in turn is backed by 20 MiB malloc space. What should be reported here? Is this even possible?

> Example 1 (real): V8 has a heap of size 15 MiB which is backed by 20 MiB in malloc. Should the reported value be 15 or 20?

I think Ulan is not taking about the v8 heap here (which AFAIK is never backed by malloc, always directly mmaped), rather on the v8/isolateX/malloc entry (it's a farily new addition) which is extra memory used during some peaks (mostly parsing / building AST IIRC) which is always fully backed by malloc.

See screenshot

> Example 2 (possibly unreal): Blink holds an object of size 12 MiB, which is represented by 15 MiB in V8, which in turn is backed by 20 MiB malloc space. What should be reported here? Is this even possible?
I believe actual JS objects always live in the heap spaces which are always not backed by malloc.
That malloc entry is to be read as "auxiliary memory that v8 needs from time to time to perform some tasks"

Deleted: v8malloc.png 16.8 KB

	v8malloc.png 16.8 KB View Download

Thanks for the detailed explanation. So that means that Ulan is essentially interested in sum(i.get('malloc', 0) for i in v8.isolates), right?

Thanks!

I see that Ignition code size stats are reported only for detailed dump. Would it be possible to report v8:malloc for light dumps too? (because v8.browsing benchmark is using light dumps).

> ulan: What exactly should the number be?
V8 provides malloced_memory counter in GetHeapStatistics for memory dumps:
https://cs.chromium.org/chromium/src/v8/src/api.cc?rcl=0&l=7570
We would like to use that number.

> Example 1 (real): V8 has a heap of size 15 MiB which is backed by 20 MiB in malloc. Should the reported value be 15 or 20?
Note that V8 heap and malloced memory are disjoint, i.e. for V8 heap we get memory pages directly from OS.

> Example 2 (possibly unreal): Blink holds an object of size 12 MiB, which is represented by 15 MiB in V8, which in turn is backed by 20 MiB malloc space. What should be reported here? Is this even possible?
We compute V8 heap size and V8 malloced memory size independent from Blink. So if V8 heap is 15 MiB and V8 malloced memory is 20MiB (these are disjoint), then we will report 15 for v8:allocated_objects_size and 20 for V8:malloc.
If a Blink object keeps a V8 heap object alive, then the V8 heap object size is attributed to V8 heap, not Blink, i.e. we do not try to keep retaining path information.

> Thanks for the detailed explanation. So that means that Ulan is essentially interested in sum(i.get('malloc', 0) for i in v8.isolates), right?
Yep!

> I see that Ignition code size stats are reported only for detailed dump. Would it be possible to report v8:malloc for light dumps too? (because v8.browsing benchmark is using light dumps).

Yes, it's no problem to report the number for light dumps as well.

> > ulan: What exactly should the number be?
> V8 provides malloced_memory counter in GetHeapStatistics for memory dumps:
> https://cs.chromium.org/chromium/src/v8/src/api.cc?rcl=0&l=7570
> We would like to use that number.

Is that number piped through to memory-infra? (i.e. is it in some memory
allocator dump, e.g. v8/isolate_0xABCD/malloc)

> > Example 1 (real): V8 has a heap of size 15 MiB which is backed by 20 MiB in malloc. Should the reported value be 15 or 20?
> Note that V8 heap and malloced memory are disjoint, i.e. for V8 heap we get memory pages directly from OS.
> 
> > Example 2 (possibly unreal): Blink holds an object of size 12 MiB, which is represented by 15 MiB in V8, which in turn is backed by 20 MiB malloc space. What should be reported here? Is this even possible?
> We compute V8 heap size and V8 malloced memory size independent from Blink. So if V8 heap is 15 MiB and V8 malloced memory is 20MiB (these are disjoint), then we will report 15 for v8:allocated_objects_size and 20 for V8:malloc.
> If a Blink object keeps a V8 heap object alive, then the V8 heap object size is attributed to V8 heap, not Blink, i.e. we do not try to keep retaining path information.

Thanks for the explanation.

> Is that number piped through to memory-infra? (i.e. is it in some memory
allocator dump, e.g. v8/isolate_0xABCD/malloc)
Yes, I think v8/isolate_0xABCD/malloc is exactly that number.

OK. Given this, I propose that this is number is exported as memory:<browser>:<process>:reported_by_chrome:v8:allocated_by_malloc:effective_size. WDYT?

> Given this, I propose that this is number is exported as memory:<browser>:<process>:reported_by_chrome:v8:allocated_by_malloc:effective_size. WDYT?
SGTM. What would be the best way to do it?

I think you need to do the following:

  1. The following code needs to be added right after https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metrics/system_health/memory_metric.html#L271:

    var v8Dump = processDump.getMemoryAllocatorDumpByFullName('v8');
    if (v8Dump !== undefined) {
      var allocatedByMalloc = 0;

      v8Dump.children.forEach(function(isolateDump) {
        var mallocDump = isolateDump.getDescendantDumpByFullName('malloc');
        if (mallocDump === undefined ||
            mallocDump.numerics['effective_size'] === undefined) {
          return;
        }
        allocatedByMalloc += mallocDump.numerics['effective_size'].value;
      });

      addProcessScalar({
        source: 'reported_by_chrome',
        component: ['v8', 'allocated_by_malloc'],
        property: 'effective_size',
        value: allocatedByMalloc,
        unit: sizeInBytes_smallerIsBetter,
        descriptionPrefixBuilder: CHROME_VALUE_PROPERTIES['effective_size']
      })
    }

  2. Change CHROME_VALUE_PROPERTIES
     (https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metrics/system_health/memory_metric.html#L357)
     to be a dictionary (propertyName as key).

  3. Update buildChromeValueDescriptionPrefix
     (https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metrics/system_health/memory_metric.html#L320)
     to return a human readable message for your case. Specifically, you
     probably want to add a special case to line 339.

How does that sound?

Thank you for the code and clear instructions :) I will upload CL.

About "3. Update buildChromeValueDescriptionPrefix":
Since formatSpec.componentPreposition is 'of', we will follow branch in line 342, so the computed description will be some something like
'effective size of v8:allocated_by_malloc in process_name',
which looks good to me without special casing.

Do you think it is worth transforming the description further to something like
'effective size of all objects allocated by malloc in v8 in process_name' ?
If do we that, we would need to also special case the 'total' branch?

I think that "Effective size of all objects allocated by malloc for v8 in process_name" would be easier to understand for people (than "allocated by v8:allocated_by_malloc"). It shouldn't be too much extra work.

What do you mean by the 'total' branch? "all_processes"?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/674bd36ad85e568b62ebd80e79e2589a02e9b7e5

commit 674bd36ad85e568b62ebd80e79e2589a02e9b7e5
Author: catapult-deps-roller <catapult-deps-roller@chromium.org>
Date: Mon Jul 18 18:09:12 2016

Roll src/third_party/catapult/ d2b666834..3c2ec0209 (1 commit).

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/d2b666834a28..3c2ec02098fa

$ git log d2b666834..3c2ec0209 --date=short --no-merges --format='%ad %ae %s'

BUG= 628239 

TBR=catapult-sheriff@chromium.org

Review-Url: https://codereview.chromium.org/2162583002
Cr-Commit-Position: refs/heads/master@{#406035}

[modify] https://crrev.com/674bd36ad85e568b62ebd80e79e2589a02e9b7e5/DEPS

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/674bd36ad85e568b62ebd80e79e2589a02e9b7e5

commit 674bd36ad85e568b62ebd80e79e2589a02e9b7e5
Author: catapult-deps-roller <catapult-deps-roller@chromium.org>
Date: Mon Jul 18 18:09:12 2016

Roll src/third_party/catapult/ d2b666834..3c2ec0209 (1 commit).

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/d2b666834a28..3c2ec02098fa

$ git log d2b666834..3c2ec0209 --date=short --no-merges --format='%ad %ae %s'

BUG= 628239 

TBR=catapult-sheriff@chromium.org

Review-Url: https://codereview.chromium.org/2162583002
Cr-Commit-Position: refs/heads/master@{#406035}

[modify] https://crrev.com/674bd36ad85e568b62ebd80e79e2589a02e9b7e5/DEPS

Jochen added tracking of the peak malloc size:
1) https://codereview.chromium.org/2153423002
2) https://codereview.chromium.org/2156043002

Now we would like to report peak malloc size in memory metric as "reported_by_chrome:v8:allocated_by_malloc:peak_size" (or something similar).

The motivation is that peaks of malloc memory in V8 occur in small time windows (e.g. compilation of a function). By sampling memory dumps we are unlikely to catch the peak.

Petr, Primiano, would it be ok if I add this to the memory metric? Suggestions regarding naming and code  are very welcome.

Defer to Petr for the details, from my viewpoint it seems perfectly reasonable.
I think the deal is:
- is that peak_malloced_memory() an absolute watermark or resets on every read? I assume the former.
- I suppose you want to keep track just of the max(peak) if that is the case.

it an absolute watermark, so it'll monotonically grow

reported_by_chrome:v8:allocated_by_malloc:peak_size sounds good to me. The code should be pretty similar to https://codereview.chromium.org/2153823002. Let me know if you need any more help. Thanks!

Thanks, Primiano and Petr. Here is a draft: https://codereview.chromium.org/2169873002/

There are two issues:
1) the "peak_size" property automatically propagates up to v8 and chrome:
- reported_by_chrome:v8:allocated_by_malloc:peak_size_max
- reported_by_chrome:v8:peak_size_max
- reported_by_chrome:peak_size_max

Is it possible disable this propagation?
Maybe I should create a separated component for peak malloc, like reported_by_chrome:v8:peak_allocated_by_malloc:size? Will that affect size of parent components?

2) Mostly nit: the name "peak_size_max" has redundant word "max". Is there an easy way to drop "max"? If not, then having "max" is fine.

1) Good point. There's already a similar problem with "locked_size". Would you be OK with landing it as is and assigning a bug to me to fix it?

2) I know it's a little strange, but it makes sense once you combine "peak_size" from multiple traces. Therefore, let's keep it as is.

> 1) Good point. There's already a similar problem with "locked_size". Would you be OK with landing it as is and assigning a bug to me to fix it?
Absolutely! Thank you.

> 2) I know it's a little strange, but it makes sense once you combine "peak_size" from multiple traces. Therefore, let's keep it as is.
Ok, makes sense.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/695cf18a165c5ef9cdc16e5dea734d04c62656ac

commit 695cf18a165c5ef9cdc16e5dea734d04c62656ac
Author: catapult-deps-roller <catapult-deps-roller@chromium.org>
Date: Thu Jul 21 15:20:09 2016

Roll src/third_party/catapult/ 60ef25306..cb90e4130 (1 commit).

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/60ef2530645f..cb90e4130adf

$ git log 60ef25306..cb90e4130 --date=short --no-merges --format='%ad %ae %s'

BUG= 628239 

TBR=catapult-sheriff@chromium.org

Review-Url: https://codereview.chromium.org/2171753002
Cr-Commit-Position: refs/heads/master@{#406853}

[modify] https://crrev.com/695cf18a165c5ef9cdc16e5dea734d04c62656ac/DEPS