Bug 63112 – nrwt: make sharding tests needing locks less hard-coded

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

[Dirk Pranke]

nrwt: make sharding tests needing locks less hard-coded

[Dirk Pranke]

Created attachment 98107 [details]
Patch

[Dirk Pranke]

Created attachment 98108 [details]
fix ChangeLog

[Ojan Vafai]

Comment on attachment 98108 [details]
fix ChangeLog

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:269
> +        self._locked_shards = []
> +        self._unlocked_shards = []

It's not clear from these names that we're talking about the HTTP lock.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:-580
> -        # Put the http tests first. There are only a couple hundred of them,
> -        # but each http test takes a very long time to run, so sorting by the
> -        # number of tests doesn't accurately capture how long they take to run.

While this comment is outdated, some comment saying why we put the HTTP tests first seems useful to me.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1362
> +            i = 0
> +            while i < len(test_lists):
> +                if test_lists[i][0] == name:
> +                    return i
> +                i += 1
> +
> +            return -1

I think you can write this as the following:
for i in range(len(test_lists)):
    if test_lists[i][0] == name:
        return i
return -1

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1372
> +        index = find(list_name, self._unlocked_shards)
> +        self._unlocked_shards.pop(index)

Is there a reason to remove items from the unlocked_shards list?

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 98108 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98108&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:269
> > +        self._locked_shards = []
> > +        self._unlocked_shards = []
> 
> It's not clear from these names that we're talking about the HTTP lock.
> 

Well, there's only one lock, and it protects the websocket tests as well as the http tests, so I find the name http_lock cumbersome and somewhat misleading. Do you have a better suggestion?

(I want to rename the _has_http_lock member field as well, but was going to do that in a different patch).

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:-580
> > -        # Put the http tests first. There are only a couple hundred of them,
> > -        # but each http test takes a very long time to run, so sorting by the
> > -        # number of tests doesn't accurately capture how long they take to run.
> 
> While this comment is outdated, some comment saying why we put the HTTP tests first seems useful to me.
> 

Okay, will add.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1362
> > +            i = 0
> > +            while i < len(test_lists):
> > +                if test_lists[i][0] == name:
> > +                    return i
> > +                i += 1
> > +
> > +            return -1
> 
> I think you can write this as the following:
> for i in range(len(test_lists)):
>     if test_lists[i][0] == name:
>         return i
> return -1
> 

Makes sense. I always forget about using range() ... too much C/C++ in my blood.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1372
> > +        index = find(list_name, self._unlocked_shards)
> > +        self._unlocked_shards.pop(index)
> 
> Is there a reason to remove items from the unlocked_shards list?

Only for consistency w/ locked_shards. Arguably this is a much longer list and might be a performance hit, but that could be fixed by switching to hash lookups if it matters.

[Ojan Vafai]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 98108 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=98108&action=review
> > 
> > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:269
> > > +        self._locked_shards = []
> > > +        self._unlocked_shards = []
> > 
> > It's not clear from these names that we're talking about the HTTP lock.
> > 
> 
> Well, there's only one lock, and it protects the websocket tests as well as the http tests, so I find the name http_lock cumbersome and somewhat misleading. Do you have a better suggestion?
> 
> (I want to rename the _has_http_lock member field as well, but was going to do that in a different patch).

I guess I'm OK with that.

> > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1372
> > > +        index = find(list_name, self._unlocked_shards)
> > > +        self._unlocked_shards.pop(index)
> > 
> > Is there a reason to remove items from the unlocked_shards list?
> 
> Only for consistency w/ locked_shards. Arguably this is a much longer list and might be a performance hit, but that could be fixed by switching to hash lookups if it matters.

I'm not really worried about the performance hit. But it adds more code to make sense of and is effectively dead code since removing the items doesn't affect anything.

