Bug 75958 - nrwt should use multiple http shards
Summary: nrwt should use multiple http shards
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 63116
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-10 06:40 PST by Kristóf Kosztyó
Modified: 2012-01-13 11:55 PST (History)
5 users (show)

See Also:


Attachments
add --max-locked-shard option (2.58 KB, patch)
2012-01-11 01:12 PST, Kristóf Kosztyó
no flags Details | Formatted Diff | Diff
add --max-locked-shard option (2.65 KB, patch)
2012-01-12 05:52 PST, Kristóf Kosztyó
ossy: review-
ossy: commit-queue-
Details | Formatted Diff | Diff
add --max-locked-shard option (5.23 KB, patch)
2012-01-13 04:56 PST, Kristóf Kosztyó
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kristóf Kosztyó 2012-01-10 06:40:24 PST
We ran some tests with multiple http shards on the Qt port and it seems stable, we haven't met any flaky http test. Maybe we can allow it by default or if the other ports still have problems with it we can make an option like --max-http-shards=i and set it's default value to 1.
Comment 1 Dirk Pranke 2012-01-10 11:29:21 PST
Sure, that seems fine. Feel free to add a patch, or I will get around to it sooner or later.
Comment 2 Kristóf Kosztyó 2012-01-11 01:12:50 PST
Created attachment 121992 [details]
add --max-locked-shard option

I added the option to set it manual so now we can test it easier.
Comment 3 Csaba Osztrogonác 2012-01-11 07:00:06 PST
Comment on attachment 121992 [details]
add --max-locked-shard option

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

LGTM. Any objection?

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:383
> +            help="Set the maximum of locked shards"),

only a little nit: "Maximum number of locked shards"
Comment 4 Ojan Vafai 2012-01-11 10:14:46 PST
Comment on attachment 121992 [details]
add --max-locked-shard option

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:679
> +        if self._options.max_locked_shards:
> +            num_of_locked_shards = self._options.max_locked_shards
> +        else:
> +            num_of_locked_shards = 1

How about adding a boolean option instead (e.g. --shard-http-tests)? Then we can return "max(math.ceil(num_workers / 4.0), 1)" and tweak that math until we get reasonable stability.
Comment 5 Csaba Osztrogonác 2012-01-12 05:21:19 PST
(In reply to comment #4)
> How about adding a boolean option instead (e.g. --shard-http-tests)? Then we can return "max(math.ceil(num_workers / 4.0), 1)" and tweak that math until we get reasonable stability.

I prefer the integer parameter, which let us use what we want.

max(math.ceil(num_workers / 4.0) wouldn't be good for 4 or 
8 workers, because 1 or 2 http shards isn't so useful ...

I think num_of_locked_shards should be an integer option now, 
and we can move forward to increase the default value of it.
Comment 6 Kristóf Kosztyó 2012-01-12 05:52:01 PST
Created attachment 122223 [details]
add --max-locked-shard option

I fixed the grammar issues :)
Comment 7 Csaba Osztrogonác 2012-01-12 06:20:28 PST
Comment on attachment 122223 [details]
add --max-locked-shard option

Let's go forward, r=me.
Comment 8 Csaba Osztrogonác 2012-01-12 06:28:29 PST
Comment on attachment 122223 [details]
add --max-locked-shard option

r-, because it broke a unittest. :-/

ERROR: test_shard_by_dir (webkitpy.layout_tests.controllers.manager_unittest.ShardingTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/oszi/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py", line 82, in test_shard_by_dir
    locked, unlocked = self.get_shards(num_workers=2, fully_parallel=False)
  File "/home/oszi/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py", line 79, in get_shards
    return self.manager._shard_tests(test_list, num_workers, fully_parallel)
  File "/home/oszi/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 593, in _shard_tests
    return self._shard_by_directory(test_files, num_workers)
  File "/home/oszi/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 664, in _shard_by_directory
    'locked_shard'),
  File "/home/oszi/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 706, in _resize_shards
    num_old_per_new = divide_and_round_up(len(old_shards), max_new_shards)
  File "/home/oszi/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 695, in divide_and_round_up
    return int(math.ceil(float(numerator) / divisor))
TypeError: unsupported operand type(s) for /: 'float' and 'Mock'

Could you check it and fix?
Comment 9 Kristóf Kosztyó 2012-01-13 04:56:28 PST
Created attachment 122412 [details]
add --max-locked-shard option

I fixed the unit test sorry about that, and I also write a new unit test for test it works properly.
Comment 10 WebKit Review Bot 2012-01-13 11:55:26 PST
Comment on attachment 122412 [details]
add --max-locked-shard option

Clearing flags on attachment: 122412

Committed r104967: <http://trac.webkit.org/changeset/104967>
Comment 11 WebKit Review Bot 2012-01-13 11:55:32 PST
All reviewed patches have been landed.  Closing bug.