[webkit-reviews] review granted: [Bug 218413] Performance dashboard should avoid building same configuration under one analysis task. : [Attachment 413502] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 218413: Performance dashboard should avoid building same configuration
under one analysis task.
https://bugs.webkit.org/show_bug.cgi?id=218413

Attachment 413502: Patch

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




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

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

> Websites/perf.webkit.org/public/api/build-requests.php:50
> +	   $root_source_build_request_id = array_get($info,
'rootSourceBuildRequest');

Hm... I think this name is quite confusing still. How about
buildRequestForRootReuse?

> Websites/perf.webkit.org/public/api/build-requests.php:105
> +    $commit_set_items_source = $db->select_rows('commit_set_items',
'commitset', array('set' => $source_id, 'requires_build' => TRUE));
> +    $commit_set_items_destination = $db->select_rows('commit_set_items',
'commitset', array('set' => $destination_id, 'requires_build' => TRUE));

This isn't right. We need to also check that other commit set items are
identical.

> Websites/perf.webkit.org/public/api/build-requests.php:107
> +	   return array('status' =>
'CannotReuseRootFromCommitSetWithDifferentNumberOfItems', 'details' =>
array('sourceCommitSet' => $source_id,

This error name is wordy. How about just
CannotReuseRootWithNonMatchingCommitSets?

> Websites/perf.webkit.org/public/v3/models/build-request.js:91
> +    async findBuildRequestWithSameRoots()
> +    {

Hm... it occurs to me that it's a bit problematic to grab whatever root which
has the same set of commit set item is a bit problematic.
Some of the test groups in this analysis task can be very old.
We really want two roots to be coming from the test group so that they're built
together.
Also, do we have a guarantee that very old root will work with the latest OS?

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:93
> +	   for (const group of testGroupList) {
> +	       const nextRequest = this._nextRequestInGroup(group, updates);

Maybe it's better to write this as:
await Promise.all(testGroupList.map((group) => this._nextRequestInGroup(group,
updates))
    .filter((request) => validRequests.has(request))
    .map((request) => this._scheduleRequest(group, nextRequest,
rootReuseUpdates)));

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:115
> +	   const rootSourceBuildRequest = await
buildRequest.findBuildRequestWithSameRoots();
> +	   if (!rootSourceBuildRequest) {

The standard convention in WebKit is that the normal code path move forward &
special case will be nested in if.
Here, the case of having a root is more of a special case so I'd put that
inside this if instead.

> Websites/perf.webkit.org/unit-tests/build-request-tests.js:61
> +function oneTestGroup() {

Nit: { on the next line.
Can we give it a more descriptive name?

> Websites/perf.webkit.org/unit-tests/build-request-tests.js:165
> +function threeTestGroups(secondTestGroupOverrides, thirdTestGroupOverrides)
{

Ditto.

> Websites/perf.webkit.org/unit-tests/build-request-tests.js:407
> +	   const requests = MockRemoteAPI.inject('https://perf.webkit.org',
NodePrivilegedAPI);

I don't think we need to pass in NodePrivilegedAPI here. We're not using it.