Bug 66227

Summary: REGRESSION (NRWT): build.webkit.org doesn't show the total number of leaks found during a test run on the Leaks bot
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: Tools / TestsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, ojan, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://build.webkit.org/builders/SnowLeopard%20Intel%20Leaks
Bug Depends on: 71059    
Bug Blocks: 34984    
Attachments:
Description Flags
Patch
none
hopefully this is easier to review
none
include code to turn on NRWT for leaks bot
none
Patch for landing eric: commit-queue+

Adam Roben (:aroben)
Reported 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/>.
Attachments
Patch (7.67 KB, patch)
2011-10-27 13:43 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
hopefully this is easier to review (8.28 KB, patch)
2011-10-27 14:20 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch for landing (9.82 KB, patch)
2011-10-27 14:36 PDT, Eric Seidel (no email) 		eric: commit-queue+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 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.
Eric Seidel (no email)
Comment 2 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
Eric Seidel (no email)
Comment 3 2011-08-29 12:18:42 PDT
Is this a blocking issue, or just a polish issue?
Adam Roben (:aroben)
Comment 4 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".
Eric Seidel (no email)
Comment 5 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.
Adam Roben (:aroben)
Comment 6 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.
Radar WebKit Bug Importer
Comment 7 2011-10-03 11:25:57 PDT
<rdar://problem/10224916>
Eric Seidel (no email)
Comment 8 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.
Eric Seidel (no email)
Comment 9 2011-10-27 13:43:40 PDT
Created attachment 112741 [details] Patch
Eric Seidel (no email)
Comment 10 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?
Adam Roben (:aroben)
Comment 11 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.
Eric Seidel (no email)
Comment 12 2011-10-27 14:20:04 PDT
Created attachment 112750 [details] hopefully this is easier to review
Eric Seidel (no email)
Comment 13 2011-10-27 14:22:35 PDT
Created attachment 112751 [details] include code to turn on NRWT for leaks bot
Adam Roben (:aroben)
Comment 14 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)?
Adam Roben (:aroben)
Comment 15 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.
Eric Seidel (no email)
Comment 16 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.
Adam Roben (:aroben)
Comment 17 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.
Eric Seidel (no email)
Comment 18 2011-10-27 14:36:04 PDT
Created attachment 112754 [details] Patch for landing
Eric Seidel (no email)
Comment 19 2011-10-27 14:38:52 PDT
This landed, svn was just hanging webkit-patch. :( http://trac.webkit.org/changeset/98639
Note You need to log in before you can comment on or make changes to this bug.