[webkit-reviews] review granted: [Bug 223886] [perf dashboard] Add sequential mode for perf dashboard A/B testing. : [Attachment 429024] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 223886: [perf dashboard] Add sequential mode for perf dashboard A/B
testing.
https://bugs.webkit.org/show_bug.cgi?id=223886

Attachment 429024: Patch

https://bugs.webkit.org/attachment.cgi?id=429024&action=review




--- Comment #30 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 429024
  --> https://bugs.webkit.org/attachment.cgi?id=429024
Patch

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

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:80
> +    $build_request_by_commit_set = array();

We should probably call this $last_build_request_by_commit_set or
$commit_set_to_last_build_request

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:81
> +    foreach ($existing_build_requests as $last_existing_build_request) {

I don't think it's appropriate to call this last_existing_build_request since
we're iterating over every build request here.
Why not just $current_build_request as you used to call it?

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:87
> +	   if ($is_existing_commit_set)
> +	       continue;
> +	   $order_shift_by_commit_set[$commit_set_for_current_request] =
(count($build_request_by_commit_set) - 1) * $additional_build_request_count;

Huh, now that this whole thing is one line maybe it's simpler to write it as:
$is_first_build_request = !array_key_exists($commit_set_for_current_request,
$build_request_by_commit_set);
...
if (!array_key_exists($commit_set_for_current_request,
$build_request_by_commit_set))
    $order_shift_by_commit_set[$commit_set_for_current_request] =
count($build_request_by_commit_set) * $additional_build_request_count;
$build_request_by_commit_set[$commit_set_for_current_request] =
$last_existing_build_request;

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:91
> +    foreach (array_reverse($order_shift_by_commit_set, true) as $commit_set
=> $increment) {

Call this $shift for consistency?

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:99
> +	   $last_existing_build_request =
$build_request_by_commit_set[$commit_set];

Whereas here, it's appropriate to call it last_existing_build_request since it
*IS* the last build request for the commit set.

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:101
> +	   $order_shift = array_get($order_shift_by_commit_set, $commit_set,
0);

There should be no need to use array_get here.
$order_shift_by_commit_set[$commit_set] should work.

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:103
> +	       duplicate_build_request_with_new_order($db,
$last_existing_build_request, $last_existing_order + $i + $order_shift);

I think it would be better to order them as:
$last_existing_order + $order_shift + $i
maybe we should store the sum of the first two terms as $starting_order:
for ($i = 1, $starting_order = $last_existing_order + $order_shift; $i <=
$additional_build_request_count; $i++)
      duplicate_build_request_with_new_order($db, $last_existing_build_request,
$starting_order + $i);
Alternatively, we could have computed last_existing_prder as follows:
$last_existing_order = $last_existing_build_request['request_order'] +
$order_shift_by_commit_set[$commit_set];

> Websites/perf.webkit.org/public/v3/models/test-group.js:113
> +	   return this.requestsForCommitSet(commitSet).filter((request) =>
request.isTest() && request.hasCompleted()).length

Nit: missing a semicolon at the end.

> Websites/perf.webkit.org/public/v3/models/test-group.js:124
> +	   const buildRequestIndex =
orderedBuildRequests.indexOf(buildRequest);

Let's make sure buildRequestIndex is greater than 0 by asserting it (i.e.
buildRequest is in orderedBuildRequests)?

> Websites/perf.webkit.org/public/v3/models/test-group.js:141
> +    async _createAlternatingRetriesForTestGroup(maxRetryFactor)

We should define this below scheduleRetriesIfNecessary so that it's easier to
follow.

> Websites/perf.webkit.org/public/v3/models/test-group.js:143
> +	   const retryLimit = maxRetryFactor * this.initialRepetitionCount();

Since this is the only place where maxRetryFactor is used and we compute this
product in
both _createAlternatingRetriesForTestGroup and
_createSequentialRetriesForTestGroup,
we should probably just compute that in the caller instead.

> Websites/perf.webkit.org/public/v3/models/test-group.js:144
> +	   const additionalRepetitionNeeded =
this.requestedCommitSets().reduce(

Maybe we should call this retryCount since that's what we're returning.

> Websites/perf.webkit.org/public/v3/models/test-group.js:152
> +	       return {retryCount: 0, mayNeedMoreRequests: false};

Hm... I've started to think maybe a better variable name might be retryCount &
stopFutureRetries.
It's rather counterintuitive that the caller does something special when
mayNeedMoreRequests is false.

> Websites/perf.webkit.org/public/v3/models/test-group.js:189
> +	       const additionalRepetition =
this.additionalRepetitionNeededToReachInitialRepetitionCount(commitSet);
> +
> +	       if (additionalRepetition +
this.repetitionCountForCommitSet(commitSet) > retryLimit)
> +		   continue;

Wait a minute. Doesn't this stop retry earlier than necessary?
e.g. if this.repetitionCountForCommitSet(commitSet) is 4 and retryLimit is at
7,
even if additionalRepetitionNeededToReachInitialRepetitionCount returns 4,
we probably still want to add 3 more retries?

> Websites/perf.webkit.org/public/v3/models/test-group.js:401
> +    async resolveMayNeedMoreRequestsFlag(maxRetryFactor)

> I think previous review, you suggested `scheduleRetriesIfNecessary` does not
reveal the fact that it clears `mayNeedMoreRequests` flag.

Yes, but I don't think that means we want to unnecessarily split the functions.
Also, resolveMayNeedMoreRequestsFlag now doesn't convey the fact we're
scheduling new retries.

We should probably call this scheduleMoreRequestsOrClearFlag.
"IfNeeded" is the term of art if we wanted to suffix but I don't think that's
necessary.
By saying that schedule X or clear flag, we're implicitly communicating that
conditionality.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:400
> +	   const retrySummaryParts = testGroup.requestedCommitSets()

Nice! So much cleaner!

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:403
> +	       .map((item) => `${item.retryCount} added to
${testGroup.labelForCommitSet(item.commitSet)} configuration`);

Maybe we don't have to say "configuration" here.
If we say "6 added to A due to failures" that should be sufficient, right?
No need to make it wordy.

>
Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js
:1468
> +    it('should reject with "InvalidRepetitionType" if test mode is not
"alternating" or "sequential"', async () => {

Nit: repetition type

>
Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js
:1480
> +    it('should reject with "InvalidRepetitionType" while creating test group
from existing analysis task with repetition type is not "alternating" or
"sequential"', async () => {

Nit: should reject with "InvalidRepetitionType" when creating a test group with
a bad repetition type for an existing analysis task

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:175
> +		   // Unlikely to hit this condition, just in case some build
requests are created without test group fetched.

I think I'd rephrase this as something like this:
// The request might be for a very old test group we didn't fetch.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:93
> +	   await
Promise.all(Array.from(buildRequestsByGroup.keys()).map(testGroupId =>
TestGroup.fetchById(testGroupId, true)));

Nit: (testGroupId) => for consistency?
Also, we should add /* ignoreCache */ commit here too.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:99
> +	       const shouldDefer =
this._shouldDeferSequentialTestRequestWithNewCommitSet(request);

This should probably be renamed to
_shouldDeferSequentialTestRequestForNewCommitSet instead.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:161
> +	   if (testGroup.repetitionCountForCommitSet(previousCommitSet) >
retryLimit)

Surely, you meant to say testGroup.repetitionCountForCommitSet(~) >=
retryLimit?

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:164
> +	   return testGroup.initialRepetitionCount() >
testGroup.successfulTestCount(previousCommitSet);

So this could condition would & what I had pointed out above would mean
that this function can forever return true & sort of dead-lock the whole thing?