Bug 76764 – run-perf-tests ignore Skipped list on chromium

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

[Ryosuke Niwa]

Apparently Chromium port doesn't inherit from Base port :(

[Ryosuke Niwa]

(In reply to comment #0)
> Apparently Chromium port doesn't inherit from Base port :(

I mean WebKit* port.

[Eric Seidel (no email)]

Yes.  Base should die.  Most of its logic should split off into other objects, and what remains should be folded up into WebKit.  Chromium needs to become a real boy some day.

[Ryosuke Niwa]

(In reply to comment #2)
> Yes.  Base should die.  Most of its logic should split off into other objects, and what remains should be folded up into WebKit.  Chromium needs to become a real boy some day.

That sounds like a good idea. But, I think I'm going to take the easy way out and move stuff from WebKit to Base for now :( Making Chromium a real WebKit port seems to require too much effort just to fix this bug.

[Dirk Pranke]

I agree that BasePort and WebKitPort should probably merge, but merging webkit and chromium is a fair pile of work (that I'm gradually working on) and we need to do that carefully.

Not something to tackle here :).

[Ryosuke Niwa]

Created attachment 123505 [details]
fixes the bug

[Adam Barth]

Comment on attachment 123505 [details]
fixes the bug

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

> Tools/ChangeLog:10
> +        layout tests but performacne tests don't use test_expectations.txt so Chromium port

performacne: typo

[Adam Barth]

I understand that you're just moving code around, but this stuff is poorly factored.  The base port shouldn't be a god object that understands details of how to parse skipped lists.

[Ryosuke Niwa]

(In reply to comment #7)
> I understand that you're just moving code around, but this stuff is poorly factored.  The base port shouldn't be a god object that understands details of how to parse skipped lists.

It's not obvious what the correct refactoring is. It seems that the first refactoring we need to do is to turn Chromium into a proper webkit port.

[Adam Barth]

> It's not obvious what the correct refactoring is. It seems that the first refactoring we need to do is to turn Chromium into a proper webkit port.

I more incremental approach is to create a class for Skipped lists that knows how to parser them and translate them into expectations rather than have the port know all those details.

[Ryosuke Niwa]

(In reply to comment #9)
> > It's not obvious what the correct refactoring is. It seems that the first refactoring we need to do is to turn Chromium into a proper webkit port.
> 
> I more incremental approach is to create a class for Skipped lists that knows how to parser them and translate them into expectations rather than have the port know all those details.

Do you strongly feel that it's a blocker for this bug? If not, I'm not motivated to do that the refactoring.

[Dirk Pranke]

Comment on attachment 123505 [details]
fixes the bug

I think this change is fine. While I agree with the general "god" sentiment about the Port class, it's not at all clear what the right way to factor out logic is here, and creating a class just for parsing Skipped files itself would be silly.

I think perhaps a separate change that moved all of the expectations-related logic out might be good, but I don't really want to make that a blocker of this and I'd want to think about that more.

[Dirk Pranke]

Comment on attachment 123505 [details]
fixes the bug

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

> Tools/Scripts/webkitpy/layout_tests/port/base.py:599
> +    @memoized

Note that I wouldn't bother with the @memoized here (I realize that the WebKitPort had it); this routine can't be called all that often, and memoizing it but not other expectations-related routines might lead to weird consistency issues.

[Ryosuke Niwa]

(In reply to comment #11)
> I think perhaps a separate change that moved all of the expectations-related logic out might be good, but I don't really want to make that a blocker of this and I'd want to think about that more.

That sounds like a good idea. Maybe we can create a class for the "list of tests" that can parse test_expectations.txt and Skipped.

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 123505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123505&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:599
> > +    @memoized
> 
> Note that I wouldn't bother with the @memoized here (I realize that the WebKitPort had it); this routine can't be called all that often, and memoizing it but not other expectations-related routines might lead to weird consistency issues.

This function is called by skips_perf_test, which is called per test. So it should be memoized.

[Dirk Pranke]

(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 123505 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=123505&action=review
> > 
> > > Tools/Scripts/webkitpy/layout_tests/port/base.py:599
> > > +    @memoized
> > 
> > Note that I wouldn't bother with the @memoized here (I realize that the WebKitPort had it); this routine can't be called all that often, and memoizing it but not other expectations-related routines might lead to weird consistency issues.
> 
> This function is called by skips_perf_test, which is called per test. So it should be memoized.

Okay, for now :). 

Ultimately I would probably put skips_perf_test() in perftestrunner and just merged it into _collect_tests(). Neither skips_layout_test or skips_perf_test() really belongs on the Port object (see bug 47528). The port object should have no real knowledge of the actual expectations, just a way to provide the contents of the files. Memoization often (not always) suggests to me that you have the wrong algorithmic design and you should fix the algorithm rather than relying on memoizing to get the performance you need.

[Ryosuke Niwa]

(In reply to comment #15)
> Ultimately I would probably put skips_perf_test() in perftestrunner and just merged it into _collect_tests(). Neither skips_layout_test or skips_perf_test() really belongs on the Port object (see bug 47528). The port object should have no real knowledge of the actual expectations, just a way to provide the contents of the files. Memoization often (not always) suggests to me that you have the wrong algorithmic design and you should fix the algorithm rather than relying on memoizing to get the performance you need.

That makes a lot of sense. I added perf_test version but was wondering why those methods are hanging off of the port object. We should probably look at the bigger picture of isolating test expectation related stuff into a separate class though.

[Ryosuke Niwa]

anyway, thanks for the review.

[Ryosuke Niwa]

Committed r105644: <http://trac.webkit.org/changeset/105644>

[Dirk Pranke]

(In reply to comment #16)
> We should probably look at the bigger picture of isolating test expectation related stuff into a separate class though.

There are (at least) two different operations going on ... the first is getting the content of things like test expectations files and skipped files, and that is (IMO) a port-specific operation, at the very least to figure out where the paths to the files are, and I try to go a step further so that you don't actually need a filesystem at all (you just ask for the a block of text data).

The second is actually parsing those files to turn them into real objects, and that is already done in generic code (in models/test_expectations.py). Possibly it makes sense to add skipped file parsing logic to the latter.