[webkit-reviews] review denied: [Bug 183888] Add a bisect button to automatically schedule bisecting A/B tasks. : [Attachment 337764] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 183888: Add a bisect button to automatically schedule bisecting A/B tasks.
https://bugs.webkit.org/show_bug.cgi?id=183888

Attachment 337764: Patch

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




--- 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(remai
ningCommitSets, 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.