Chromium Code Reviews
|
|
DescriptionAdd an additional content_shell per worker for running virtual tests.
The secondary content_shell will only be used for rendering the virtual test's references, and only if the references use default flags.
The secondary content_shell is lazily initialized and shutdown at the end of each test suite.
BUG=471431
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196445
Patch Set 1 #Patch Set 2 : Add integration test for virtual tests with default reference args #
Total comments: 6
Patch Set 3 : use extra content shell for references with default args instead #Patch Set 4 : remove DriverHandler and driver preparation/reconstruction logic #Patch Set 5 : fix docstring for Driver.run_test() #
Total comments: 2
Messages
Total messages: 29 (3 generated)
joelo@chromium.org changed reviewers: + dpranke@chromium.org, jsbell@chromium.org, ojan@chromium.org
dpranke@, ojan@, jsbell@: ptal?
I have some general comments on the approach in the CL first; once we get that sorted out, I'll probably have some additional feedback on the details of the change. https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py (right): https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:355: """Maintains the lifecycle of a driver for a given worker. I'm not seeing what the value of this class is; it seems like it is just duplicating the logic that already exists in the Driver class? In particular, the contract for the driver class says that driver.run_test() is responsible for starting a new content_shell if and when it is needed, so the logic you have here in prepare() seems unneeded? I'm probably missing something in your thinking? https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py (right): https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:83: self._reference_driver = virtual_driver If I'm following this change correctly, this means that any worker that ever runs both regular (non-virtual) tests and virtual tests will start up and use two workers, rather than restarting when we switch from a non-virtual shard to a virtual shard. This means, I think, that we will have fewer restarts of the regular content_shell, but we will more often be switching between the two different drivers (in addition to switching back and forth) during a virtual suite when the the reftest doesn't use args, we'll switch back and forth between shards). Do you have any sense of the perf impact of this? I don't really know how much of a hit we pay for restarts between shards compared to the hit of having more warm content_shells.
https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py (right): https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:83: self._reference_driver = virtual_driver On 2015/06/01 at 22:06:20, Dirk Pranke wrote: > If I'm following this change correctly, this means that any worker that ever runs both regular (non-virtual) tests and virtual tests will start up and use two workers, rather than restarting when we switch from a non-virtual shard to a virtual shard. > > This means, I think, that we will have fewer restarts of the regular content_shell, but we will more often be switching between the two different drivers (in addition to switching back and forth) during a virtual suite when the the reftest doesn't use args, we'll switch back and forth between shards). > > Do you have any sense of the perf impact of this? Why would switching between different drivers have a perf impact? > I don't really know how much of a hit we pay for restarts between shards compared to the hit of having more warm content_shells. You're saying that maybe we should kill the extra driver when we finish with a shard?
On 2015/06/01 23:19:54, ojan wrote: > https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... > File Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py > (right): > > https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... > Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:83: > self._reference_driver = virtual_driver > On 2015/06/01 at 22:06:20, Dirk Pranke wrote: > > If I'm following this change correctly, this means that any worker that ever > runs both regular (non-virtual) tests and virtual tests will start up and use > two workers, rather than restarting when we switch from a non-virtual shard to a > virtual shard. > > > > This means, I think, that we will have fewer restarts of the regular > content_shell, but we will more often be switching between the two different > drivers (in addition to switching back and forth) during a virtual suite when > the the reftest doesn't use args, we'll switch back and forth between shards). > > > > Do you have any sense of the perf impact of this? > > Why would switching between different drivers have a perf impact? paging, swapping, memory pressure, etc. (potentially, at least). Also, perhaps if the "idle" drivers aren't really idle (background gc kicks in or something). > > > I don't really know how much of a hit we pay for restarts between shards > compared to the hit of having more warm content_shells. > > You're saying that maybe we should kill the extra driver when we finish with a > shard? That might actually be the worst of both worlds. I think we probably just need to do some benchmarking of the different options and see what happens.
On 2015/06/01 at 23:27:14, dpranke wrote: > On 2015/06/01 23:19:54, ojan wrote: > > > I don't really know how much of a hit we pay for restarts between shards > > compared to the hit of having more warm content_shells. > > > > You're saying that maybe we should kill the extra driver when we finish with a > > shard? > > That might actually be the worst of both worlds. I think we probably just need to > do some benchmarking of the different options and see what happens. I'm not sure what the other options are. The only ones I can think of are the current approach of having both remain alive and the one of killing the extra driver.
On 2015/06/01 23:42:46, ojan wrote: > On 2015/06/01 at 23:27:14, dpranke wrote: > > On 2015/06/01 23:19:54, ojan wrote: > > > > I don't really know how much of a hit we pay for restarts between shards > > > compared to the hit of having more warm content_shells. > > > > > > You're saying that maybe we should kill the extra driver when we finish with > a > > > shard? > > > > That might actually be the worst of both worlds. I think we probably just need > to > > do some benchmarking of the different options and see what happens. > > I'm not sure what the other options are. The only ones I can think of are the > current approach of having both remain alive and the one of killing the extra > driver. IIUC, we're not just restarting the content_shell between virtual and non virtual shards, but (twice) between every test in a virtual test suite that uses default reference args (it needs to render the virtual output, followed by the reference with different flags, and restart again for the the next virtual test, etc). There was a significant improvement when I ran this last week (virtual tests are 2-3 times faster than without the tweak, using 8 workers on my machine). We have since skipped more of the virtual tests (presumably to speed up cycle times), so that's dropped down to 20-30% improvement. I'm not 100% sure about how it affects the try servers, will do more testing.
https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py (right): https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:355: """Maintains the lifecycle of a driver for a given worker. On 2015/06/01 22:06:20, Dirk Pranke wrote: > I'm not seeing what the value of this class is; it seems like it is just > duplicating the logic that already exists in the Driver class? > > In particular, the contract for the driver class says that driver.run_test() is > responsible for starting a new content_shell if and when it is needed, so the > logic you have here in prepare() seems unneeded? > > I'm probably missing something in your thinking? There's actually no new logic here -- the worker class above now needs to keep track of the state of 2 drivers, so instead of duplicating the logic I encapsulated it in a separate class. prepare() also takes care of restarting the driver/content_shell if it has crashed. According to it's docstring, driver.run_test() can leave the driver in an 'indeterminate state' if a test crashes/times out. I also peeked at the code and AFAICT it doesn't appear to handle restarting the driver..
On 2015/06/01 23:42:46, ojan wrote: > On 2015/06/01 at 23:27:14, dpranke wrote: > > On 2015/06/01 23:19:54, ojan wrote: > > > > I don't really know how much of a hit we pay for restarts between shards > > > compared to the hit of having more warm content_shells. > > > > > > You're saying that maybe we should kill the extra driver when we finish with > a > > > shard? > > > > That might actually be the worst of both worlds. I think we probably just need > to > > do some benchmarking of the different options and see what happens. > > I'm not sure what the other options are. The only ones I can think of are the > current approach of having both remain alive and the one of killing the extra > driver. option 1: (the approach prior to this change): run one driver, restart it when the arguments change option 2: run one driver, use it for non-virtual tests and most virtual tests; only use a second driver just for the virtual test references w/ no arguments option 3: (this CL) run two drivers, run non-virtual tests and virtual references w/ no arguments w/ the main driver, use a second driver for all other virtual tests option 4: (your suggestion, if I understand it correctly) option #3 but stop the second driver at the end of each shard Each might have advantages and disadvantages. Option 3 seems to have the fewest restarts, which probably makes the biggest difference, but I can imagine a situation where option 2 might be better (though I think it's unlikely). I suppose I can imagine a situation where option 4 might also be better, if we were really pressed for memory. There might be other options as well, but it's probably not worth looking hard for them.
On 2015/06/02 at 18:57:48, dpranke wrote: > On 2015/06/01 23:42:46, ojan wrote: > > On 2015/06/01 at 23:27:14, dpranke wrote: > > > On 2015/06/01 23:19:54, ojan wrote: > > > > > I don't really know how much of a hit we pay for restarts between shards > > > > compared to the hit of having more warm content_shells. > > > > > > > > You're saying that maybe we should kill the extra driver when we finish with > > a > > > > shard? > > > > > > That might actually be the worst of both worlds. I think we probably just need > > to > > > do some benchmarking of the different options and see what happens. > > > > I'm not sure what the other options are. The only ones I can think of are the > > current approach of having both remain alive and the one of killing the extra > > driver. > > option 1: (the approach prior to this change): run one driver, restart it when the arguments change > > option 2: run one driver, use it for non-virtual tests and most virtual tests; only use a second driver > just for the virtual test references w/ no arguments > > option 3: (this CL) run two drivers, run non-virtual tests and virtual references w/ no arguments w/ > the main driver, use a second driver for all other virtual tests > > option 4: (your suggestion, if I understand it correctly) option #3 but stop the second driver at the > end of each shard > > Each might have advantages and disadvantages. Option 3 seems to have the fewest restarts, > which probably makes the biggest difference, but I can imagine a situation where option 2 > might be better (though I think it's unlikely). I suppose I can imagine a situation where option 4 > might also be better, if we were really pressed for memory. > > There might be other options as well, but it's probably not worth looking hard for them. Do you want joel to try out all four options and measure? I was thinking we'd just do option 3 and see how it works in practice. That wouldn't tell us if there were a better choice though. Maybe it's worth trying option 2? IMO, option 4 isn't even worth trying. I only suggested it because I thought you were suggesting it. :)
https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py (right): https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:355: """Maintains the lifecycle of a driver for a given worker. On 2015/06/02 00:07:04, joelo wrote: > On 2015/06/01 22:06:20, Dirk Pranke wrote: > > I'm not seeing what the value of this class is; it seems like it is just > > duplicating the logic that already exists in the Driver class? > > > > In particular, the contract for the driver class says that driver.run_test() > is > > responsible for starting a new content_shell if and when it is needed, so the > > logic you have here in prepare() seems unneeded? > > > > I'm probably missing something in your thinking? > > There's actually no new logic here -- the worker class above now needs to keep > track of the state of 2 drivers, so instead of duplicating the logic I > encapsulated it in a separate class. > > prepare() also takes care of restarting the driver/content_shell if it has > crashed. According to it's docstring, driver.run_test() can leave the driver in > an 'indeterminate state' if a test crashes/times out. I also peeked at the code > and AFAICT it doesn't appear to handle restarting the driver.. Oh, I see the confusion. The docstring is wrong :( run_test() does not leave the child process in an indeterminate state; if the test completed normally, the child is still running. If the test crashed, the child is gone. If the test times out, the driver kills the child (so it is also gone). (see driver.py:182). run_test() is responsible for starting a child if need be, so I think prepare() is doing nothing useful. Then, assuming that's true, your wrapper class doesn't really do much except move lines around in the file. I don't think it's encapsulating enough to be useful at that point. Does that make sense? (Also, please update the docstring to match reality as a part of this).
On 2015/06/02 19:03:33, ojan wrote: > Do you want joel to try out all four options and measure? I was thinking we'd > just do option 3 and see how it works in practice. That wouldn't tell us if > there were a better choice though. Maybe it's worth trying option 2? IMO, option > 4 isn't even worth trying. I only suggested it because I thought you were > suggesting it. :) I'm sorry, I guess I wasn't clear enough. I guess I was asking if he had any sense yet of the perf impact (hopefully improvements) for #3 over #1, and possibly if he was getting enough of a feel for how the variables interact to see if he had a guess as to how #2 might behave. I don't know that we need formal tests unless we find that #3 causes problems elsewhere due to the impact of having more shells running and consuming memory when idle. (I don't think we need to look at #4 at all at this time, either).
On 2015/06/02 at 19:12:40, dpranke wrote: > On 2015/06/02 19:03:33, ojan wrote: > > Do you want joel to try out all four options and measure? I was thinking we'd > > just do option 3 and see how it works in practice. That wouldn't tell us if > > there were a better choice though. Maybe it's worth trying option 2? IMO, option > > 4 isn't even worth trying. I only suggested it because I thought you were > > suggesting it. :) > > I'm sorry, I guess I wasn't clear enough. > > I guess I was asking if he had any sense yet of the perf impact (hopefully improvements) for > #3 over #1, and possibly if he was getting enough of a feel for how the variables interact > to see if he had a guess as to how #2 might behave. > > I don't know that we need formal tests unless we find that #3 causes problems elsewhere > due to the impact of having more shells running and consuming memory when idle. > > (I don't think we need to look at #4 at all at this time, either). OK. I think we're on the same page. joel said: "There was a significant improvement when I ran this last week (virtual tests are 2-3 times faster than without the tweak, using 8 workers on my machine). We have since skipped more of the virtual tests (presumably to speed up cycle times), so that's dropped down to 20-30% improvement. I'm not 100% sure about how it affects the try servers, will do more testing." So, that answers the #3 over #1 question, but not the guess of how #2 might behave. joel?
On 2015/06/02 19:52:32, ojan wrote: > On 2015/06/02 at 19:12:40, dpranke wrote: > > On 2015/06/02 19:03:33, ojan wrote: > > > Do you want joel to try out all four options and measure? I was thinking > we'd > > > just do option 3 and see how it works in practice. That wouldn't tell us if > > > there were a better choice though. Maybe it's worth trying option 2? IMO, > option > > > 4 isn't even worth trying. I only suggested it because I thought you were > > > suggesting it. :) > > > > I'm sorry, I guess I wasn't clear enough. > > > > I guess I was asking if he had any sense yet of the perf impact (hopefully > improvements) for > > #3 over #1, and possibly if he was getting enough of a feel for how the > variables interact > > to see if he had a guess as to how #2 might behave. > > > > I don't know that we need formal tests unless we find that #3 causes problems > elsewhere > > due to the impact of having more shells running and consuming memory when > idle. > > > > (I don't think we need to look at #4 at all at this time, either). > > OK. I think we're on the same page. > > joel said: "There was a significant improvement when I ran this last week > (virtual tests are 2-3 times faster than without the tweak, using 8 workers on > my machine). We have since skipped more of the virtual tests (presumably to > speed up cycle times), so that's dropped down to 20-30% improvement. I'm not > 100% sure about how it affects the try servers, will do more testing." > > So, that answers the #3 over #1 question, but not the guess of how #2 might > behave. joel? So, we only use default args for slimmingpaint, most of which we currently skip. But if we were to run them all without the spurious restarts it should take roughly 20% of the execution time (see http://test-results.appspot.com/dashboards/treemap.html#treemapfocus=AllTests...), during which we need the extra content_shell. Option #2 would give us some memory benefit for the remaining 80% provided we shut down the 2nd driver at the end of each virtual test suite. And doing so wouldn't add any overhead (from restarts) since there are only 6 slimmingpaint suites (less than the # of workers). That's actually just a small tweak to this CL, let me try that.
https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py (right): https://codereview.chromium.org/1161863003/diff/20001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:355: """Maintains the lifecycle of a driver for a given worker. On 2015/06/02 19:06:04, Dirk Pranke wrote: > On 2015/06/02 00:07:04, joelo wrote: > > On 2015/06/01 22:06:20, Dirk Pranke wrote: > > > I'm not seeing what the value of this class is; it seems like it is just > > > duplicating the logic that already exists in the Driver class? > > > > > > In particular, the contract for the driver class says that driver.run_test() > > is > > > responsible for starting a new content_shell if and when it is needed, so > the > > > logic you have here in prepare() seems unneeded? > > > > > > I'm probably missing something in your thinking? > > > > There's actually no new logic here -- the worker class above now needs to keep > > track of the state of 2 drivers, so instead of duplicating the logic I > > encapsulated it in a separate class. > > > > prepare() also takes care of restarting the driver/content_shell if it has > > crashed. According to it's docstring, driver.run_test() can leave the driver > in > > an 'indeterminate state' if a test crashes/times out. I also peeked at the > code > > and AFAICT it doesn't appear to handle restarting the driver.. > > Oh, I see the confusion. > > The docstring is wrong :( run_test() does not leave the child process in an > indeterminate state; if the test completed normally, the child is still running. > If the test crashed, the child is gone. If the test times out, the driver kills > the child (so it is also gone). (see driver.py:182). > > run_test() is responsible for starting a child if need be, so I think prepare() > is doing nothing useful. Then, assuming that's true, your wrapper class doesn't > really do much except move lines around in the file. I don't think it's > encapsulating enough to be useful at that point. > > Does that make sense? > > (Also, please update the docstring to match reality as a part of this). Ah, I see how the driver gets reset now. Let me remove the prepare logic and make sure that it doesn't break anything (since it was already there before).
On 2015/06/02 23:42:30, joelo wrote: > On 2015/06/02 19:52:32, ojan wrote: > > On 2015/06/02 at 19:12:40, dpranke wrote: > > > On 2015/06/02 19:03:33, ojan wrote: > > > > Do you want joel to try out all four options and measure? I was thinking > > we'd > > > > just do option 3 and see how it works in practice. That wouldn't tell us > if > > > > there were a better choice though. Maybe it's worth trying option 2? IMO, > > option > > > > 4 isn't even worth trying. I only suggested it because I thought you were > > > > suggesting it. :) > > > > > > I'm sorry, I guess I wasn't clear enough. > > > > > > I guess I was asking if he had any sense yet of the perf impact (hopefully > > improvements) for > > > #3 over #1, and possibly if he was getting enough of a feel for how the > > variables interact > > > to see if he had a guess as to how #2 might behave. > > > > > > I don't know that we need formal tests unless we find that #3 causes > problems > > elsewhere > > > due to the impact of having more shells running and consuming memory when > > idle. > > > > > > (I don't think we need to look at #4 at all at this time, either). > > > > OK. I think we're on the same page. > > > > joel said: "There was a significant improvement when I ran this last week > > (virtual tests are 2-3 times faster than without the tweak, using 8 workers on > > my machine). We have since skipped more of the virtual tests (presumably to > > speed up cycle times), so that's dropped down to 20-30% improvement. I'm not > > 100% sure about how it affects the try servers, will do more testing." > > > > So, that answers the #3 over #1 question, but not the guess of how #2 might > > behave. joel? > > So, we only use default args for slimmingpaint, most of which we currently skip. > But if we were to run them all without the spurious restarts it should take > roughly 20% of the execution time (see > http://test-results.appspot.com/dashboards/treemap.html#treemapfocus=AllTests...), > during which we need the extra content_shell. Option #2 would give us some > memory benefit for the remaining 80% provided we shut down the 2nd driver at the > end of each virtual test suite. And doing so wouldn't add any overhead (from > restarts) since there are only 6 slimmingpaint suites (less than the # of > workers). That's actually just a small tweak to this CL, let me try that. Whoops, that wasn't entirely accurate -- the main driver would still be restarted for each virtual test suite, not just the sliming paint ones. Still, it might not add much overhead as compared to restarting every test.
I did some testing locally, here are some numbers (8 workers, virtual tests only): Before: 8m4.704s Found 20979 tests; running 6303, skipping 14676. 6301 tests ran as expected (6239 passed, 62 didn't), 2 didn't: Option #2: 6m0.720s Found 20979 tests; running 6303, skipping 14676. 6301 tests ran as expected (6241 passed, 60 didn't), 2 didn't: And including most of the skipped virtual tests: Before: 47m33.796s Found 20979 tests; running 20316, skipping 663. 20273 tests ran as expected (20209 passed, 64 didn't), 43 didn't: Option #2: 18m47.881s Found 20979 tests; running 20316, skipping 663. 20204 tests ran as expected (20141 passed, 63 didn't), 112 didn't: Option #3: 17m46.062s Found 20979 tests; running 20316, skipping 663. 20269 tests ran as expected (20207 passed, 62 didn't), 47 didn't:
dpranke@, ojan@, ptal? Note that the drivers in Worker are no longer reconstructed after every crash/timeout, instead we create them once in start() and trust them to handle crashes correctly. I also removed the driver not-None check right after the driver is created (line 280) -- the FIXME indicates that it handles a device crashing but I don't see how that might happen. Am I missing something?
lgtm w/ one nit. https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py (right): https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:255: self._kill_driver(self._secondary_driver, 'secondary') Do we need to do this? Neither ojan nor I seemed to think it would be worth it.
Thanks dpranke@! https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py (right): https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:255: self._kill_driver(self._secondary_driver, 'secondary') On 2015/06/03 19:03:40, Dirk Pranke wrote: > Do we need to do this? Neither ojan nor I seemed to think it would be worth it. This was the only thing that gave option #2 (this CL now) an advantage over option #3 -- We only need the extra content_shell once per worker (6 slimmingpaint test suites / 8 workers), so shutting it down would free up memory for the rest of the shards.
On 2015/06/04 00:53:26, joelo wrote: > Thanks dpranke@! > > https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... > File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py > (right): > > https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:255: > self._kill_driver(self._secondary_driver, 'secondary') > On 2015/06/03 19:03:40, Dirk Pranke wrote: > > Do we need to do this? Neither ojan nor I seemed to think it would be worth > it. > > This was the only thing that gave option #2 (this CL now) an advantage over > option #3 -- We only need the extra content_shell once per worker (6 > slimmingpaint test suites / 8 workers), so shutting it down would free up memory > for the rest of the shards. That's assuming that each shard does get to a different worker, which may or may not be the case. Regardless, I thought the logic in this CL was still option #3, but adding the shutdown at the end of the shard turns it into option #4; I see now that I missed a "not", and it is still option #2 (or the option #4 variant of #2). I'm not sure it matters much either way.
On 2015/06/04 01:00:00, Dirk Pranke wrote: > On 2015/06/04 00:53:26, joelo wrote: > > Thanks dpranke@! > > > > > https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... > > File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py > > (right): > > > > > https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... > > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:255: > > self._kill_driver(self._secondary_driver, 'secondary') > > On 2015/06/03 19:03:40, Dirk Pranke wrote: > > > Do we need to do this? Neither ojan nor I seemed to think it would be worth > > it. > > > > This was the only thing that gave option #2 (this CL now) an advantage over > > option #3 -- We only need the extra content_shell once per worker (6 > > slimmingpaint test suites / 8 workers), so shutting it down would free up > memory > > for the rest of the shards. > > That's assuming that each shard does get to a different worker, which may or may > not be the case. They should be assuming we always keep the alpha sorting, but restarting once or twice shouldn't make much difference. > > Regardless, I thought the logic in this CL was still option #3, but adding the > shutdown at the end > of the shard turns it into option #4; I see now that I missed a "not", and it is > still option #2 (or the option #4 > variant of #2). > > I'm not sure it matters much either way. Ah sorry, should have clarified with the update.
On 2015/06/04 01:15:12, joelo wrote: > On 2015/06/04 01:00:00, Dirk Pranke wrote: > > On 2015/06/04 00:53:26, joelo wrote: > > > Thanks dpranke@! > > > > > > > > > https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... > > > File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py > > > (right): > > > > > > > > > https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... > > > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:255: > > > self._kill_driver(self._secondary_driver, 'secondary') > > > On 2015/06/03 19:03:40, Dirk Pranke wrote: > > > > Do we need to do this? Neither ojan nor I seemed to think it would be > worth > > > it. > > > > > > This was the only thing that gave option #2 (this CL now) an advantage over > > > option #3 -- We only need the extra content_shell once per worker (6 > > > slimmingpaint test suites / 8 workers), so shutting it down would free up > > memory > > > for the rest of the shards. > > > > That's assuming that each shard does get to a different worker, which may or > may > > not be the case. > They should be assuming we always keep the alpha sorting, but restarting > once or twice shouldn't make much difference. It depends on whether the other workers are already busy w/ other shards (unless you've moved these to the front of the queue in a CL I missed?). But yeah, I don't think it matters much.
On 2015/06/04 01:25:07, Dirk Pranke wrote: > On 2015/06/04 01:15:12, joelo wrote: > > On 2015/06/04 01:00:00, Dirk Pranke wrote: > > > On 2015/06/04 00:53:26, joelo wrote: > > > > Thanks dpranke@! > > > > > > > > > > > > > > https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... > > > > File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1161863003/diff/80001/Tools/Scripts/webkitpy/... > > > > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:255: > > > > self._kill_driver(self._secondary_driver, 'secondary') > > > > On 2015/06/03 19:03:40, Dirk Pranke wrote: > > > > > Do we need to do this? Neither ojan nor I seemed to think it would be > > worth > > > > it. > > > > > > > > This was the only thing that gave option #2 (this CL now) an advantage > over > > > > option #3 -- We only need the extra content_shell once per worker (6 > > > > slimmingpaint test suites / 8 workers), so shutting it down would free up > > > memory > > > > for the rest of the shards. > > > > > > That's assuming that each shard does get to a different worker, which may or > > may > > > not be the case. > > They should be assuming we always keep the alpha sorting, but restarting > > once or twice shouldn't make much difference. > > It depends on whether the other workers are already busy w/ other shards > (unless you've moved these to the front of the queue in a CL I missed?). > > But yeah, I don't think it matters much. Ah, I see what you mean. Nope, I didn't move them to the front (but that's probably something to consider if things start looking imbalanced with more slimmingpaint tests).
The CQ bit was checked by joelo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1161863003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196445
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1158323009/ by leviw@chromium.org. The reason for reverting is: Causing issues on Android bots: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20(Nex....
Message was sent while issue was closed.
Patchset #6 (id:100001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
