1156982 - Add separators to BloatView output

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Currently the BloatView parser expects that columns are whitespace separated, but we get cases like this:

== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 1847
     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
                                              Per-Inst   Leaked    Total      Rem
   0 TOTAL                                          29       16 283640633        0
   1                                                16       16        1        1
 255 FilterNodeLightingSoftware<PointLight, D      192        0        3        0
 256 FilterNodeLightingSoftware<PointLight, S      464        0        4        0
 257 FilterNodeLightingSoftware<SpotLight, Sp      768        0        2        0
 684 WeakReference<MessageListener>                 16        0   142672 18446744073709551615

The first entry after TOTAL is blank, so who knows what the test harness parser thinks is happening.  Then, the Filter stuff has spaces in the name, so the parser will be confused by that.  Note that it says there are 0 remaining of these, so test harness won't actually report anything there.  I'm not sure why those entries are printed, but there's likely some underflow involved, which I guess I can try to split out in another bug.

Comment 1

[Andrew McCreight [:mccr8]]

Oh, right, so I was going to say, we should output some separator character we think won't appear, and then get the test harness to deal with it.  I was thinking |.  (I CC'ed Jesse in case he has scripts that also parse this format.)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

These entries are being printed because, really, all the others were printed, but then all the ones that the test harness code understood got stripped out.

Comment 3

[Andrew McCreight [:mccr8]]

Thanks, that makes sense.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Add separators to BloatView output.

This will ensure we properly parse class names containing spaces.

Note that if a class name somehow ends up containing operator| then this will end up again silently failing.

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

It is very orange, but that's only because I accidentally pushed with the content process leak threshold reduced to 0. But at least that lets us know that it still works.  The output looked reasonable in a log I checked.

Here's what the output looks like:

17:53:30     INFO -  == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 24675
17:53:30     INFO -       |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
17:53:30     INFO -       |                                      | Per-Inst   Leaked|   Total      Rem|
17:53:30     INFO -     0 |TOTAL                                 |       16     2444|91990309       33|
17:53:30     INFO -    20 |AsyncTransactionTrackersHolder        |       40       40|     291        1|
17:53:30     INFO -    84 |CompositorChild                       |      472      472|       1        1|
17:53:30     INFO -    85 |CondVar                               |       24       48|     278        2|
17:53:30     INFO -   331 |MessagePump                           |        8        8|      49        1|
17:53:30     INFO -   340 |Mutex                                 |       20       60|   14151        3|
17:53:30     INFO -   359 |PCompositorChild                      |      412      412|       1        1|
17:53:30     INFO -   365 |PImageBridgeChild                     |      416      416|       1        1|
17:53:30     INFO -   420 |ReentrantMonitor                      |       24       24|    5909        1|
17:53:30     INFO -   421 |RefCountedMonitor                     |       48       96|       5        2|
17:53:30     INFO -   422 |RefCountedTask                        |        8       32|      10        4|
17:53:30     INFO -   480 |StoreRef                              |        8       32|       6        4|
17:53:30     INFO -   536 |WaitableEventKernel                   |       40       40|      52        1|
17:53:30     INFO -   543 |WeakReference<MessageListener>        |        8       16|    8069        2|
17:53:30     INFO -   610 |ipc::MessageChannel                   |      292      584|       5        2|
17:53:30     INFO -  1113 |nsTArray_base                         |        4       24|34409891        6|
17:53:30     INFO -  1124 |nsThread                              |      140      140|      48        1|

It is kind of ugly, I will admit.

The important regexp change is that before the name was a sequence of non-space characters followed by a sequence of space characters, while now it is a sequence of non-| characters followed by a | character.  Then I run rstrip to remove any trailing space, rather than trying to muck around with doing it all in the regexp.

Comment 5

[Andrew McCreight [:mccr8]]

I'll need to update the bloat log documentation with my amazing ascii art.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8596024 [details] [diff] [review]
Add separators to BloatView output.

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

::: build/automationutils.py
@@ +104,5 @@
> +  #     |            |Per-Inst  Leaked|     Total  Rem|
> +  #   0 |TOTAL       |      17     192| 419115886    2|
> +  # 833 |nsTimerImpl |      60     120|     24726    2|
> +  lineRe = re.compile(r"^\s*\d+ \|"
> +                      r"(?P<name>[^|]+)\|\s*"

I think we should have a comment here to remind people that the class name can include spaces, and we'll get rid of any trailing space during processing.

@@ +105,5 @@
> +  #   0 |TOTAL       |      17     192| 419115886    2|
> +  # 833 |nsTimerImpl |      60     120|     24726    2|
> +  lineRe = re.compile(r"^\s*\d+ \|"
> +                      r"(?P<name>[^|]+)\|\s*"
> +                      r"(?P<size>-?\d+)\s+(?P<bytesLeaked>-?\d+)\s*\|\s*"

I think the regex is a little clearer if you write it like:

r"...stuff...\|"
r"\s*...stuff...\|"
r"\s*...stuff..."

So that the breaks in the regex strings correspond to the fields that we're trying to process.  Having the \s* at the end of the strings seems odd to me...though maybe having it at the beginning obfuscates things?  Not sure.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8753e61b1db2

Comment 8

[Andrew McCreight [:mccr8]]

I added a comment, and also added an example class with a space in it.

I adjusted the regexp as you suggested.

ni myself to update the docs if this sticks.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/8753e61b1db2

Comment 10

[Andrew McCreight [:mccr8]]

I updated https://developer.mozilla.org/en-US/docs/Mozilla/Performance/BloatView