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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 13 00:23:50 PDT 2018 

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #337764|review?                     |review-
              Flags|                            |

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

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

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:4
> +class MiddleCommitSetFinder {

I'd call this CommitSetRangeBisector or CommitSetRangeSplitter instead if I were you.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:9
> +        const [oneCommitSet, anotherCommitSet] = commitSetsToSplit;
> +        if (oneCommitSet.containsRootPatchOrOwnedCommit() || anotherCommitSet.containsRootPatchOrOwnedCommit())

"one" and "another" are terrible names.
Call them "first" & "last" or "start" & "end".

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:20
> +        for (const repository of oneCommitSet.topLevelRepositories()) {

It would be cleaner for this to be
filter((repository) => firstCommitSet.commitForRepository(repository) != lastCommitSet.commitForRepository(repository))
then map since every repository after the identity check results in exactly one promise.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:28
> +                const [startCommit, endCommit] = oneCommit.time() < anotherCommit.time() ? [oneCommit, anotherCommit] : [anotherCommit, oneCommit];
> +                commitRangeByRepository.set(repository, [startCommit, endCommit]);

We should add a helper function to order/sort commits to CommitLog and do:
commitRangeByRepository.set(repository, CommitLog.orderCommits([startCommit, endCommit]));
outside of this if.
We should probably also add CommitLog.hasOrdering(startCommit, endCommit) which returns false
if startCommit.hasCommitTime() != endCommit.hasCommitTime() || startCommit.hasCommitOrder() != endCommit.hasCommitOrder()
(the arguments should probably be called firstCommit & secondCommit).

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:30
> +                    allCommitsWithCommitTime.push(startCommit, ...commits)}));

Missing ; and a new line after push(~).

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:31
> +                repositoriesWithCommitTime.add(repository);

I think it would be cleaner to split the list by filtering twice
to create one list for commits with commit time, and another one with order
instead of updating sets & arrays like this while iterating over repositories.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:40
> +                const [startCommit, endCommit] = oneCommit.order() < anotherCommit.order() ? [oneCommit, anotherCommit] : [anotherCommit, oneCommit];
> +                commitRangeByRepository.set(repository, [startCommit, endCommit]);

Once you have CommitLog.hasOrdering & commitLog.orderCommits,
all of this code should be shared with the code inside the above if statement.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:43
> +                    commits = [startCommit, ...commits];

