1056954 - Normalize 'pid' values when diffing

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Eric Rahm [:erahm]]

We already do this for forms such as "pid 1234". We should handle other forms as well such as pid=1234, this would improve diffing for ion and kgsl.

gralloc could be improved as well, but we need to add the process name to the entry so that the values coalesce properly.

ion-memory diff example:

0.05 MB (100.0%) -- ion-memory
├──-5.91 MB (-10800.00%) ── Homescreen (pid=5518) [21] [-]
├──5.91 MB (10800.00%) ── Homescreen (pid=6051) [21] [+]
├──1.87 MB (3421.43%) ── b2g (pid=5884) [5] [+]
├──-1.82 MB (-3321.43%) ── b2g (pid=5319) [3] [-]
├──0.00 MB (00.00%) -- kworker
│  ├──1.58 MB (2892.86%) ── 0:0 (pid=3891) [+]
│  └──-1.58 MB (-2892.86%) ── 1:1 (pid=4446) [-]
├──-1.58 MB (-2892.86%) ── mdss_fb0 (pid=5327) [-]
└──1.58 MB (2892.86%) ── mdss_fb0 (pid=5892) [+]

kgsl-memory diff example:

0.09 MB (100.0%) -- kgsl-memory
├──10.42 MB (11600.00%) -- b2g (pid=5884)
│  ├───3.98 MB (4426.09%) ── egl_image [11] [+]
│  ├───3.16 MB (3521.74%) ── egl_surface [2] [+]
│  ├───2.02 MB (2247.83%) ── gl [11] [+]
│  ├───0.63 MB (695.65%) ── command [10] [+]
│  ├───0.61 MB (682.61%) ── any(0) [18] [+]
│  ├───0.02 MB (21.74%) ── texture [5] [+]
│  └───0.00 MB (04.35%) ── arraybuffer [+]
└──-10.33 MB (-11500.00%) -- b2g (pid=5319)
   ├───-3.95 MB (-4395.65%) ── egl_image [10] [-]
   ├───-3.16 MB (-3521.74%) ── egl_surface [2] [-]
   ├───-2.02 MB (-2247.83%) ── gl [11] [-]
   ├───-0.61 MB (-682.61%) ── any(0) [18] [-]
   ├───-0.56 MB (-626.09%) ── command [9] [-]
   ├───-0.02 MB (-21.74%) ── texture [5] [-]
   └───-0.00 MB (-4.35%) ── arraybuffer [-]

gralloc diff example:


0.05 MB (100.0%) -- gralloc
├──-5.91 MB (-10800.00%) ── pid(5518)/buffer(width=256, height=256, bpp=4, stride=288) [21]
├──5.91 MB (10800.00%) ── pid(6051)/buffer(width=256, height=256, bpp=4, stride=288) [21]
├──3.35 MB (6120.09%) -- pid(5884)
│  ├──3.13 MB (5718.75%) ── buffer(width=480, height=854, bpp=4, stride=480) [2] [+]
│  ├──0.16 MB (301.34%) ── buffer(width=480, height=45, bpp=4, stride=480) [2] [+]
│  └──0.05 MB (100.0%) ── buffer(width=203, height=32, bpp=4, stride=224) [2] [+]
└──-3.29 MB (-6020.09%) -- pid(5319)
   ├──-3.13 MB (-5718.75%) ── buffer(width=480, height=854, bpp=4, stride=480) [2] [-]
   └──-0.16 MB (-301.34%) ── buffer(width=480, height=45, bpp=4, stride=480) [2] [-]

Comment 1

[Eric Rahm [:erahm]]

Attachment: Normalize pid values when diffing

This patch normalizes all pid entries of the form /pid.[number]/, which matches "pid 123", "pid=1234", etc. It applies the pid normalization to the path as well.

Comment 2

[Eric Rahm [:erahm]]

Example output with the same memory-reports in comment 0:

0.05 MB (100.0%) -- gralloc
└──0.05 MB (100.0%) ── pid(NNN)/buffer(width=203, height=32, bpp=4, stride=224) [2]

0.05 MB (100.0%) -- ion-memory
├──0.00 MB (00.00%) -- kworker
│  ├──1.58 MB (2892.86%) ── 0:0 (pid=NNN) [+]
│  └──-1.58 MB (-2892.86%) ── 1:1 (pid=NNN) [-]
└──0.05 MB (100.0%) ── b2g (pid=NNN) [5]

0.09 MB (100.0%) -- kgsl-memory
└──0.09 MB (100.0%) -- b2g (pid=NNN)
   ├──0.06 MB (69.57%) ── command [10]
   └──0.03 MB (30.43%) ── egl_image [11]

Comment 3

[Eric Rahm [:erahm]]

try: https://tbpl.mozilla.org/?tree=Try&rev=1ef2aac5567a

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8477579 [details] [diff] [review]
Normalize pid values when diffing

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

Thanks!

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +822,5 @@
>      // Strip out some non-deterministic stuff that prevents clean diffs --
>      // e.g. PIDs, addresses, null principal UUIDs. (Note that we don't strip
>      // out all UUIDs because some of them -- such as those used by add-ons --
>      // are deterministic.)
> +    let pidRegex = /pid(.)\d+/g;

/pid([ =])\d+/g ?

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8477579 [details] [diff] [review]
Normalize pid values when diffing

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

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +822,5 @@
>      // Strip out some non-deterministic stuff that prevents clean diffs --
>      // e.g. PIDs, addresses, null principal UUIDs. (Note that we don't strip
>      // out all UUIDs because some of them -- such as those used by add-ons --
>      // are deterministic.)
> +    let pidRegex = /pid(.)\d+/g;

Yeah, it seems better to narrow the scope. I'll update it.

Comment 6

[Eric Rahm [:erahm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ebeaf707ed6b

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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