Bug 48103 – new-run-webkit-tests: clean up duplicate, ugly code in port/chromium_gpu*.py

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[Dirk Pranke]

See bug 47874 and bug 47884 for background ...

[Dirk Pranke]

Created attachment 100176 [details]
Patch using Multiple inheritance - doesn't work so well.

[Dirk Pranke]

Updating a patch that converts just the GPU code to a more multiple-inheritance + mixin-style approach. I can't decide if this is actually much of an improvement or not.

The GPU's constructor code wants to rely on the base port's constructor already having been called (in order to call things like get_option()). However, the GPU class needs to be declared before the base port so that the methods override properly, so you get:

class MacGPU(GPU, Mac):
    def __init__(self):
         Mac.__init__(self)
         GPU.__init__(self)

(i.e., the constructors are called in the "opposite" order). This felt weird, so I moved the GPU "constructor" to _set_default_options().

We could also get rid of any dependency on the MRO by explicitly specifying which super class to call, e.g.:

class MacGPU(GPU, Mac):
    def default_child_processes(self):
         return GPU.default_child_processes(self)

and so on, but this kind of repetition and explicit overriding seems to default some of the point.
    
What do y'all think? Are there other options? In bug 47884 Tony suggested using a delegate class, but I don't think that is actually going to be much different from the code as currently checked in.

[Tony Chang]

This doesn't look like mixins to me.  It looks more like the text book case of multiple inheritance showing why it's bad.

I think the current code isn't great (we shouldn't be using inheritance for this), but it's probably better than multiple inheritance.  One alternative would be to make GPU be a property of the chromium.py port and somehow change behavior for the overridden methods if the GPU property is set.

Anyway, I don't think it's that big of a deal either way and it's probably OK to WONTFIX this bug unless someone feels motivated to post a cleanup patch.

[Dirk Pranke]

Created attachment 100545 [details]
Try again - fold chromium_gpu into other chromium* ports.

[Dirk Pranke]

Moving the google_chrome*.py changes to bug 64384.

[Tony Chang]

Comment on attachment 100545 [details]
Try again - fold chromium_gpu into other chromium* ports.

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

This looks much better to me!

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:296
> +            return self._gpu_tests(self, paths)

Where is _gpu_tests?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:64
>          if input_name and platform:
> -            port = chromium_gpu.get(platform=platform, port_name=input_name,
> -                                    options=mock_options)
> +            port = factory.get(platform=platform, port_name=input_name, options=mock_options)
>          else:
> -            port = chromium_gpu.get(port_name=port_name, options=mock_options)
> +            port = factory.get(port_name=port_name, options=mock_options)

Could this be:

if input_name and platform:
    port_name = input_name
port = factory.get(port_name=port_name, platform=platform, options=mock_options)

[Eric Seidel (no email)]

Comment on attachment 100545 [details]
Try again - fold chromium_gpu into other chromium* ports.

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

This is *way* better than waht we have, but could still be better.  Please fix the things mentioned in review before landing.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:89
> +        if self.get_option('accelerated_compositing') is None:
> +            self._options.accelerated_compositing = True

We have a function set_default_option which does exactly this.  (I think you even wrote the function originally.)

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:96
> +        # on the bots.
> +        if self.get_option('builder_name') is not None and not ' - GPU' in self._options.builder_name:
> +            self._options.builder_name += ' - GPU'

This is bad.  We should generate the builder names from component parts on demand, not store the whole thing and modify it blindly.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> +        fallback_paths = (self.GRAPHICS_TYPE_FALLBACK_PATHS[self._graphics_type] +
> +                          self.ARCHITECTURE_FALLBACK_PATHS[self._architecture] +
> +                          self.VERSION_FALLBACK_PATHS[self._version] +
> +                          self.BASE_FALLBACK_PATHS)

See the implementation of this function in webkit.py.  This is OK too... we'll have to see how it looks in the end.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:178
> +        if self._graphics_type == 'gpu':
> +            return 1

the gpu port seems super lame.

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 100545 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100545&action=review
> 
> This looks much better to me!
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:296
> > +            return self._gpu_tests(self, paths)
> 
> Where is _gpu_tests?
> 

Umm ... whoops? 

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:64
> >          if input_name and platform:
> > -            port = chromium_gpu.get(platform=platform, port_name=input_name,
> > -                                    options=mock_options)
> > +            port = factory.get(platform=platform, port_name=input_name, options=mock_options)
> >          else:
> > -            port = chromium_gpu.get(port_name=port_name, options=mock_options)
> > +            port = factory.get(port_name=port_name, options=mock_options)
> 
> Could this be:
> 
> if input_name and platform:
>     port_name = input_name
> port = factory.get(port_name=port_name, platform=platform, options=mock_options)

Yes, I think that would work.

Investigating this and the above has revealed that the tests weren't testing anything like I thought they were. New patch coming ... :(

