Bug 63832 - new-run-webkit-tests should support --leaks
Summary: new-run-webkit-tests should support --leaks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 34984 64792
  Show dependency treegraph
 
Reported: 2011-07-01 12:33 PDT by WebKit Review Bot
Modified: 2011-07-19 04:34 PDT (History)
4 users (show)

See Also:


Attachments
wip (11.50 KB, patch)
2011-07-15 16:25 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
wip, makes everything crash (28.98 KB, patch)
2011-07-18 15:33 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (38.25 KB, patch)
2011-07-18 16:49 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (38.38 KB, patch)
2011-07-18 17:52 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (39.13 KB, patch)
2011-07-18 18:06 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fixed errors pointed out by dirk (41.20 KB, patch)
2011-07-18 18:27 PDT, Eric Seidel (no email)
dpranke: review+
dpranke: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2011-07-01 12:33:26 PDT
new-run-webkit-tests should support --leaks
Requested by abarth on #webkit.
Comment 1 Eric Seidel (no email) 2011-07-01 12:42:42 PDT
This will require a little work.  There is a lot of leak detection/interpretation logic in ORWT these days.
Comment 2 Eric Seidel (no email) 2011-07-15 16:25:57 PDT
Created attachment 101067 [details]
wip
Comment 3 Eric Seidel (no email) 2011-07-18 15:33:02 PDT
Created attachment 101222 [details]
wip, makes everything crash
Comment 4 Eric Seidel (no email) 2011-07-18 16:49:53 PDT
Created attachment 101233 [details]
Patch
Comment 5 Eric Seidel (no email) 2011-07-18 17:52:03 PDT
Created attachment 101245 [details]
Patch
Comment 6 Dirk Pranke 2011-07-18 17:59:41 PDT
Comment on attachment 101245 [details]
Patch

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

I'm sure you've heard this before, but (as I've frequently been given the feedback myself), mixing whitespace diffs, and code cleanup along with actual changes makes reviewing the actual change harder. It'd be great if you could split the changes into two different patches.

R-'ing because there's at least (I think) one out-and-out typo, and apart from that it's hard for me to tell what the intended logic of the mods to the base class are (i.e., what are the semantics of checking for leaks, when can those methods be called, etc.).

> Tools/Scripts/webkitpy/layout_tests/port/base.py:291
> +    def check_for_leaks(self, process_name, process_pid):

What are the actual semantics of this routine? I assume pid must be alive? What is this supposed to do or return? Is it totally arbitrary?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:296
> +        # while running the layout tests.

Print to where? Is this a log call? what level?

It might be better if the information was just returned so that the caller could decide what to do with it (e.g., maybe this should also be stored in results.json?))

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:43
> +class LeakDetector(object):

I would probably move this into a separate module, e..g, mac_leak_detector.py or something.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:105
> +        return "leaks-%s-%s-%s.txt" % (process_name, process_pid, len(self._leaks_filenames))

Do you need to keep the actual list of filenames? It looks like you're mostly just using this as a counter and maybe that would be clearer?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:107
> +    def parse_leak_files(self):

don't you need to pass in leak_files here?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:121
> +        _log.info(" ? checking for leaks in %s" % process_name)

This might want to be log.debug() ... I think this is called in every worker, right? We probably don't want that being logged all the time.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:141
> +            _log.warn(" + %s leaks (%s bytes) were found, details in %s" % (count, bytes, leaks_output_path))

I'm not sure if you want this to be treated as a warning, or if this should just be info() ?

warnings are used to indicate that something went wrong with NRWT itself, usually (but it was a survivable error).

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:250
> +        _log.warn("%s unique leaks found!" % unique_leaks)

