nrwt: make sharding tests needing locks less hard-coded
Created attachment 98107 [details] Patch
Created attachment 98108 [details] fix ChangeLog
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?
(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.
(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.
(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.
Created attachment 98287 [details] stop tracking unlocked shards, add better comments about the lock strategy, other cleanup from review feedback
Created attachment 98308 [details] fix rebase
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 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 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.
(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.
Created attachment 99016 [details] add unit tests to ensure locks are dropped promptly
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?
Committed r90192: <http://trac.webkit.org/changeset/90192>
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.
Committed r90417: <http://trac.webkit.org/changeset/90417>