Bug 63832

This will require a little work. There is a lot of leak detection/interpretation logic in ORWT these days.

Created attachment 101067 [details] wip

Created attachment 101222 [details] wip, makes everything crash

Created attachment 101233 [details] Patch

Created attachment 101245 [details] Patch

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.

Created attachment 101250 [details] Patch

Comment on attachment 101250 [details] Patch Will respond to dirk's comments and re-post.

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

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

Created attachment 101254 [details] Fixed errors pointed out by dirk

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

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

Committed r91245: <http://trac.webkit.org/changeset/91245>