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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted 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 338218: Patch

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




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

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

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:17
> +	   const indexForCommitWithOnlyOrderByRepository = new Map;

I think we need to rename this to
indexForAllTimelessCommitsWithOrderByRepository,
and explain in the change log that we have to build this map for all commits as
opposed to the remaining commits
because we care about the distance between commits considering commits not
appearing any of commit sets we have.

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:21
> +	       .filter((repository) =>
firstCommitSet.commitForRepository(repository) !==
secondCommitSet.commitForRepository(repository));

Check has ordering here as well.

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:32
> +	      
fetchingPromises.push(CommitLog.fetchBetweenRevisions(repository,
startCommit.revision(), endCommit.revision())

And then map the result.

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:33
> +		   .then((commits) => {

That way, you can us await here like so:
await Promise.all(topLevelRepositoriesWithCommitChange.map((repository) => {
    const commits = await CommitLog.fetchBetweenRevisions(repository,
startCommit.revision(), endCommit.revision());
    ...
}));

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:54
> +	   const commitSetsInRange =
CommitSetRangeBisector._findCommitSetsWithinRange(firstCommitSet,
availableCommitSets, commitRangeByRepository);

We can just do: this._findCommitSetsWithinRange

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:85
> +    static _sortAndUniqueCommitSets(commitSets, repositoriesWithCommitTime,
repositoryWithOnlyCommitOrder)

This function name doesn't tell us what kind of sorting we're doing.
It should be renamed to something that describes how we're sorting; e.g.
orderCommitSetsByTimeThenOrder

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:87
> +	   const sortedCommitSets = commitSets.sort((firstCommitSet,
secondCommitSet) => {

I think it's cleaner to first make the copy of commitSets.

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:93
> +		   if (firstCommit.time() === secondCommit.time())
> +		       continue;
> +		   return firstCommit.time() - secondCommit.time();

Just do:
const diff = firstCommit.time() - secondCommit.time();
if (!diff)
   continue;
return diff;

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:100
> +		   if (firstCommit.order() === secondCommit.order())
> +		       continue;
> +		   return firstCommit.order() - secondCommit.order();

Ditto.

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:127
> +	   const commitOnlyOrderAvailableToCommitSets =
CommitSetRangeBisector._buildCommitToCommitSetMap(indexForCommitWithOnlyOrderBy
Repository.keys(), remainingCommitSets);

This variable name is rather long & confusing. How about
commitWithOrderToCommitSets?

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:130
> +	       const commits = remainingCommitSets.map((commitSet) =>
commitSet.commitForRepository(repository));

I think should call this one: commitsInRemainingSetsForCurrentRepository.

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:132
> +	       const commitSets =
commitOnlyOrderAvailableToCommitSets.get(closestCommit);

Since we have so many sets & maps, we should be slightly more explicit than
"commitSets".
How about commitSetsContainingClosestCommit?

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:154
> +    static _findClosestCommitByIndex(indexByCommit, commits)

This doesn't tell us closest to what.
Maybe _findCommitClosestToMiddleIndex?

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

I think this code should be below where measurementSet.fetchBetween happens so
that the fetching could be parallelized.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:180
> +	   console.assert(platform);
> +	   console.assert(metric);

I think we should just return an empty list instead of asserting.

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

Can we have blank lines around await?

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:190
> +	   for (const point of timeSeriesView)
> +	       allCommitSetsInTask.add(point.commitSet());

I don't think checking equality with Set is useful because MeasurementAdaptor
creates a unique MeasurementCommitSet for each data point.
Why don't we just map and extract each commit set instead?

> Websites/perf.webkit.org/public/v3/models/commit-log.js:100
> +	       firstCommit.hasCommitOrder() && secondCommit.hasCommitOrder();

Nit: || should be on this line, and we should wrap this condition in
parentheses as well.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:107
> +	   const firstCommitSmaller = (firstCommit.hasCommitTime() &&
secondCommit.hasCommitTime()
> +	       && firstCommit.time() < secondCommit.time()) ||
firstCommit.order() < secondCommit.order();

This is not right. We need to use tertiary expression here:
firstCommit.hasCommitTime() && secondCommit.hasCommitTime() ?
firstCommit.time() < secondCommit.time() : firstCommit.order() <
secondCommit.order()
In fact, we can just check firstCommit.hasCommitTime() since we're asserting
that two commits have an order already.

Also, please add a test for this.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:124
> +	   return commitSet.repositories().length ===
this._repositoryToCommitMap.size &&

Nit: && should be on the new line.

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

Can we just say containsRootOrPatchOrOwnedCommit?
"RootPatch" sounds like one word as opposed to two words with missing comma.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:261
> +	   this._middleCommitSetByTestGroup = null;

I think should call this _bisectingCommitSetByTestGroup to match the
terminology elsewhere.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:283
> +	   const testGroupIdSet = new Set(testGroups.map((testGroup) =>
testGroup.id()));
> +	   const sameTestGroups = this._testGroups.length === testGroups.length
&& this._testGroups.every((testGroup) => testGroupIdSet.has(testGroup.id()));

I don't think this is right because we're getting the filtered test groups,
which is affected by their hidden state.
Use TestGroup.findAllByTask instead.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:290
> +	       this._middleCommitSetByTestGroup = new Map;

This should be reset using a lazily evaluated function.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:291
> +	       const fetchingPromises = testGroups.map(async (testGroup) => {

We should probably just compute this for the current test group.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:831
> +	       const testGroupName =
CommitSet.createNameWithoutCollision(CommitSet.diff(previousCommitSet,
currentCommitSet), existingTestGroupNames);

Use createAndRefetchTestGroups instead since we're not gonna support custom
analysis tasks here.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:7
> +let MockModels = require('./resources/mock-v3-models.js').MockModels;
> +let MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;

Use const.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:183
> +	   let requests = MockRemoteAPI.inject();

Use const.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:200
> +	       const promise =
CommitSetRangeBisector.commitSetClosestToMiddleOfAllCommits([startCommitSet,
endCommitSet], allCommitSets);
> +	       const middleCommitSet = await promise;

We don't the local variable "promise". Just await directly.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:204
> +	   it('should throw exception if failed to fetch commit log', async ()
=> {

throw exception when*

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:216
> +		   assert.equal(error, rejectReason);

Try using assert.throws instead.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:221
> +	   it('should return "null" if no commit set is find other than the
commit sets define the range', async () => {

No commit set is found* other than the commit sets that* define

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:225
> +	       let promise =
CommitSetRangeBisector.commitSetClosestToMiddleOfAllCommits([startCommitSet,
endCommitSet], [startCommitSet, endCommitSet]);

Use const.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:232
> +			   repository: MockModels.osx.id(),

Can we store MockModels.osx & MockModels.webkit as local variables so that we
don't keep repeating?

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:291
> +	   it('should return bisection point closest to the middle of revision
range', async () => {

return bisecting* commit set* closest to the middle.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:295
> +	       let promise =
CommitSetRangeBisector.commitSetClosestToMiddleOfAllCommits([startCommitSet,
endCommitSet], allCommitSets);

Uset const.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:298
> +	       const osx_fetch_request = requests.find((fetch_request) =>
fetch_request.url ===
'/api/commits/9/?precedingRevision=osx-commit-1&lastRevision=osx-commit-3');
> +	       const webkit_fetch_request = requests.find((fetch_request) =>
fetch_request.url ===
'/api/commits/11/?precedingRevision=webkit-commit-1&lastRevision=webkit-commit-
6');

Use camelCase.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:359
> +	       const middleCommitSet = await promise;
> +	       const expectedMiddleCommitSet = allCommitSets[3];
> +	       assert.equal(middleCommitSet, expectedMiddleCommitSet);

I don't we think we need to split this into three different lines.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:362
> +	   it('should return same bisection point even when two commit sets
from original commit set have inversed order', async () => {

have inverted* order or inverse* order

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:366
> +	       let promise =
CommitSetRangeBisector.commitSetClosestToMiddleOfAllCommits([endCommitSet,
startCommitSet], allCommitSets);

Use const.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:427
> +	       const middleCommitSet = await promise;
> +	       const expectedMiddleCommitSet = allCommitSets[3];
> +	       assert.equal(middleCommitSet, expectedMiddleCommitSet);

Ditto about fitting all of this into a single line.

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:525
> +});

I think we need a few more test cases.
1. The case when a subset of commit sets contain more owned repositories than
others, and vice versa.
2. Cases where the order of commits in two different repositories are reversed
in some repositories.