Bug 77143 – run-webkit-tests calls "nm" when it doesn't need to

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

run-webkit-tests calls "nm" when it doesn't need to

[Ojan Vafai]

Created attachment 124182 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 124182 [details]
Patch

A few thoughts:
1.  I'm generally in favor of this change (I like seeing things be faster!)
2.  This starts to feel like these "skipped lists" should just act as filters on the test list
3.  I think we need at least one comment in the code about "why" these calls are used.  To make it clear that we're trying to avoid the nm call because it's slow.

[Ojan Vafai]

Created attachment 124185 [details]
Patch

[Ojan Vafai]

(In reply to comment #2)
> (From update of attachment 124182 [details])
> 2.  This starts to feel like these "skipped lists" should just act as filters on the test list

I was thinking about something like this too. Specifically, if we initialized TestExpectations *before* we collected the list of tests, we could avoid a considerably amount of duplicate disk IO and we could avoid walking skipped subdirectories entirely in some cases. It would be a tricky refactor though since currently TestExpectations assumes it has the full list of tests and since a more specific line can unskip a specific test in a skipped directory.

In either case, it's probably a bigger change than I'm willing to make right now.

> 3.  I think we need at least one comment in the code about "why" these calls are used.  To make it clear that we're trying to avoid the nm call because it's slow.

Done.

[Tony Chang]

Comment on attachment 124185 [details]
Patch

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

Commenting on style.  I don't really know this code.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:309
> +        for feature, directories in self._missing_feature_to_skipped_tests().items():

feature is not used so you could use .values() instead of .items().

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:311
> +            for directory in directories:
> +                for test in test_list:

It looks like you want the pair-wise combination of each test and directory.  You can use itertools.product for this instead of nested loops.

[Dirk Pranke]

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

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:308
> +    def _should_check_for_missing_features(self, test_list):

what happens if test_list == None (or [])? We should still check (since this is equivalent to all tests), but this will return False, won't it?

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 124182 [details] [details])
> > 2.  This starts to feel like these "skipped lists" should just act as filters on the test list
> 
> I was thinking about something like this too. Specifically, if we initialized TestExpectations *before* we collected the list of tests, we could avoid a considerably amount of duplicate disk IO and we could avoid walking skipped subdirectories entirely in some cases. It would be a tricky refactor though since currently TestExpectations assumes it has the full list of tests and since a more specific line can unskip a specific test in a skipped directory.
> 

I have long thought that such a change would be the best way to reduce time-to-first-test; as you say, it is a sizeable refactoring and I've always had other things to work on (and other lower hanging fruit).

[Ojan Vafai]

Comment on attachment 124185 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:309
>> +        for feature, directories in self._missing_feature_to_skipped_tests().items():
> 
> feature is not used so you could use .values() instead of .items().

Done.

>> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:311
>> +                for test in test_list:
> 
> It looks like you want the pair-wise combination of each test and directory.  You can use itertools.product for this instead of nested loops.

Except I want to bail the first match that I see. Wouldn't itertools.product be a lot slower? Keep in mind that test_list can be tens of thousands of strings.

[Tony Chang]

Comment on attachment 124185 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:311
>>> +                for test in test_list:
>> 
>> It looks like you want the pair-wise combination of each test and directory.  You can use itertools.product for this instead of nested loops.
> 
> Except I want to bail the first match that I see. Wouldn't itertools.product be a lot slower? Keep in mind that test_list can be tens of thousands of strings.

itertools.product is a generator, it should be the same speed as 2 nested for loops.

[Ojan Vafai]

Created attachment 124193 [details]
Patch

[Ojan Vafai]

(In reply to comment #6)
> what happens if test_list == None (or [])? We should still check (since this is equivalent to all tests), but this will return False, won't it?

Why is test_list == None/[] equivalent to all test? As best I can tell, the only time we ever pass None/[] through this codepath is in the Rebaseline classes, in which case we probably don't want to include these tests anyways.

[Dirk Pranke]

(In reply to comment #10)
> (In reply to comment #6)
> > what happens if test_list == None (or [])? We should still check (since this is equivalent to all tests), but this will return False, won't it?
> 
> Why is test_list == None/[] equivalent to all test? As best I can tell, the only time we ever pass None/[] through this codepath is in the Rebaseline classes, in which case we probably don't want to include these tests anyways.

Ah, sorry, I had the wrong context in my head. When we call port.tests(), None == "find me all the tests". You're right, by the time we call skipped_tests from inside TestExpectations, we're either guaranteed to have all the tests we want to run, or be linting, so this doesn't matter. Never mind :).

[Ojan Vafai]

Created attachment 124200 [details]
Patch

[Ojan Vafai]

Comment on attachment 124185 [details]
Patch

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

>>>> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:311
>>>> +                for test in test_list:
>>> 
>>> It looks like you want the pair-wise combination of each test and directory.  You can use itertools.product for this instead of nested loops.
>> 
>> Except I want to bail the first match that I see. Wouldn't itertools.product be a lot slower? Keep in mind that test_list can be tens of thousands of strings.
> 
> itertools.product is a generator, it should be the same speed as 2 nested for loops.

It turned out to be marginally slower in some cases, but not enough to matter. Changed it to use itertools.

[Ojan Vafai]

Committed r106058: <http://trac.webkit.org/changeset/106058>