Bug 218413 – Performance dashboard should avoid building same configuration under one analysis task.

• Component: New Bugs
• Status: RESOLVED FIXED

Performance dashboard should avoid building same configuration under one analysis task.

Created attachment 412832 [details]
Patch

<rdar://problem/70429799>

[Ryosuke Niwa]

Comment on attachment 412832 [details]
Patch

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

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

reuseBuildRequest sounds like a boolean.
How about rootSourceBuildRequest or buildRequestToReuseRootFrom or something?
Or maybe buildRequestForRoots?

> Websites/perf.webkit.org/public/api/build-requests.php:170
> +    if ($reused_root_for_patch != count($root_by_commit_and_patch) || $reused_root_for_owned_commit != count($root_by_owned_commit)) {
> +        return array('status' => 'NotAllBuiltRootAreUsedFromSourceCommitSet', 'details' => array('reusedRootByPatchCount' => $reused_root_for_patch,

Why don't we check that commit sets are identical other than commitset_root_file,
or at least ones that require commitset_requires_build?

We can then simply check that all commit_set_items which need commitset_root_file are satisfied at the end.
It seems like we can dramatically simplify the logic here.

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

Nit: { should be on the new line.
This is a rather confusingly named function.
It basically finds a build request which have the same roots to build, right?
Why don't just say that: findBuildRequestForSameRoots?
Or maybe buildRequestWithSamePatches?
I think the fact it's in the same analysis task is kind of unimportant.
If we wanted to, we can make it support build request from another analysis task in the future and there is no issue there.

> Websites/perf.webkit.org/public/v3/models/build-request.js:95
> +        const allTestGroupsInTask = await TestGroup.fetchForTask(this.analysisTaskId(), true);

It's rather subtle that this has to ignore the cache. Perhaps we should add a comment here.
Do we also have a test for this cache ignoring behavior?

> Websites/perf.webkit.org/public/v3/models/build-request.js:102
> +                if (!buildRequest.platform().group() && !this.platform().group()

Can we just add a helper to Platform to do these two comparisons of group & platform: e.g.
buildRequest.platform().isInSameGroupAs(this.platform())

> Websites/perf.webkit.org/public/v3/models/build-request.js:108
> +                if (!buildRequest.commitSet().equals(this.commitSet(), true))

This "true" here is super confusing. Can't we just add a helper method like equalsIgnoringRoot instead?

> Websites/perf.webkit.org/public/v3/models/build-request.js:113
> +                if (buildRequest.isScheduled() && !scheduledBuildRequest)
> +                    scheduledBuildRequest = buildRequest;

It seems like this will override it with whichever build request that comes last?
That doesn't seem right. We should probably prefer whichever was scheduled earliest.

> Websites/perf.webkit.org/public/v3/models/build-request.js:115
> +                if (buildRequest.status() == 'running' && !runningBuildRequest)
> +                    runningBuildRequest = buildRequest;

Ditto.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:105
> -    equals(other)
> +    equals(other, ignoreBuiltRoot)

Then you can add one liner here which just calls equals with this argument set to true.
Also, please define the default argument value instead of making undefined argument implicitly treated as false.

> Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:-84
> -

What happened here?

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:96
> +            const promise = nextRequest.sameBuildTypeBuildRequestUnderSameAnalysisTask().then((sameBuildTypeBuildRequest) => {

Can we extract this portion as another helper async function?

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:119
> +        updates = await this._pullBuildbotOnAllSyncers(buildReqeustsByGroup);
> +        updates = {
> +            ...rootReuseUpdates,
> +            ...updates
> +        };

I think this is dangerous. There is a race here between us noticing there is a build request we can re-use and polling it again from builedbot.
Between those two times, the state of build request may have changed; e.g. user has canceled it.
We at minimum, we should check that condition in the server side API.
It's also possible that the root we previously found has already been purged here. That too has to be checked.
It also makes a bit uneasy that we're trying to reconcile things from buildbot & our own logic at the same time.
That seems unsafe. Maybe we should sanity check here that we're not trying to update both at the same time, and error out if we're.

Comment on attachment 412832 [details]
Patch

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

>> Websites/perf.webkit.org/public/api/build-requests.php:170
>> +        return array('status' => 'NotAllBuiltRootAreUsedFromSourceCommitSet', 'details' => array('reusedRootByPatchCount' => $reused_root_for_patch,
> 
> Why don't we check that commit sets are identical other than commitset_root_file,
> or at least ones that require commitset_requires_build?
> 
> We can then simply check that all commit_set_items which need commitset_root_file are satisfied at the end.
> It seems like we can dramatically simplify the logic here.

Sure, will do.

>> Websites/perf.webkit.org/public/v3/models/build-request.js:95
>> +        const allTestGroupsInTask = await TestGroup.fetchForTask(this.analysisTaskId(), true);
> 
> It's rather subtle that this has to ignore the cache. Perhaps we should add a comment here.
> Do we also have a test for this cache ignoring behavior?

Will add one.

>> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:96
>> +            const promise = nextRequest.sameBuildTypeBuildRequestUnderSameAnalysisTask().then((sameBuildTypeBuildRequest) => {
> 
> Can we extract this portion as another helper async function?

Will do.

>> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:119
>> +        };
> 
> I think this is dangerous. There is a race here between us noticing there is a build request we can re-use and polling it again from builedbot.
> Between those two times, the state of build request may have changed; e.g. user has canceled it.
> We at minimum, we should check that condition in the server side API.
> It's also possible that the root we previously found has already been purged here. That too has to be checked.
> It also makes a bit uneasy that we're trying to reconcile things from buildbot & our own logic at the same time.
> That seems unsafe. Maybe we should sanity check here that we're not trying to update both at the same time, and error out if we're.

On the build-requests api, there is a check to ensure the rootSourceBuildRequest is 'completed' and there is a corresponding unit test named `should fail request with "CanOnlyReuseCompletedBuildRequest" if build request to reuse is not completed`.
Will add a check on the api to ensure the roots to reuse are not deleted. And will add a test for it.

Regarding `update both at the same time`, after discussed with Ryosuke in person, we both agreed on we should use buildbot status as the ground truth. If there a build request has updates both on `updates` and `rootReuseUpdates`, `updates` will overwrite it.
Current patch is doing that, will add a comment on for line 116 - 119.
Furthermore, there is a unit test name `should not reuse the root when same build request is avaialble but the build request has been scheduled` which tests this case.

Created attachment 413222 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 413222 [details]
Patch

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

> Websites/perf.webkit.org/public/api/build-requests.php:103
> +function reuse_roots_in_commit_set($source_id, $destination_id, $db) {

$db should be the first argument by convention.

> Websites/perf.webkit.org/public/api/build-requests.php:111
> +    foreach ($commit_set_items_destination as &$commit_set_item_destination) {

Why not just $destination_item?

> Websites/perf.webkit.org/public/api/build-requests.php:112
> +        $matching_commit_set_item = NULL;

Ditto: $source_item. It's clear that it's matching one.
What's not clear where it's used is whether this is for the source or the designation.

> Websites/perf.webkit.org/public/api/build-requests.php:130
> +        $root_file_row = $db->select_first_row('uploaded_files', 'file', array('id' => $matching_commit_set_item['commitset_root_file']));
> +        if ($root_file_row['file_deleted_at'])
> +            return array('status' => 'CannotReuseDeletedRoot', 'details' => array('commitSet' => $destination_id, 'rootFile' => $root_file_id));

There is a race here. It's possible that between the time we check this condition
and the new add commit_set_items, prune_old_files would have ran.

> Websites/perf.webkit.org/public/api/build-requests.php:133
> +        $db->update_row('commit_set_items', 'commitset', array('set' => $destination_id, 'commit' => $commit_set_item_destination['commitset_commit']),
> +            array('root_file' => $root_file_id), 'set');

We probably need to check the above condition again after running this query.
Maybe add a comment clarifying that race condition too.

> Websites/perf.webkit.org/public/v3/models/build-request.js:90
> +    async findBuildRequestForSameRoots()

Maybe this should be called findBuildRequest*With*SameRoots.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:77
> +    allBuiltRootsAvailable()

A function that answers a question like this should be of that form.
e.g. areAllRootsAvailable.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:120
> +    _equalsWithCondition(other, ignoreBuiltRoot)

"Condition" is very vague term. Why not _equalsOptionallyIgnoringRoot?

> Websites/perf.webkit.org/public/v3/models/platform.js:30
> +        if (!this.group() && !other.group() && this.id() != other.id())
> +            return false;

I think it's clear to say:
if (!this.group() && !other.group())
    return this == other;
Because this is a DataModelObject, we shouldn't have multiple platforms of the same ID.

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:68
> +    it('should reuse roots from existing commit set if build requests are the same', async () => {

You mean "reuse roots from existing build requests if the commits sets are equal except the existence of roots".

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:293
> +    it('should fail request with "FailedToFindReuseBuildRequest" is reuse request is invalid', async () => {

Nit: is -> if.
Also, "reuse request is valid" sounds tautological. All test cases that fail are due to the precondition being invalid.
How about "if the build request to reuse does not exist"?

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:91
> +    addReusableBuildRequestsMockData(db, statusList, needsNotification=true, addMockConfiguration=true)

Nit: By convention, this should be named addMockBuildRequestsWithRoots.
These functions shouldn't be named after what they're used for but after what they add.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:217
> +    addReusableTestGroupWithOwnedCommits(db)

Ditto.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:303
> +    mockTestSyncConfigWithTwoBuildersOneTester: function ()

Isn't the significance here that webkit-svn accepts patch?
We should name these functions after what's important.
Probably mockTestSyncConfigWithPatchAcceptingBuilder

> Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:950
> +        it('should reuse root from completed build request for same build request', async () => {

reuse *the roots* from *a* completed build request with *the same commit sets*?

> Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1036
> +        it('should defer scheduling the build request if there is another same build request is "running", but should schedule if "running" build request fails afterwards', async () => {

I don't follow this description.

> Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1157
> +            assert.deepEqual(MockRemoteAPI.requests[0].data, {'id': 900, 'jsonrpc': '2.0', 'method': 'force', 'params':
> +                    {'wk': '191622', 'os': '10.11 15A284', 'wk-patch': 'http://localhost:8180/api/uploaded-file/100.txt',
> +                        'build-request-id': '900', 'forcescheduler': 'force-some-builder-2'}});

Nit: Wrong indentation. Always 4 spaces.

> Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:1160
> +        it('should not reuse the root when same build request is avaialble but the build request has been scheduled', async () => {

"same build request" is super confusing term.
We should say "a build request with the same commit sets".

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:97
> +            const promise = this._reuseRootIfAvailbleElseTrySchedule(group, nextRequest, rootReuseUpdates);
> +            promiseList.push(promise);

It seems like we can do this in a single line?

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:103
> +        // Use buildbot status as ground truth, if a build request has updates on both
> +        // 'rootReuseUpdates' and 'updates', 'updates' should overwrite 'rootReuseUpdates'.

This is very wordy way of saying "rootReuseUpdates will be overridden by status fetched from buildbot"

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:114
> +    async _reuseRootIfAvailbleElseTrySchedule(testGroup, buildRequest, updates)

This function is mostly about scheduling it on buildbot.
In the case if it's one requesting to build roots, then it can optionally re-use the old one.
But one could argue that it's also a part of "scheduling" it anyway.
So why not just _scheduleRequest?

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:116
> +        const rootSourceBuildRequest = await buildRequest.findBuildRequestForSameRoots();

I think it's more descriptive to call this existingBuildRequestWithRoots or something.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:118
> +            return await this._scheduleRequestIfSlaveIsAvailable(buildRequest, testGroup.requests,

We need a patch to replace all these non-inclusive terminology...

> Websites/perf.webkit.org/unit-tests/build-request-tests.js:410
> +        const requests = MockRemoteAPI.inject('https://perf.webkit.org', NodePrivilegedAPI);
> +        beforeEach(() => {
> +            PrivilegedAPI.configure('test', 'password');
> +        });

Why do we need to configure to mock privileged API at all here? We're not using any privileged API are we?

Created attachment 413502 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 413502 [details]
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.

Comment on attachment 413502 [details]
Patch

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

>> Websites/perf.webkit.org/public/api/build-requests.php:105
>> +    $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.

$commit_set_items_source = $db->query_and_fetch_all('SELECT * FROM commit_set_items WHERE commitset_set = $1 AND commitset_commit IS NOT NULL', array($source_id));
$commit_set_items_destination = $db->query_and_fetch_all('SELECT * FROM commit_set_items WHERE commitset_set = $1 AND commitset_commit IS NOT NULL', array($destination_id));
Will exclude all the custom roots (which will have commitset_commit to be null) and check if the remaining commit set items are equal, just like what we did on findBuildRequestWithSameRoots

>> Websites/perf.webkit.org/public/v3/models/build-request.js:91
>> +    {
> 
> 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?

Discussed in person, the OS needs to be the same otherwise the CommitSet equal check will fail.
Current root reuse policy may miss some cases that we can reuse the root, but we think this might be a good compromise.

>> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:93
>> +            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)));

This might not work as  the first argument of `_scheduleRequest` is referring to the first argument `group` in the lambda for first map.
We may return both (group, request) pair for the first lambda.
```
await Promise.all(testGroupList.map((group) => [group, this._nextRequestInGroup(group, updates)])
    .filter(([group, request]) => validRequests.has(request)).
    .map(([group, request]) => this._scheduleRequest(group, request, rootReuseUpdates)));
```

Landed in r269871.