471950 - Improve bloatdiff.pl error handling

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1230946587.1230950961.19839.gz

compare bloat logs complete failed
=== Output ===
C:\WINDOWS\system32\cmd.exe /c perl tools/rb/bloatdiff.pl ../bloat.log.old ../bloat.log
[...]
Current file:  ../bloat.log
Previous file: ../bloat.log.old
[...]
ERROR - unable to calculate bloat/leak data.

HINT - Did your test run complete successfully?
HINT - Are you pointing at the right log files?
[...]
program finished with exit code 1
[...]
TinderboxPrint:<abbr title="refcnt Leaks">RLk</abbr>:0B
}

1a) The first hint should be replaced by automatic detection code...
1b) Moreover, it would help to have a hint of what/where the script actually "failed" !
1c) As a minimum, it may help to print the size of the 2 files.
2) |TinderboxPrint| should report "FAIL" instead of "0B".

Comment 1

[Serge Gautherie (:sgautherie)]

This happens elsewhere too, for example:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.1/1230969828.1230971784.31016.gz
WINNT 5.2 mozilla-1.9.1 leak test build on 2009/01/03 00:03:48

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

We should probably handle the error which leads to a truncated bloat log, and hence bloatdiff complaining.

In the build log in comment #1, we execute
  python leaktest.py -l bloat.log
and get (eventually)
  ###!!! ASSERTION: Failed to create the object: 'Error', file e:/builds/moz2-slave/mozilla-1.9.1-win32-debug/build/dom/src/base/nsDOMClassInfo.cpp, line 5006
which is fatal in these modern times. Then there are some leak stats, a stack for the assertion, and
 ERROR FAIL Exited with code 3 during test run
 SUCCESS: The process with PID 10328 has been terminated.
 [some irrelevant http logging]
 program finished with exit code 0

So we're not paying any attention to the exit code of leaktest.py (3) and the buildbot step appears to succeed (last line). It may be we want to be able to continue on to codesighs so can't make the whole build die at that point, but we'll have to improve the handling for a bloat run failing out somehow.

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

ted  & sayrer fixed the exit status issue over in bug 472706, so leaktest.py actually passes on what the test app returned.

Comment 4

[Mark Banner (:standard8)]

We've seen the error in comment 0 a few times now on Thunderbird, with no explained cause.

However I've just been playing around with leaks, and I did:

cp bloat.log bloat.log.old
perl mozilla/tools/rb/bloatdiff.pl bloat.log.old bloat.log

this gave me that error. I changed one number and it generated the output correctly.

It appears that bloatdiff.pl can't handle two identical logs. I can see this happening slightly more in the TB case because we're not covering as much as Firefox does at the moment.

Comment 5

[Mark Banner (:standard8)]

Attachment: Don't fail on no new leak

If one of the files didn't exist or is empty, the script complains about that. So I think the most likely cause of these errors is that we've had two runs exactly the same, or we've had no new bloat, both of which seem to me to be perfectly valid situations.

Therefore this patch removes the check for zero bloat percent, and adds an appropriate comment for the final possible reason.

I've also changed ERROR to Error: as then the tinderbox parsing will pick up the line as an actual error and display it at the start of the log display (hence making it easier to find the error in the log when it does occur).

I'm not sure if this bug needs moving between components, or if I've got the right reviewer, so please redirect as necessary.

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 360921 [details] [diff] [review]
Don't fail on no new leak

>     if (! $newMap{"TOTAL"} or 
>-	! $newMap{"TOTAL"}{bloat} or 
>+	! $newMap{"TOTAL"}{bloat}) {

Nit: align the '!'s.

Comment 7

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 360921 [details] [diff] [review]
Don't fail on no new leak

OK, the checks still left give us a rough proxy for the new log being valid.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 360921 [details] [diff] [review]
Don't fail on no new leak

This time, set 2nd review to the module owner.

Comment 9

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

Comment on attachment 360921 [details] [diff] [review]
Don't fail on no new leak

>+        print "There is either no data or the bloat sum is zero\n\n";

"the bloat sum is zero" means that there were no logged objects allocated, which is basically equivalent to no data.  So I'd drop the "or the bloat sum is zero" or find some other way to reword.

Since you're changing almost half the lines in the file that have tabs in them, could you just make sure to completely untabify the file?

r=dbaron with that

Comment 10

[Mark Banner (:standard8)]

Checked in with comments addressed: http://hg.mozilla.org/mozilla-central/rev/e51cf7cb4f63.

I'll push this to mozilla-1.9.1 later as its a test-only change.

Comment 11

[Mark Banner (:standard8)]

Checked in to mozilla-1.9.1 as test-only change: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ed3533ec9e9b

Comment 12

[Chris Cooper [:coop] (he/him)]

Moving closed Future bugs into Release Engineering in preparation for removing the Future component.