Ugh :(

Fixing this "right" could be annoying, all right. I don't know anything about run-leaks (and have never used the feature in ORWT); does it run when the process is still alive, or after the process exits? Does it pick up incremental leaks since the last run, or anything smart like that?

I can't tell if you are checking for leaks per-test, or per-driver-instance, or something else, so it's hard to say what the "right" way to fix this is.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:456
>      def restart(self):

Is restart() actually called by anything()? I didn't see any callers in the currently checked-in code. Does this patch call it from somewhere? If not, you can probably just delete this whole function.
Comment 7 Eric Seidel (no email) 2011-07-18 18:06:20 PDT
Created attachment 101250 [details]
Patch
Comment 8 Eric Seidel (no email) 2011-07-18 18:07:45 PDT
Comment on attachment 101250 [details]
Patch

Will respond to dirk's comments and re-post.
Comment 9 Eric Seidel (no email) 2011-07-18 18:13:58 PDT
(In reply to comment #6)
> (From update of attachment 101245 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101245&action=review
> 
> I'm sure you've heard this before, but (as I've frequently been given the feedback myself), mixing whitespace diffs, and code cleanup along with actual changes makes reviewing the actual change harder. It'd be great if you could split the changes into two different patches.

Agreed.  Will do.

> R-'ing because there's at least (I think) one out-and-out typo, and apart from that it's hard for me to tell what the intended logic of the mods to the base class are (i.e., what are the semantics of checking for leaks, when can those methods be called, etc.).
>
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:291
> > +    def check_for_leaks(self, process_name, process_pid):
> 
> What are the actual semantics of this routine? I assume pid must be alive? What is this supposed to do or return? Is it totally arbitrary?

Yup.  Pid must be alive.  Otherwise undefined.

> > Tools/Scripts/webkitpy/layout_tests/port/base.py:296
> > +        # while running the layout tests.
> 
> Print to where? Is this a log call? what level?
> 
> It might be better if the information was just returned so that the caller could decide what to do with it (e.g., maybe this should also be stored in results.json?))
> 
> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:43
> > +class LeakDetector(object):
> 
> I would probably move this into a separate module, e..g, mac_leak_detector.py or something.

I certainly could.  The more I think about it, the more it may belong closer to executive.  It has little to do with "port" except for the excusion lists and where to find run-leaks/parse-malloc-history.

> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:105
> > +        return "leaks-%s-%s-%s.txt" % (process_name, process_pid, len(self._leaks_filenames))
> 
> Do you need to keep the actual list of filenames? It looks like you're mostly just using this as a counter and maybe that would be clearer?

I've fixed this in my most recent patch.  ORWT kept a global list and used it in various places.  That doesn't work for NRWT's multi-worker setup, so slowly that code has been dying, and is now finally dead. :)

> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:107
> > +    def parse_leak_files(self):
> 
> don't you need to pass in leak_files here?

Thx.  Will unittest.

> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:121
> > +        _log.info(" ? checking for leaks in %s" % process_name)
> 
> This might want to be log.debug() ... I think this is called in every worker, right? We probably don't want that being logged all the time.

Agreed.  I also got rid of the ? and + from these logs.  Just more leftover from the ORWT code.

> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:141
> > +            _log.warn(" + %s leaks (%s bytes) were found, details in %s" % (count, bytes, leaks_output_path))
> 
> I'm not sure if you want this to be treated as a warning, or if this should just be info() ?

Donno.  Ideally i'd like to print it through the printer, I would think?  Except workers don't seem to have a printer object.

> warnings are used to indicate that something went wrong with NRWT itself, usually (but it was a survivable error).

I see.  I'll make it info then.  Info will be printed by default I assume?  We need leak summary information, etc. to be printed by default.

> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:250
> > +        _log.warn("%s unique leaks found!" % unique_leaks)
> 
> Ugh :(
> 
> Fixing this "right" could be annoying, all right. I don't know anything about run-leaks (and have never used the feature in ORWT); does it run when the process is still alive, or after the process exits? Does it pick up incremental leaks since the last run, or anything smart like that?

I'm not sure I understand.  what's "wrong" about it?

> I can't tell if you are checking for leaks per-test, or per-driver-instance, or something else, so it's hard to say what the "right" way to fix this is.

Per server/driver instance.

> > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:456
> >      def restart(self):
> 
> Is restart() actually called by anything()? I didn't see any callers in the currently checked-in code. Does this patch call it from somewhere? If not, you can probably just delete this whole function.

Done.
Comment 10 Dirk Pranke 2011-07-18 18:24:46 PDT
(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 101245 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=101245&action=review
> > 
> > I'm sure you've heard this before, but (as I've frequently been given the feedback myself), mixing whitespace diffs, and code cleanup along with actual changes makes reviewing the actual change harder. It'd be great if you could split the changes into two different patches.
> 
> Agreed.  Will do.
> 
> > R-'ing because there's at least (I think) one out-and-out typo, and apart from that it's hard for me to tell what the intended logic of the mods to the base class are (i.e., what are the semantics of checking for leaks, when can those methods be called, etc.).
> >
> > > Tools/Scripts/webkitpy/layout_tests/port/base.py:291
> > > +    def check_for_leaks(self, process_name, process_pid):
> > 
> > What are the actual semantics of this routine? I assume pid must be alive? What is this supposed to do or return? Is it totally arbitrary?
> 
> Yup.  Pid must be alive.  Otherwise undefined.
> 
> > > Tools/Scripts/webkitpy/layout_tests/port/base.py:296
> > > +        # while running the layout tests.
> > 
> > Print to where? Is this a log call? what level?
> > 
> > It might be better if the information was just returned so that the caller could decide what to do with it (e.g., maybe this should also be stored in results.json?))
> > 
> > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:43
> > > +class LeakDetector(object):
> > 
> > I would probably move this into a separate module, e..g, mac_leak_detector.py or something.
> 
> I certainly could.  The more I think about it, the more it may belong closer to executive.  It has little to do with "port" except for the excusion lists and where to find run-leaks/parse-malloc-history.
> 

Yeah, I was thinking it belonged in common.system somewhere as well.

> > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:105
> > > +        return "leaks-%s-%s-%s.txt" % (process_name, process_pid, len(self._leaks_filenames))
> > 
> > Do you need to keep the actual list of filenames? It looks like you're mostly just using this as a counter and maybe that would be clearer?
> 
> I've fixed this in my most recent patch.  ORWT kept a global list and used it in various places.  That doesn't work for NRWT's multi-worker setup, so slowly that code has been dying, and is now finally dead. :)
> 

Okay.

> > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:107
> > > +    def parse_leak_files(self):
> > 
> > don't you need to pass in leak_files here?
> 
> Thx.  Will unittest.
> 
> > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:121
> > > +        _log.info(" ? checking for leaks in %s" % process_name)
> > 
> > This might want to be log.debug() ... I think this is called in every worker, right? We probably don't want that being logged all the time.
> 
> Agreed.  I also got rid of the ? and + from these logs.  Just more leftover from the ORWT code.
> 
> > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:141
> > > +            _log.warn(" + %s leaks (%s bytes) were found, details in %s" % (count, bytes, leaks_output_path))
> > 
> > I'm not sure if you want this to be treated as a warning, or if this should just be info() ?
> 
> Donno.  Ideally i'd like to print it through the printer, I would think?  Except workers don't seem to have a printer object.
> 

Yeah, workers don't have access to the printer object. Not coincidentally, we don't usually ask the workers to print anything :) I've normally tried to avoid that, but it may not always be possible in the future.

> > warnings are used to indicate that something went wrong with NRWT itself, usually (but it was a survivable error).
> 
> I see.  I'll make it info then.  Info will be printed by default I assume?  We need leak summary information, etc. to be printed by default.
> 

Yes, info() is printed by default.

> > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:250
> > > +        _log.warn("%s unique leaks found!" % unique_leaks)
> > 
> > Ugh :(
> > 
> > Fixing this "right" could be annoying, all right. I don't know anything about run-leaks (and have never used the feature in ORWT); does it run when the process is still alive, or after the process exits? Does it pick up incremental leaks since the last run, or anything smart like that?
> 
> I'm not sure I understand.  what's "wrong" about it?
> 

There may not be anything "wrong" about it, which is why it was in quotes. Mostly I was referring to it being "wrong" in the sense that you were also calling it a hack. Without knowing what the intended behavior was, it was hard to say what the right way to implement it would be.

> > I can't tell if you are checking for leaks per-test, or per-driver-instance, or something else, so it's hard to say what the "right" way to fix this is.
> 
> Per server/driver instance.
> 

Hm. Okay. There is no mechanism to report "per-server/driver instance"-level data anywhere. i.e., there is neither a way to send a message from worker -> manager, nor a way to summarize per-worker data in the results objects (results.json, etc.). Perhaps there should be in the future, and we could also create a "worker_data" message that was sent from worker to manager. 

Writing stuff to the filesystem isn't too bad, as long as it's not being used as a communication mechanism to get data back into the manager for further processing (AFAIK, we don't do that anywhere). If the manager is just reading data and logging it, that's less objectionable, although I have normally tried not to rely on the filesystem for anything at all. Architecturally I was tempted to never even write to the filesystem from the workers, but (a) that would force the manager to do a lot more work and (b) I wasn't sure how sending images, diffs, etc. across the message queue would even scale.
Comment 11 Eric Seidel (no email) 2011-07-18 18:27:55 PDT
Created attachment 101254 [details]
Fixed errors pointed out by dirk
Comment 12 Eric Seidel (no email) 2011-07-18 18:28:29 PDT
I've not split out the unrelated cleanups yet, but I will if no one is brave enough to review it as-is in the next 12 hours or so. :)

Dinner time for me.
Comment 13 Dirk Pranke 2011-07-18 18:46:10 PDT
Comment on attachment 101254 [details]
Fixed errors pointed out by dirk

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

change basically looks fine. R+-ing / CQ-'ing assuming you will clean this stuff up.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:291
> +    def check_for_leaks(self, process_name, process_pid):

Please add comments to this and print_leaks_summary() about what the routines should do (what the contracts are); you answered in the bug comments, but I would like those answers to be in the code.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:42
> +# If other ports/platforms decide to support --leaks, we should see about sharing as much of this code as possible.

You are going to move this into a different file, right?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:136
> +            _log.info("%s leaks (%s bytes including %s excluded leaks) were found, details in %s" % (adjusted_count, bytes, excluded, leaks_output_path))

Were you going to change these to log.debug() also? These are printed out by the worker while the driver is actively running, right? Do you want these to be printed mid-run? (I'm not sure if this is intentional or not). I don't really have a problem with this being printed mid-run (it's kind of like an unexpected failure), but it would be good to figure out how to send this data from worker to manager to not have to log from the worker. You might want this to be a FIXME or something. Also note that you might want to log a blank line first in order to deal with the overlapping of progress-meter log messages from the manager.
Comment 14 Eric Seidel (no email) 2011-07-19 02:14:55 PDT
(In reply to comment #13)
> (From update of attachment 101254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101254&action=review
> 
> change basically looks fine. R+-ing / CQ-'ing assuming you will clean this stuff up.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:291
> > +    def check_for_leaks(self, process_name, process_pid):
> 
> Please add comments to this and print_leaks_summary() about what the routines should do (what the contracts are); you answered in the bug comments, but I would like those answers to be in the code.

Will fix.  THanks.

> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:42
> > +# If other ports/platforms decide to support --leaks, we should see about sharing as much of this code as possible.
> 
> You are going to move this into a different file, right?

I'm just going to leave it a FIXME for now.

> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:136
> > +            _log.info("%s leaks (%s bytes including %s excluded leaks) were found, details in %s" % (adjusted_count, bytes, excluded, leaks_output_path))
> 
> Were you going to change these to log.debug() also? These are printed out by the worker while the driver is actively running, right? Do you want these to be printed mid-run? (I'm not sure if this is intentional or not). I don't really have a problem with this being printed mid-run (it's kind of like an unexpected failure), but it would be good to figure out how to send this data from worker to manager to not have to log from the worker.

Yes, they're expected to be printed mid-run.  (When the worker is spinning down.).  Ideally yes, we'd send the data back and the manager would print it, but there seems to be no way to do that.

> You might want this to be a FIXME or something. Also note that you might want to log a blank line first in order to deal with the overlapping of progress-meter log messages from the manager.
Comment 15 Eric Seidel (no email) 2011-07-19 02:25:55 PDT
Committed r91245: <http://trac.webkit.org/changeset/91245>