[Webkit-unassigned] [Bug 183888] Add a bisect button to automatically schedule bisecting A/B tasks.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=183888

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

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

Here are my comments half way through the review.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:175
> +        const allTestGroupsInTask = TestGroup.findAllByTask(this.id());

What guarantees that we've already fetched all the test groups?

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:191
> +        try {

I don't really use try-catch elsewhere.
I'm not certain if it's a good idea to use it here either because it can swallow unexpected exceptions.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:201
> +    static async performBinaryBisect(commitSetInTestGroup, allCommitSetsInTask, selectBisectionPointStrategy)

First off, "bisection" is a binary search by definition". "Bi" means two.
Second off, this isn't performing bisection itself. It's creating a new test group to bisect.
So I think we need to rename this to something like createTestGroupForBisection.

Finally, the argument to this function doesn't need to say selectBisectionPointStrategy. selectBisectionPoint would suffice.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:204
> +            throw 'Can only bisect a test group with 2 commit sets';

We can simply return this as a return value in the case we had an error, and null otherwise.
This way, if this function throws an exception (unexpectedly), it would be easier to debug.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:206
> +        const commitSetSortByLatestCommitTime = (oneCommitSet, anotherCommitSet) => oneCommitSet.latestCommitTime() - anotherCommitSet.latestCommitTime();

Does this fallback to build time for "system" graphs?

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:210
> +        if (!allSimpleCommitSets)
> +            throw 'Cannot bisect on a range with root/patch/owned commit';

Is the idea that we'd like to implement the MVP and expand it later?
We certainly want to be able to support this in the future, right?

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:242
> +    static async selectBisectionPointWithSimilarRevisionDistance(uniqueSortedCommitSets)

Nit: sortedUniqueCommitSets.
It's unclear what these commit sets are for. I think the idea here is to pick one commit set out of them?
If so, maybe orderedCommitSet might sufficient. I don't think "unique" adds much value here.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:91
> +    isCommitSetWithoutRootPatchOrOwnedCommit() {

Nit: { should be on the new line.
Since this is a method on commit set, there is no need to say commit say.
How about containsRootPatchOrOwnedCommit and negate the meaning.
It's usually better to have a function which tests the positive condition than a negative condition
because then we end up writing code like !isCommitSetWithoutRootPatchOrOwnedCommit, which is harder to read.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:103
> +            if (this.requiresBuildForRepository(repository))
> +                return false;

Why are we checking this condition?

> Websites/perf.webkit.org/public/v3/models/commit-set.js:169
> +        const allRepositories = new Set(firstCommitSet.repositories());
> +        secondCommitSet.repositories().forEach((repository) => allRepositories.add(repository));

Why not just new Set([...a, ...b])?

> Websites/perf.webkit.org/public/v3/models/commit-set.js:170
> +        let nameParts = [];

This should be const.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:186
> +            if (firstCommit == secondCommit && firstPatch != secondPatch)
> +                nameParts.push(`${repository.name()}: Patch-${firstPatch.id()} - Patch-${secondPatch.id()}`);

I don't think it makes sense to expose the attachment ID like this. We don't do elsewhere in the UI.
If we're concerned about the length of the name, then we can just diff the name with the maximum length of name to show.
e.g. if we had WebKit-WebComponents-A.patch and WebKit-WebComponents-B.patch, we can just show:
WebKit: "...A.patch" - "...B.patch"

> Websites/perf.webkit.org/public/v3/models/commit-set.js:189
> +                nameParts.push(`${repository.name()}: ${firstCommit.label()}+Patch-${firstPatch.id()} - ${secondCommit.label()}+Patch-${secondPatch.id()}`);

I think "r124 with a.patch" reads better than "r124+Patch a.patch".

> Websites/perf.webkit.org/public/v3/models/commit-set.js:198
> +            const leftRootFileDescription = uniqueInFirstCommitSet.map((rootFile) => rootFile.id()).join(', ');
> +            const rightRootFileDescription = uniqueInSecondCommitSet.map((rootFile) => rootFile.id()).join(', ');

Again, I don't think it makes sense to use attachment IDs.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180331/0ad4ecea/attachment.html>