Bug 66227 - REGRESSION (NRWT): build.webkit.org doesn't show the total number of leaks found during a test run on the Leaks bot
Summary: REGRESSION (NRWT): build.webkit.org doesn't show the total number of leaks fo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Seidel (no email)
URL: http://build.webkit.org/builders/Snow...
Keywords: InRadar, Regression
Depends on: 71059
Blocks: 34984
  Show dependency treegraph
 
Reported: 2011-08-15 08:03 PDT by Adam Roben (:aroben)
Modified: 2011-10-27 14:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.67 KB, patch)
2011-10-27 13:43 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
hopefully this is easier to review (8.28 KB, patch)
2011-10-27 14:20 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
include code to turn on NRWT for leaks bot (9.66 KB, patch)
2011-10-27 14:22 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (9.82 KB, patch)
2011-10-27 14:36 PDT, Eric Seidel (no email)
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-08-15 08:03:29 PDT
To reproduce:

1. Go to http://build.webkit.org/builders/SnowLeopard%20Intel%20Leaks

Each build says something like this:

Failed leaks found for a total of 196,240 bytes! 1 unique leaks found!

With ORWT, it used to say something like this:

Failed 1400 total leaks found!

Not having the total number of leaks printed makes it hard to see at a glance whether the number of leaks is increasing or decreasing. I suspect it also breaks Leaks Viewer's recent-leaky-builds list, which can be seen by going to <http://build.webkit.org/LeaksViewer/>.
Comment 1 Eric Seidel (no email) 2011-08-15 10:02:30 PDT
We could get this number from re-parsing the files.  But ORWT's design of having a single global variable as a counter cannot work.

I'm happy to move --leaks processing back to ORWT until these two bugs are fixed.  That's probably simplest given that I have to do some non-NRWT work this week.
Comment 2 Eric Seidel (no email) 2011-08-15 10:22:10 PDT
I've reverted back to ORWT for the Leaks bot for now: http://trac.webkit.org/changeset/93046
Comment 3 Eric Seidel (no email) 2011-08-29 12:18:42 PDT
Is this a blocking issue, or just a polish issue?
Comment 4 Adam Roben (:aroben) 2011-08-29 12:22:20 PDT
Well, it breaks one of our main tools for analyzing the results of the Leaks bot. So I guess you could consider it "blocking".
Comment 5 Eric Seidel (no email) 2011-08-29 13:00:20 PDT
OK.  So what specifically do you need to unbreak the tool?

You need a total count of all leaks found? (which I can get by re-processing all of the *-leaks.txt files at the end).  It seems that since LeaksViewer is already processing all the *-leaks.txt files, it would just count them up itself.

The individual count per-file is printed out in the log as we go.  We just don't print a total summary (except the summarized unique leaks) now.
Comment 6 Adam Roben (:aroben) 2011-08-29 13:25:10 PDT
(In reply to comment #5)
> OK.  So what specifically do you need to unbreak the tool?
> 
> You need a total count of all leaks found? (which I can get by re-processing all of the *-leaks.txt files at the end).

Right.

> It seems that since LeaksViewer is already processing all the *-leaks.txt files, it would just count them up itself.

That would dramatically increase the amount of time it takes for Leaks Viewer's "recent builds" list to appear.
Comment 7 Radar WebKit Bug Importer 2011-10-03 11:25:57 PDT
<rdar://problem/10224916>
Comment 8 Eric Seidel (no email) 2011-10-24 10:41:20 PDT
It looks like the leaks bot got switched back to ORWT.  I'm investigating switching it to NRWT again today.
Comment 9 Eric Seidel (no email) 2011-10-27 13:43:40 PDT
Created attachment 112741 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-10-27 13:53:03 PDT
I believe this change works.  I can't do a full system test (without fixing bug 71059 first), since it seems run-leaks doesn't work on lion?
Comment 11 Adam Roben (:aroben) 2011-10-27 14:13:04 PDT
Comment on attachment 112741 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112741&action=review

> Tools/ChangeLog:12
> +        * Scripts/webkitpy/layout_tests/port/leakdetector.py:
> +        * Scripts/webkitpy/layout_tests/port/leakdetector_unittest.py:
> +        * Scripts/webkitpy/layout_tests/port/mac.py:

Can you add some function-level comments? That would make this patch a lot easier to review.
Comment 12 Eric Seidel (no email) 2011-10-27 14:20:04 PDT
Created attachment 112750 [details]
hopefully this is easier to review
Comment 13 Eric Seidel (no email) 2011-10-27 14:22:35 PDT
Created attachment 112751 [details]
include code to turn on NRWT for leaks bot
Comment 14 Adam Roben (:aroben) 2011-10-27 14:24:08 PDT
Comment on attachment 112750 [details]
hopefully this is easier to review

View in context: https://bugs.webkit.org/attachment.cgi?id=112750&action=review

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:142
> +        total_bytes_string, unique_leaks = self._leak_detector.count_unique_leaks(leaks_files)

It's a little weird that count_unique_leaks returns more than just a count of unique leaks. I don't have any great naming suggestions.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:143
> +        total_leaks = self._leak_detector.count_total_leaks(leaks_files)

Would it be more efficient to count total and unique leaks in a single function (so you could pass through the leaks files just once)?
Comment 15 Adam Roben (:aroben) 2011-10-27 14:25:43 PDT
Comment on attachment 112751 [details]
include code to turn on NRWT for leaks bot

The run-webkit-tests changes are missing from your ChangeLog. But they should probably be done separately anyway. r=me on that bit. See my comments on the old patch for the Python changes.
Comment 16 Eric Seidel (no email) 2011-10-27 14:27:11 PDT
(In reply to comment #14)
> (From update of attachment 112750 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112750&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:142
> > +        total_bytes_string, unique_leaks = self._leak_detector.count_unique_leaks(leaks_files)
> 
> It's a little weird that count_unique_leaks returns more than just a count of unique leaks. I don't have any great naming suggestions.

Sure will make it count_total_bytes_and_unique_leaks.  :)

> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:143
> > +        total_leaks = self._leak_detector.count_total_leaks(leaks_files)
> 
> Would it be more efficient to count total and unique leaks in a single function (so you could pass through the leaks files just once)?

Doesn't really matter.  Thats what I had originally, but I figured this might be slightly cleaner.  The unique_leaks function has to call out to parse-malloc-history which has to read the files itself.
Comment 17 Adam Roben (:aroben) 2011-10-27 14:29:25 PDT
Comment on attachment 112750 [details]
hopefully this is easier to review

View in context: https://bugs.webkit.org/attachment.cgi?id=112750&action=review

>>> Tools/Scripts/webkitpy/layout_tests/port/mac.py:143
>>> +        total_leaks = self._leak_detector.count_total_leaks(leaks_files)
>> 
>> Would it be more efficient to count total and unique leaks in a single function (so you could pass through the leaks files just once)?
> 
> Doesn't really matter.  Thats what I had originally, but I figured this might be slightly cleaner.  The unique_leaks function has to call out to parse-malloc-history which has to read the files itself.

Gotcha. The current separation seems fine then.
Comment 18 Eric Seidel (no email) 2011-10-27 14:36:04 PDT
Created attachment 112754 [details]
Patch for landing
Comment 19 Eric Seidel (no email) 2011-10-27 14:38:52 PDT
This landed, svn was just hanging webkit-patch. :(  http://trac.webkit.org/changeset/98639