Don't duplicate virtual and non-virtual baselines.

Don't duplicate virtual and non-virtual baselines.

BUG=674202

[Jack Bates, 2016-12-15 18:08:44]

Description was changed from

==========
Don't duplicate virtual and non-virtual baselines.

BUG=674202
==========

to

==========
Don't duplicate virtual and non-virtual baselines.

BUG=674202
==========

[Jack Bates, 2016-12-15 18:08:44]

jack@nottheoilrig.com changed reviewers:
+ qyearsley@chromium.org

[Jack Bates, 2016-12-15 18:23:06]

This change causes optimize-baselines to remove virtual baselines if they're
duplicates of their non-virtual baselines.
What do you think of this solution?

[qyearsley, 2016-12-15 18:58:45]

+ojan, this is related to the issue from 2013: http://crbug.com/237701

jack@, thanks for working on this :-)

My main comment is that I'd like to see a unit test method that confirms the
fixed behavior.

A few other suggestions for simplifying this:

1. Is it possible we're duplicating some part of
Port.virtual_baseline_search_path?
2. Should Port.baseline_search_path include a virtual baseline directory if
there is one?
3. Could the new part of BaselineOptimizer._relative_baseline_search_paths could
be extracted to another helper method?

See:
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/To...

PS: Our use of the names "paths" and "path" may be a little inconsistent. My
understanding of the name port.baseline_search_path() (which returns a list of
directory paths) is that it's a "path" in the sense that you visit each
directory there in order (when searching for a baseline), although it may sound
like its a single file path. If this is confusing, we could consider renaming
some things.

[Jack Bates, 2016-12-15 21:03:00]

On 2016/12/15 18:58:45, qyearsley wrote:
> +ojan, this is related to the issue from 2013: http://crbug.com/237701
> 
> jack@, thanks for working on this :-)
> 
> My main comment is that I'd like to see a unit test method that confirms the
> fixed behavior.

Yes, I'll see what I can do.

> A few other suggestions for simplifying this:
> 
> 1. Is it possible we're duplicating some part of
> Port.virtual_baseline_search_path?

Port.virtual_baseline_search_path appends suite.name (e.g.
virtual/spinvalidation/paint) to each directory, whereas we need to check the
non-virtual directories (i.e. suite.base) for duplicate baselines.

> 2. Should Port.baseline_search_path include a virtual baseline directory if
> there is one?

Currently, virtual_baseline_search_path is e.g.
- /.../LayoutTests/platform/linux/virtual/spinvalidation/paint
- /.../LayoutTests/platform/win/virtual/spinvalidation/paint

The virtual baseline_root is /.../LayoutTests/virtual/spinvalidation/paint

baseline_search_path is e.g.
- /.../LayoutTests/platform/linux
- /.../LayoutTests/platform/win

And finally, the non-virtual baseline_root is /.../LayoutTests

So to include virtual_baseline_search_path in baseline_search_path, I think we'd
also need to include baseline_root?

It seems odd to me that the virtual_baseline_search_path includes the first
component of the test (e.g. "paint") whereas baseline_search_path doesn't. I
haven't figured out the reason for this yet.

> 3. Could the new part of BaselineOptimizer._relative_baseline_search_paths
could
> be extracted to another helper method?

I'm not sure yet how to do this in a way that improves the clarity ...

> See:
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/To...
> 
> PS: Our use of the names "paths" and "path" may be a little inconsistent. My
> understanding of the name port.baseline_search_path() (which returns a list of
> directory paths) is that it's a "path" in the sense that you visit each
> directory there in order (when searching for a baseline), although it may
sound
> like its a single file path. If this is confusing, we could consider renaming
> some things.

[Jack Bates, 2016-12-15 22:23:17]

On 2016/12/15 21:03:00, Jack Bates wrote:
> On 2016/12/15 18:58:45, qyearsley wrote:
> > +ojan, this is related to the issue from 2013: http://crbug.com/237701
> > 
> > jack@, thanks for working on this :-)
> > 
> > My main comment is that I'd like to see a unit test method that confirms the
> > fixed behavior.
> 
> Yes, I'll see what I can do.

Done. I added a unit test that confirms the fixed behavior.

