[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 #12 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 337249
  --> https://bugs.webkit.org/attachment.cgi?id=337249
Patch

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

> Websites/perf.webkit.org/public/v3/analysis-strategies.js:4
> +    static async aggregateCommitSetsFromAnalysisTask(analysisTask)

This should probably just be a method of AnalysisTask.

> Websites/perf.webkit.org/public/v3/analysis-strategies.js:20
> +        const startPoint = series.findById(analysisTask.startMeasurementId());
> +        const endPoint = series.findById(analysisTask.endMeasurementId());
> +
> +        const pointAfterEnd = endPoint.series.nextPoint(endPoint);

Just use series.viewBetweenPoints.

> Websites/perf.webkit.org/public/v3/analysis-strategies.js:31
> +        if (rangeSpecifiedByCommitSets.length != 2) {

This should just be an assert since we don't currently have any test group with less or more than two commit sets.

> Websites/perf.webkit.org/public/v3/analysis-strategies.js:39
> +            AnalysisStrategies.logger.error('Cannot bisect a range that is smaller than 3 commit sets');

This shouldn't emit an error message since we can legitimately hit this case when there are less than three commits between two commit sets.

> Websites/perf.webkit.org/public/v3/analysis-strategies.js:50
> +        if (!commonRepositories.size) {
> +            AnalysisStrategies.logger.error('No common repository is found among selected commit sets');
> +            return [];
> +        }

We should just assert.

> Websites/perf.webkit.org/public/v3/analysis-strategies.js:56
> +        const commitsByRepository = new Map;
> +        const commitSetsByConfig = new Map;

I think we should extract a helper function which builds these two maps.

> Websites/perf.webkit.org/public/v3/analysis-strategies.js:64
> +            try {

Don't swallow error like this. With this, we don't get any backtraces when the network or some other kind of an error happens.

> Websites/perf.webkit.org/public/v3/analysis-strategies.js:68
> +                AnalysisStrategies.logger.error(`Failed to fetch commits between "${firstCommit.label()}" - "${lastCommit.label()}" due to ${error}`);

You need to use commit.title() instead to include the repository name.

> Websites/perf.webkit.org/public/v3/analysis-strategies.js:247
> +const AnalysisStrategies = {
> +    defaultStrategy: bisectRangeWithSimilarRevisionDistanceAmongExistingCommitSets,
> +    bisectRangeWithSimilarRevisionDistanceAmongExistingCommitSets,
> +    CommitSetAggregator,
> +    RangeSplitStrategies,
> +    logger: console
> +};

As we discussed in person, I don't think there is much benefit to making these things runtime switchable.
So there's no need to create these strategy objects/maps.

-- 
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/20180406/0c3f9101/attachment-0002.html>