Bug 66227 – REGRESSION (NRWT): build.webkit.org doesn't show the total number of leaks found during a test run on the Leaks bot

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Roben (:aroben)]

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/>.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I've reverted back to ORWT for the Leaks bot for now: http://trac.webkit.org/changeset/93046

[Eric Seidel (no email)]

Is this a blocking issue, or just a polish issue?

[Adam Roben (:aroben)]

Well, it breaks one of our main tools for analyzing the results of the Leaks bot. So I guess you could consider it "blocking".

[Eric Seidel (no email)]

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.

[Adam Roben (:aroben)]

(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.

[Radar WebKit Bug Importer]

<rdar://problem/10224916>

[Eric Seidel (no email)]

It looks like the leaks bot got switched back to ORWT.  I'm investigating switching it to NRWT again today.

[Eric Seidel (no email)]

Created attachment 112741 [details]
Patch

[Eric Seidel (no email)]

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?

[Adam Roben (:aroben)]

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.

[Eric Seidel (no email)]

Created attachment 112750 [details]
hopefully this is easier to review

[Eric Seidel (no email)]

Created attachment 112751 [details]
include code to turn on NRWT for leaks bot

[Adam Roben (:aroben)]

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)?

[Adam Roben (:aroben)]

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.

[Eric Seidel (no email)]

(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.

[Adam Roben (:aroben)]

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.

[Eric Seidel (no email)]

Created attachment 112754 [details]
Patch for landing

[Eric Seidel (no email)]

This landed, svn was just hanging webkit-patch. :(  http://trac.webkit.org/changeset/98639