(In reply to comment #7)

> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:89
> > +        if self.get_option('accelerated_compositing') is None:
> > +            self._options.accelerated_compositing = True
> 
> We have a function set_default_option which does exactly this.  (I think you even wrote the function originally.)

Ah, you're right, except that it is set_option_default(). I had forgotten about that. This was cut&paste, obviously, but I'll update it.
 
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:96
> > +        # on the bots.
> > +        if self.get_option('builder_name') is not None and not ' - GPU' in self._options.builder_name:
> > +            self._options.builder_name += ' - GPU'
> 
> This is bad.  We should generate the builder names from component parts on demand, not store the whole thing and modify it blindly.
>

Yeah, except that this would require coordinating with the buildbots, since we don't control the names. Still a good idea.
 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> > +        fallback_paths = (self.GRAPHICS_TYPE_FALLBACK_PATHS[self._graphics_type] +
> > +                          self.ARCHITECTURE_FALLBACK_PATHS[self._architecture] +
> > +                          self.VERSION_FALLBACK_PATHS[self._version] +
> > +                          self.BASE_FALLBACK_PATHS)
> 
> See the implementation of this function in webkit.py.  This is OK too... we'll have to see how it looks in the end.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:178
> > +        if self._graphics_type == 'gpu':
> > +            return 1
> 
> the gpu port seems super lame.

This could probably be higher, we just haven't tried it ever since the GPU tests stabilized (and the GPU tests are usually pretty fast).

[Dirk Pranke]

(In reply to comment #8)
> > if input_name and platform:
> >     port_name = input_name
> > port = factory.get(port_name=port_name, platform=platform, options=mock_options)
> 
> Yes, I think that would work.
>

Okay, turns out you can't do this. The code as written unfortunately relies on platform not being passed in as a keyword argument if you didn't want it (i.e., passing in platform=None breaks things), and also 
the code tests that if I pass in 'chromium-gpu' and 'darwin', I need to get 'chromium-gpu-mac' as the port_name.
 
These tests are lame. They need to be rewritten along with the whole factory algorithm. However, I don't want to do that in this patch to avoid perturbing things more than necessary.

I will upload another patch to take a stab at cleaning up the factory, if Eric doesn't beat me to it.

[Dirk Pranke]

Created attachment 100590 [details]
revise w/ review feedback, fix tests and factory routines

[Eric Seidel (no email)]

Comment on attachment 100590 [details]
revise w/ review feedback, fix tests and factory routines

I'm in the middle of fixing fallback paths for win.  When I finish that I'll review this in greater depth.  I think the fallback path dictionary lookup is going to end up being the wrong approach, but I'll know better once I finish my win patch shortly.

[Tony Chang]

I think this patch is an improvement, but here's another idea I had when discussing this with Ojan:

Just have a single list per platform and remove values that don't apply.  For example, Linux would have:
'chromium-gpu-linux', 'chromium-gpu-win', 'chromium-gpu', 'chromium-linux-x86', 'chromium-linux', 'chromium-win', 'chromium', 'win', 'mac' and you remove the values that don't apply for the configuration.

Mac would have:
'chromium-gpu-mac', 'chromium-gpu', 'chromium-mac-leopard', 'chromium-mac', 'chromium', 'mac-leopard', 'mac-snowleopard', 'mac', and you remove the things that don't apply.

The downside to this approach is that it's hard to see the actual fallback for things like mac-leopard-cpu.  I think that could be solved by just having a command line script (n-r-w-t --print-fallback-list) that dumps all the fallback lists for all test configurations (could also log this every run).

We could also just enumerate all the configurations and list the fallback order for each one, but that seems error prone and hard to update.

It's nice to have an easy to read list in the code, but I think that's less important than simplifying the code.

[Dirk Pranke]

(In reply to comment #12)
> I think this patch is an improvement, but here's another idea I had when discussing this with Ojan:
> 
> Just have a single list per platform and remove values that don't apply.  For example, Linux would have:
> 'chromium-gpu-linux', 'chromium-gpu-win', 'chromium-gpu', 'chromium-linux-x86', 'chromium-linux', 'chromium-win', 'chromium', 'win', 'mac' and you remove the values that don't apply for the configuration.
> 
> Mac would have:
> 'chromium-gpu-mac', 'chromium-gpu', 'chromium-mac-leopard', 'chromium-mac', 'chromium', 'mac-leopard', 'mac-snowleopard', 'mac', and you remove the things that don't apply.
>

I wonder what the code to remove the things would look like (how complicated it would be). At any rate, I'm inclined to leave this patch as-is for now, until we see what Eric comes up with for the non-chromium ports.
 
> The downside to this approach is that it's hard to see the actual fallback for things like mac-leopard-cpu.  I think that could be solved by just having a command line script (n-r-w-t --print-fallback-list) that dumps all the fallback lists for all test configurations (could also log this every run).
>

I think printing the fallback path as part of the ''config" output makes sense (I've thought about that off and on for a while). I'll upload a separate patch for that today.
 
> We could also just enumerate all the configurations and list the fallback order for each one, but that seems error prone and hard to update.
> 

This is basically what the chromium cpu ports did prior to this patch. It's not that unwieldy.

-- Dirk

[Dirk Pranke]

This has been open for a month, and I'm not sure if it's particularly relevant any more given the refactoring that abarth and eseidel and dglazkov have all been hacking on. I'm closing this as WONTFIX; someone please comment if I'm mistaken.