Bug 63112 - nrwt: make sharding tests needing locks less hard-coded
Summary: nrwt: make sharding tests needing locks less hard-coded
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 63103 63788
Blocks: 63116
  Show dependency treegraph
 
Reported: 2011-06-21 20:07 PDT by Dirk Pranke
Modified: 2011-07-05 17:07 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.13 KB, patch)
2011-06-21 20:07 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix ChangeLog (8.00 KB, patch)
2011-06-21 20:09 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
stop tracking unlocked shards, add better comments about the lock strategy, other cleanup from review feedback (13.18 KB, patch)
2011-06-22 18:35 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix rebase (8.34 KB, patch)
2011-06-22 20:21 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
add unit tests to ensure locks are dropped promptly (11.96 KB, patch)
2011-06-28 18:11 PDT, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-06-21 20:07:25 PDT
nrwt: make sharding tests needing locks less hard-coded
Comment 1 Dirk Pranke 2011-06-21 20:07:53 PDT
Created attachment 98107 [details]
Patch
Comment 2 Dirk Pranke 2011-06-21 20:09:26 PDT
Created attachment 98108 [details]
fix ChangeLog
Comment 3 Ojan Vafai 2011-06-22 11:19:04 PDT
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?
Comment 4 Dirk Pranke 2011-06-22 11:24:34 PDT
(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.
Comment 5 Ojan Vafai 2011-06-22 11:29:50 PDT
(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.
Comment 6 Dirk Pranke 2011-06-22 11:39:22 PDT
(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.
Comment 7 Dirk Pranke 2011-06-22 18:35:38 PDT
Created attachment 98287 [details]
stop tracking unlocked shards, add better comments about the lock strategy, other cleanup from review feedback
Comment 8 Dirk Pranke 2011-06-22 20:21:42 PDT
Created attachment 98308 [details]
fix rebase
Comment 9 Ojan Vafai 2011-06-24 15:45:51 PDT
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.
Comment 10 Dirk Pranke 2011-06-24 16:34:11 PDT
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.
Comment 11 Ojan Vafai 2011-06-27 09:40:05 PDT
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.
Comment 12 Dirk Pranke 2011-06-27 13:29:24 PDT
(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.
Comment 13 Dirk Pranke 2011-06-28 18:11:22 PDT
Created attachment 99016 [details]
add unit tests to ensure locks are dropped promptly
Comment 14 Dirk Pranke 2011-06-28 18:12:47 PDT
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?
Comment 15 Dirk Pranke 2011-06-30 19:11:35 PDT
Committed r90192: <http://trac.webkit.org/changeset/90192>
Comment 16 Adam Barth 2011-07-01 00:32:03 PDT
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.
Comment 17 Dirk Pranke 2011-07-05 17:07:21 PDT
Committed r90417: <http://trac.webkit.org/changeset/90417>