nrwt: support more than two drivers in DriverProxy
Created attachment 129167 [details] Patch
Comment on attachment 129167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129167&action=review > Tools/ChangeLog:9 > + Now that we can support per-test command lines for > + Drivers, modify DriverProxy to keep a map of running I'm not sure what this is referring to. Are you saying this is needed for testing? Does that mean tests are reusing the same instance of DriverProxy? Should we just mock out DriverProxy instead?
I see, I followed the bug chain to find bug 76501. Did you find out the answer to the question in bug 76501 comment 1? I don't want us to do all this work to find out that it still doesn't work with the tools (garden-o-matic, webkit-patch rebaseline-expectations, etc). Also, virtual directories have an extra cost in that we have to teach gardeners about them. How many gpu configurations do we have to support? Can we just have separate bots run those specific tests (the bots should cycle quickly since it's only a subset of the test suite)? Would it be sufficient to just have a win7, mac SL and linux bot in these configurations?
(In reply to comment #3) > I see, I followed the bug chain to find bug 76501. Did you find out the answer to the question in bug 76501 comment 1? I don't know the answer to the question in comment #1 beyond what I said in comment #2 - namely, it should theoretically work but there may be a few bugs > I don't want us to do all this work to find out that it still doesn't work with the tools (garden-o-matic, webkit-patch rebaseline-expectations, etc). > I don't really know of any way of finding that out for sure except to do the work and see what happens. It's also important to keep in mind that garden-o-matic doesn't work with the gpu ports today. > Also, virtual directories have an extra cost in that we have to teach gardeners about them. They'll pretty much work like regular directories (except that the actual tests will be located elsewhere), so I don't think this'll be a big deal; it's no worse than subclassing objects. > > How many gpu configurations do we have to support? Can we just have separate bots run those specific tests (the bots should cycle quickly since it's only a subset of the test suite)? Would it be sufficient to just have a win7, mac SL and linux bot in these configurations? What problem would that solve? We already run the gpu tests on the bots, in all the layout test configurations; the problem is that the existence of the gpu port themselves complicates things. I have come to believe that the virtual test dir thing is just a better solution, and once it's there, we'll find other uses for it as well (I already have at least one in mind that'll be needed for the apple win port).
(In reply to comment #4) > (In reply to comment #3) > > How many gpu configurations do we have to support? Can we just have separate bots run those specific tests (the bots should cycle quickly since it's only a subset of the test suite)? Would it be sufficient to just have a win7, mac SL and linux bot in these configurations? > > What problem would that solve? We already run the gpu tests on the bots, in all the layout test configurations; the problem is that the existence of the gpu port themselves complicates things. I thought the main problem we were trying to solve is that the tools don't work with the gpu port. I.e., it's hard to rebaseline and garden-o-matic doesn't know about them. That said, it doesn't help with the test_expectations.txt mess, while virtual directories does. > I have come to believe that the virtual test dir thing is just a better solution, and once it's there, we'll find other uses for it as well (I already have at least one in mind that'll be needed for the apple win port). I think we're trading one set of problems for another set of problems, but I'm not opposed to trying it.
About this review in particular, it would be better if we could make this runtime configurable through layoutTestController rather than having to launch new processes.
(In reply to comment #6) > About this review in particular, it would be better if we could make this runtime configurable through layoutTestController rather than having to launch new processes. Sorry, I just realized you can't use layoutTestController since the tests are virtual directories. You could however do this by communicating with DRT via stdin. In hindsight, I should have done the same for ref tests + --no-pixel.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > How many gpu configurations do we have to support? Can we just have separate bots run those specific tests (the bots should cycle quickly since it's only a subset of the test suite)? Would it be sufficient to just have a win7, mac SL and linux bot in these configurations? > > > > What problem would that solve? We already run the gpu tests on the bots, in all the layout test configurations; the problem is that the existence of the gpu port themselves complicates things. > > I thought the main problem we were trying to solve is that the tools don't work with the gpu port. I.e., it's hard to rebaseline and garden-o-matic doesn't know about them. That said, it doesn't help with the test_expectations.txt mess, while virtual directories does. > Right. The main problem we're trying to solve is the proliferation of ports, especially ports that reflect the desire to just run certain subsets of tests differently :) (ports in the physical class sense should really have to map onto things that are built). It would've been easier to fix the tools. > > I have come to believe that the virtual test dir thing is just a better solution, and once it's there, we'll find other uses for it as well (I already have at least one in mind that'll be needed for the apple win port). > > I think we're trading one set of problems for another set of problems, but I'm not opposed to trying it. Could be ... guess we'll see. JamesR's almost managed to kill off the gpu tests anyway.
(In reply to comment #7) > (In reply to comment #6) > > About this review in particular, it would be better if we could make this runtime configurable through layoutTestController rather than having to launch new processes. > > Sorry, I just realized you can't use layoutTestController since the tests are virtual directories. > > You could however do this by communicating with DRT via stdin. In hindsight, I should have done the same for ref tests + --no-pixel. Yeah, unfortunately that's going to take changing all the DRTs as NRWT code. Probably a good thing to do down the road. I can file a separate bug for that; it would be easy to do that at the same time as when I try to fix the bugs keeping us from using webkit-style DRT protocol instead of the test_shell protocol.
(In reply to comment #9) > (In reply to comment #7) > > (In reply to comment #6) > > > About this review in particular, it would be better if we could make this runtime configurable through layoutTestController rather than having to launch new processes. > > > > Sorry, I just realized you can't use layoutTestController since the tests are virtual directories. > > > > You could however do this by communicating with DRT via stdin. In hindsight, I should have done the same for ref tests + --no-pixel. > > Yeah, unfortunately that's going to take changing all the DRTs as NRWT code. Probably a good thing to do down the road. I can file a separate bug for that; it would be easy to do that at the same time as when I try to fix the bugs keeping us from using webkit-style DRT protocol instead of the test_shell protocol. In the case of the GPU tests, you would only have to change Chromium's DRT. That seems more focused than trying to change common NRWT code for a Chromium specific need.
(In reply to comment #10) > In the case of the GPU tests, you would only have to change Chromium's DRT. That seems more focused than trying to change common NRWT code for a Chromium specific need. I'm a bit confused ... are you talking about just this specific patch, or the larger 'remove gpu ports, and virtual test dirs' idea?
(In reply to comment #11) > (In reply to comment #10) > > In the case of the GPU tests, you would only have to change Chromium's DRT. That seems more focused than trying to change common NRWT code for a Chromium specific need. > > I'm a bit confused ... are you talking about just this specific patch, or the larger 'remove gpu ports, and virtual test dirs' idea? Sorry, I'm not trying to be confusing. This specific patch. Rather than supporting any number of drivers, would it be possible to reusing the existing drivers? E.g., for a given virtual directory that needs a different configuration, we would pass a command via stdin to DRT to switch it to the right configuration for that test.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > In the case of the GPU tests, you would only have to change Chromium's DRT. That seems more focused than trying to change common NRWT code for a Chromium specific need. > > > > I'm a bit confused ... are you talking about just this specific patch, or the larger 'remove gpu ports, and virtual test dirs' idea? > > Sorry, I'm not trying to be confusing. This specific patch. > > Rather than supporting any number of drivers, would it be possible to reusing the existing drivers? E.g., for a given virtual directory that needs a different configuration, we would pass a command via stdin to DRT to switch it to the right configuration for that test. OK. I am a bit worried about what sort of bugs (if any) we might run into doing that and how much work that would be (although I don't think it'll be that hard or that risky, I just don't know that code that well). So, I would prefer to land this solution as is, finish the reset of this chain of bugs, and then come back to this idea. I do like the idea of just keeping on DRT around and changing its configuration as needed rather than starting and stopping them. Does that work for you?
Created attachment 129491 [details] fix bad merge in webkit.py - driver_input.args doesn't exist yet
Comment on attachment 129491 [details] fix bad merge in webkit.py - driver_input.args doesn't exist yet View in context: https://bugs.webkit.org/attachment.cgi?id=129491&action=review Why do some of these methods refer to self._driver whereas other iterate over all the drivers? > Tools/Scripts/webkitpy/layout_tests/port/driver.py:-160 > - def start(self): > - raise NotImplementedError('Driver.start') Why did you remove this?
Tony, the bigger picture here is that we've had a lot of trouble managing the expectations for the GPU configuration because it multiplies the number of configurations by two. For example, when we add Android bots, we need to add two Android configurations: one for GPU and one for non-GPU. Dirk's idea with virtual directories is to make the GPU configuration basically invisible to almost all of the tools. Because we run only a small number of tests in the GPU and non-GPU configuration, running the GPU configuration all the time won't increase the number of tests we run very much. I agree that re-configuring a running DRT instance is better than having multiple instance of DRT to manage. We already re-configure the DRT based on test path, so doing something similar here would make sense. That said, I think it's ok to proceed with this patch as is. Hopefully Dirk will come back and improve the design once we've got the new approach working end-to-end.
(In reply to comment #15) > (From update of attachment 129491 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129491&action=review > > Why do some of these methods refer to self._driver whereas other iterate over all the drivers? > The ones that refer to self._driver are really calling methods that just need *a* driver to be valid. Those routines should probably actually either be @classmethod or routines on the Port object instead, but I didn't really want to make those changes as part of this patch (Was planning to do so in a subsequent patch). I will add a FIXME for now and come back to this. > > Tools/Scripts/webkitpy/layout_tests/port/driver.py:-160 > > - def start(self): > > - raise NotImplementedError('Driver.start') > > Why did you remove this? Whoops. Bad merge :(. Will fix.
Yeah, having the self._driver member is an invitation to bugs.
(In reply to comment #16) > Tony, the bigger picture here is that we've had a lot of trouble managing the expectations for the GPU configuration because it multiplies the number of configurations by two. For example, when we add Android bots, we need to add two Android configurations: one for GPU and one for non-GPU. > > Dirk's idea with virtual directories is to make the GPU configuration basically invisible to almost all of the tools. Because we run only a small number of tests in the GPU and non-GPU configuration, running the GPU configuration all the time won't increase the number of tests we run very much. I don't have a problem with these changes as I said in comment #5. > I agree that re-configuring a running DRT instance is better than having multiple instance of DRT to manage. We already re-configure the DRT based on test path, so doing something similar here would make sense. > > That said, I think it's ok to proceed with this patch as is. Hopefully Dirk will come back and improve the design once we've got the new approach working end-to-end. I don't see the advantage of checking in code if everyone agrees it's a sub optimal design and we're going to rewrite it immediately. That just makes the rewrite harder, the reviewing harder, and the extra changes increase the chance of breaking the tree. This doesn't seem that time critical-- we're talking about an extra week or two on the bots and maybe an extra day of Dirk's time. But as I told Dirk in IM, I don't feel strongly enough against it to r- it.
Created attachment 129522 [details] patch for landing
Committed r109264: <http://trac.webkit.org/changeset/109264>
> I don't see the advantage of checking in code if everyone agrees it's a sub optimal design and we're going to rewrite it immediately. That just makes the rewrite harder, the reviewing harder, and the extra changes increase the chance of breaking the tree. I certainly understand your perspective. I think the idea here is to ship early and iterate.
Comment on attachment 129522 [details] patch for landing Cleared review? from attachment 129522 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).