[qyearsley, 2016-12-16 01:00:13]

On 2016/12/15 at 22:23:17, jack wrote:
> On 2016/12/15 21:03:00, Jack Bates wrote:
> > On 2016/12/15 18:58:45, qyearsley wrote:
> > > +ojan, this is related to the issue from 2013: http://crbug.com/237701
> > > 
> > > jack@, thanks for working on this :-)
> > > 
> > > My main comment is that I'd like to see a unit test method that confirms
the
> > > fixed behavior.
> > 
> > Yes, I'll see what I can do.
> 
> Done. I added a unit test that confirms the fixed behavior.

LGTM, thanks for adding the test :-)

[Dirk Pranke, 2016-12-16 01:07:54]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-12-16 01:07:54]

I would like either ojan or I (preferably I) or both of us to review this before
it lands. I will try to get to it soon, possibly even tonight but more likely
tomorrow.

[qyearsley, 2016-12-16 01:18:10]

On 2016/12/16 at 01:07:54, dpranke wrote:
> I would like either ojan or I (preferably I) or both of us to review this
before it lands. I will try to get to it soon, possibly even tonight but more
likely tomorrow.

Sounds good - I think it's possible that it can be simplified or clarified
(possibly by using/changing methods that are in port).

[Dirk Pranke, 2016-12-18 04:26:00]

lgtm. Sorry for the delay; I thought this was a different, much more complex
change :).

https://codereview.chromium.org/2575383004/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py
(right):

https://codereview.chromium.org/2575383004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py:86:
directories.append(self._baseline_root(baseline_name))
Stylistically, I don't tend to like repeatedly modifying variables
in place like this. I'd probably write this as something more like:

directories = ([self._filesystem.replath(path, self._webkit_base)
                for path in self._baseline_search_path(port, baseline_name)] + 
               [self._baseline_root(baseline_name)])

https://codereview.chromium.org/2575383004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py:94:
directories += non_virtual_directories
and similar.

[qyearsley, 2017-01-03 19:03:33]

On 2016/12/18 at 04:26:00, dpranke wrote:
> lgtm. Sorry for the delay; I thought this was a different, much more complex
change :).
> 
>
https://codereview.chromium.org/2575383004/diff/20001/third_party/WebKit/Tool...
> File
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py
(right):
> 
>
https://codereview.chromium.org/2575383004/diff/20001/third_party/WebKit/Tool...
>
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py:86:
directories.append(self._baseline_root(baseline_name))
> Stylistically, I don't tend to like repeatedly modifying variables
> in place like this. I'd probably write this as something more like:
> 
> directories = ([self._filesystem.replath(path, self._webkit_base)
>                 for path in self._baseline_search_path(port, baseline_name)] +

>                [self._baseline_root(baseline_name)])
> 
>
https://codereview.chromium.org/2575383004/diff/20001/third_party/WebKit/Tool...
>
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py:94:
directories += non_virtual_directories
> and similar.

Note, this CL still LGTM (and I agree with dpranke@'s suggestion)

[qyearsley, 2017-01-18 00:34:12]

On 2017/01/03 at 19:03:33, qyearsley wrote:
> On 2016/12/18 at 04:26:00, dpranke wrote:
> > lgtm. Sorry for the delay; I thought this was a different, much more complex
change :).
> > 
> >
https://codereview.chromium.org/2575383004/diff/20001/third_party/WebKit/Tool...
> > File
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py
(right):
> > 
> >
https://codereview.chromium.org/2575383004/diff/20001/third_party/WebKit/Tool...
> >
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py:86:
directories.append(self._baseline_root(baseline_name))
> > Stylistically, I don't tend to like repeatedly modifying variables
> > in place like this. I'd probably write this as something more like:
> > 
> > directories = ([self._filesystem.replath(path, self._webkit_base)
> >                 for path in self._baseline_search_path(port, baseline_name)]
+ 
> >                [self._baseline_root(baseline_name)])
> > 
> >
https://codereview.chromium.org/2575383004/diff/20001/third_party/WebKit/Tool...
> >
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py:94:
directories += non_virtual_directories
> > and similar.
> 
> Note, this CL still LGTM (and I agree with dpranke@'s suggestion)

Ping