[Dirk Pranke]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 98108 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=98108&action=review
> > > 
> > > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:269
> > > > +        self._locked_shards = []
> > > > +        self._unlocked_shards = []
> > > 
> > > It's not clear from these names that we're talking about the HTTP lock.
> > > 
> > 
> > Well, there's only one lock, and it protects the websocket tests as well as the http tests, so I find the name http_lock cumbersome and somewhat misleading. Do you have a better suggestion?
> > 
> > (I want to rename the _has_http_lock member field as well, but was going to do that in a different patch).
> 
> I guess I'm OK with that.
> 
> > > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1372
> > > > +        index = find(list_name, self._unlocked_shards)
> > > > +        self._unlocked_shards.pop(index)
> > > 
> > > Is there a reason to remove items from the unlocked_shards list?
> > 
> > Only for consistency w/ locked_shards. Arguably this is a much longer list and might be a performance hit, but that could be fixed by switching to hash lookups if it matters.
> 
> I'm not really worried about the performance hit. But it adds more code to make sense of and is effectively dead code since removing the items doesn't affect anything.

Okay, I'll try to rework it to make it a bit clearer what's going on.

[Dirk Pranke]

Created attachment 98287 [details]
stop tracking unlocked shards, add better comments about the lock strategy, other cleanup from review feedback

[Dirk Pranke]

Created attachment 98308 [details]
fix rebase

[Ojan Vafai]

Comment on attachment 98308 [details]
fix rebase

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:637
> +        self._pending_locked_shards = locked_shards

should this be self._remaining_locked_shards? If so, it would be nice to also add a test that breaks given that we never seem to add anything to _remaining_locked_shards.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1372
> +            return

don't need the return statement.

[Dirk Pranke]

Comment on attachment 98308 [details]
fix rebase

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

>> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:637
>> +        self._pending_locked_shards = locked_shards
> 
> should this be self._remaining_locked_shards? If so, it would be nice to also add a test that breaks given that we never seem to add anything to _remaining_locked_shards.

Yes, good catch. However, I don't understand what you want to test here. Could you rephrase this? What do you mean by "a test that breaks"? Breaks what?

>> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1372
>> +            return
> 
> don't need the return statement.

Ok.

[Ojan Vafai]

Comment on attachment 98308 [details]
fix rebase

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

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:637
>>> +        self._pending_locked_shards = locked_shards
>> 
>> should this be self._remaining_locked_shards? If so, it would be nice to also add a test that breaks given that we never seem to add anything to _remaining_locked_shards.
> 
> Yes, good catch. However, I don't understand what you want to test here. Could you rephrase this? What do you mean by "a test that breaks"? Breaks what?

Presumably, the fact that this line is setting _pending_locked_shards and other code expects _remaining_locked_shards to be set means that the code is doing the wrong thing under some circumstance that a test isn't catching.

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 98308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98308&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:637
> >>> +        self._pending_locked_shards = locked_shards
> >> 
> >> should this be self._remaining_locked_shards? If so, it would be nice to also add a test that breaks given that we never seem to add anything to _remaining_locked_shards.
> > 
> > Yes, good catch. However, I don't understand what you want to test here. Could you rephrase this? What do you mean by "a test that breaks"? Breaks what?
> 
> Presumably, the fact that this line is setting _pending_locked_shards and other code expects _remaining_locked_shards to be set means that the code is doing the wrong thing under some circumstance that a test isn't catching.

Ah. Excellent point :).

The "broken" code is doing something conservatively wrong, in that it just won't drop the lock as early as it could. We forcibly drop the lock while exiting _run_tests to be safe, so it's a fairly harmless bug.

Testing that this is dropped as early as possible is kinda difficult. I'll have to think about this one.

[Dirk Pranke]

Created attachment 99016 [details]
add unit tests to ensure locks are dropped promptly

[Dirk Pranke]

Comment on attachment 99016 [details]
add unit tests to ensure locks are dropped promptly

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_unittest.py:86
> +    def test_http_locking(tester):

Note the non-standard 'tester' name for the self, to avoid collisions with the nested class. This seems clunky but every other mechanism I could think of seemed clunkier. Suggestions?

[Dirk Pranke]

Committed r90192: <http://trac.webkit.org/changeset/90192>

[Adam Barth]

I rolled this patch out in http://trac.webkit.org/changeset/90209 and the bot healed, but I'm slightly unsure whether those two events were related.  We can try re-landing this patch if you think the two events were coincidental.

[Dirk Pranke]

Committed r90417: <http://trac.webkit.org/changeset/90417>