Why do we need to create a new list?? Can't we just use -1 for startCommit's index or + 1 in forEach below?

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:67
> +    static _filterSortAndUniqueCommitSets(oneCommitSetFromTestGroup, availableCommitSets, commitRangeByRepository, repositoriesWithCommitTime, repositoryWithOnlyCommitOrder) {

This doesn't tell us what criteria is used to filter. Also, passing in this many arguments usually means
this is not the right abstraction layer to extract as a helper function.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:68
> +        const commitSetsInRange = availableCommitSets.filter((commitSet) => {

This should be its own helper function, and we should directly call it in commitSetClosestToMiddleOfAllCommits

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:69
> +            if (!commitSet.hasSameRepositories(oneCommitSetFromTestGroup))

I don't think this function cares whether commit set is from the test group or not.
What's important here is that this is the commit set we use to filter other ones.
So the variable name should reflect that. maybe filteringCommitSet?
Now that think about it, it might be cleaner for this helper function to simply take an array of repositories instead.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:75
> +                const [startCommit, endCommit] = commitRangeByRepository.get(repository);

So commitRangeByRepository is only used by this code to filter commit sets.
Why can't this store a closure which checks the filtering condition instead.
i.e. why can't we simply do:
if (!commitRangeByRepository.get(repository)(commit))
    return false;
where the map returns a closure which evaluates to true if the commit was in the range.
Obviously, we should rename the variable if we did do that.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:90
> +        const sortedCommitSets = commitSetsInRange.sort((commitSet0, commitSet1) => {

Again, this should be a helper function on its own, and be called directly in commitSetClosestToMiddleOfAllCommits.
Why are we so inconsistent in naming function arguments?
We should be calling these firstCommitSet, secondCommitSet.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:115
> +        const sortedUniqueCommitSets = [sortedCommitSets[0]];
> +        for (let i = 1; i < sortedCommitSets.length; i += 1) {
> +            const previousCommitSet = sortedCommitSets[i - 1];
> +            const currentCommitSet = sortedCommitSets[i];
> +            if (!previousCommitSet.equals(currentCommitSet))
> +                sortedUniqueCommitSets.push(currentCommitSet);
> +        }
> +        return sortedUniqueCommitSets;

This can be rewritten more concisely as: commitSets.filter((currentSet, i) => !i || !currentSet.equals(commitSets[i - 1]));

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:118
> +    static _findCommitSetsClosestToMiddleOfCommitsWithTime(sortedCommitSets, repositoriesWithCommitTime, allCommitsWithCommitTime) {

This function isn't really finding a commit set closest to the middle. It's filtering the list of commit sets.
How about _closestCommitSetsToBisectingCommitByTime?
Note that we normally don't use verbs such as "find" and "get" unless they involve an out argument.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:123
> +        allCommitsWithCommitTime.sort((commit0, commit1) => commit0.time() - commit1.time())

Please name these as firstCommit and secondCommit.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:126
> +        const commitWithTimeToCommitSets = new Map;

Since this function is named _findCommitSetsClosestToMiddleOfCommitsWithTime,
we can simply call this commitToCommitSetMap.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:150
> +    static _findCommitSetsClosestToMiddleOfCommitsWithOrder(remainingCommitSets, indexForCommitWithOnlyOrderByRepository) {

Nit: all these functions should have { on new line.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:155
> +        for (const repository of indexForCommitWithOnlyOrderByRepository.keys()) {

This is pretty much the exact same code as the one in _findCommitSetsClosestToMiddleOfCommitsWithTime.
We should extract this as a helper function.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:168
> +            for (const commitSet of remainingCommitSets) {

This is pretty much the same code we have in _findCommitSetsClosestToMiddleOfCommitsWithTime.
The only difference being whether we have an enumerable set of commits vs. commit sets.
We should extract a helper function for this.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:185
> +    static _chooseCommitSetClosestToMiddleOfSortedCommitSetsAmongRemainingCommitSets(remainingCommitSets, sortedCommitSets) {

"AmongRemaingCommitSets" doesn't add any additional information.

> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:193
> +        const indexInSortedCommitSets = new Map;
> +        sortedCommitSets.forEach((commitSet, index) => indexInSortedCommitSets.set(commitSet, index));

Why can't simply return remainingCommitSets[Math.floor(remainingCommitSets.length / 2)]?

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:170
> +    async middleCommitSetForTestGroup(testGroup)

We shouldn't be adding a method which relies on MiddleCommitSetFinder to AnalysisTask.
That's a layering violation if any.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:178
> +    async aggregateCommitSets()

This doesn't make it clear from where we're aggregating commit sets.
How about commitSetsFromTestGroupsAndMeasurementSet?

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:189
> +        await measurementSet.fetchBetween(this.startTime(), this.endTime());

This will throw an exception on a custom analysis task.
We should check the true-ness of platform & metric.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:38
> +    hasCommitTime() { return this._rawData['time'] > 0 && this._rawData['time'] !== null && this._rawData['time'] !== undefined; }

Just use != null. In JS, null == undefined but null !== undefined.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:46
> +    hasCommitOrder() { return this._rawData['order'] !== null && this._rawData['order'] !== undefined; }

Ditto. Note 0 != null and 0 != undefined in JS.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:122
> +    hasSameRepositories(commitSet)

We should probably call this hasSameTopLevelRepositories since we don't check owned commits' repositories.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:274
> +            this.dispatchAction('analyzeTestGroup', this._currentTestGroup, middleCommitSet, repetitionCount);

"analyzeTestGroup" is a very generic/vague name.
We should simply match the name of the part, and call it bisectTestGroup.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:289
> +            await Promise.all(fetchingPromises);

We can't block the updating of all UI until all commits are fetched.
We need to simply enqueueToRender once this work is done.
r- because this would cause a serious UI rendering delay.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:819
> +    async _analyzeCurrentTestGroup(testGroup, middleCommitSet, repetitionCount)

Ditto abut this generic name. This should be _bisectTestGroup since whether the test group is current or not is irrelevant for this function.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:823
> +        const commitSets = [oneCommitSet, middleCommitSet, anotherCommitSet];

Why don't we make this function take a list of commit sets instead?

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:824
> +        const platform = this._task.platform() || testGroup.platform();

How can task not have a platform but a test group can for a non-custom analysis task?

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:832
> +                const testGroups =  await TestGroup.createWithCustomConfiguration(this._task, platform, testGroup.test(), testGroupName, repetitionCount, [previousCommitSet, currentCommitSet]);

Nit: Two spaces between = and await.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:835
> +                alert('Failed to create a new test group: ' + error);

We should probably break immediately when the attempt to create the first test group failed
instead of preceding to create the second test groups after showing an error message.

-- 
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/20180413/3f64a6ae/attachment-0001.html>

More information about the webkit-unassigned mailing list