<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body><span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span> changed
          <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Write a script that detects chart changes by using v3 API."
   href="https://bugs.webkit.org/show_bug.cgi?id=184419">bug 184419</a>
          <br>
             <table border="1" cellspacing="0" cellpadding="8">
          <tr>
            <th>What</th>
            <th>Removed</th>
            <th>Added</th>
          </tr>

         <tr>
           <td style="text-align:right;">Attachment #338979 Flags</td>
           <td>review?
           </td>
           <td>review-
           </td>
         </tr></table>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Write a script that detects chart changes by using v3 API."
   href="https://bugs.webkit.org/show_bug.cgi?id=184419#c5">Comment # 5</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Write a script that detects chart changes by using v3 API."
   href="https://bugs.webkit.org/show_bug.cgi?id=184419">bug 184419</a>
              from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=338979&action=diff" name="attach_338979" title="Patch">attachment 338979</a> <a href="attachment.cgi?id=338979&action=edit" title="Patch">[details]</a></span>
Patch

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=338979&action=review">https://bugs.webkit.org/attachment.cgi?id=338979&action=review</a>

r- because there is a significant amount of restructuring of the code we need to do here.
Also r- because we definitely need tests for this new script.

<span class="quote">> Websites/perf.webkit.org/public/v3/async-task.js:34
> +        return typeof module == 'undefined' && typeof window == 'undefined';</span >

This doesn't make sense. "window" is always defined inside a browser outside a WebWorker.
r- because of this bug. Also, we clearly need a browser test to make sure this function returns true inside a browser.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:24
> +async function analysisLoop(options)</span >

This function needs to have try-catch to make sure the script doesn't stop running after encountering the first exception.
But we shouldn't have try-catch anywhere else in the codebase.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:41
> +class Analyzer {</span >

This is an extremely generic name... It doesn't even tell us what it analyzes.
How about MeasurementSetAnalyzer?

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:45
> +        this._testingConfigurations = Analyzer.configurationsForTesting(manifest);</span >

If we're exposing Analyzer.configurationsForTesting as a static method,
it would be cleaner for the caller (analysisLoop) to call this method,
and then Analyzer to simply take the result.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:52
> +        this._logger.info("Start detecting.");</span >

"Start detecting" is probably not the most descriptive log.
It's probably better to say "starting analysis" since we're gonna add more analysis features to this script.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:57
> +    static configurationsForTesting(manifest)</span >

Please add a FIXME here to share this code with DashboardPage.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:82
> +    async analyzeTestConfiguration(platform, metric)</span >

This function should probably take a measurement set instead and be renamed to analyzeMeasurementSet.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:84
> +        const measurementSet = MeasurementSet.findSet(platform.id(), metric.id(), platform.lastModified(metric));</span >

Why isn't done during configurationsForTesting?

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:85
> +        this._logger.info(`\n==== Analyzing the last ${this._changeDetectionConfigs.maxDays} days: "${metric.fullName()}" on "${platform.name()}" ====\n`);</span >

It's kind of silly to log the number of days we're analyzing since that's identical for all platforms & metrics.
Why don't we spit this out when we log "start detecting".

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:86
> +        const endTime = Date.now();</span >

This end time would be different for each platform/metric we fetch.
I think we should compute this once when we start the analysis so that they're consistent across platforms & metrics.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:93
> +        let segmentedValues = await measurementSet.fetchSegmentation('segmentTimeSeriesByMaximizingSchwarzCriterion', [], 'current', false);</span >

Use const.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:100
> +        const ranges = Statistics.findRangesForChangeDetectionsWithWelchsTTest(rawValues, segmentedValues, 0.99).map((range) =>({</span >

Either omit 0.99 or make it configurable.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:118
> +                const disjoint = range.endPoint.seriesIndex < taskStartPoint.seriesIndex</span >

Nit: isDisjoint but with respect to what?
Maybe isDisjoint

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:122
> +                if (!disjoint)
> +                    range.overlappingAnalysisTasks.push(task);</span >

This is silly. We're collecting the list of overlapping analysis tasks just to filter ranges in the next statement.
We should simply invert the nesting of these two loops, and omit ranges right away.
Also, we don't have to find starting & ending points to check the overlapped-ness;
simply call startTime() / endTime() on the task itself. i.e.
ranges.filter((range) => {
    bool hasOverlappingTask = false;
    for (const task of analysisTasks) {
        bool taskEndsBeforeRangeStart = task.endTime() < range.startPoint.time;
        bool taskStartsAfterRangeEnd = range.endPoint.time < task.startTime();
        if (!(taskEndsBeforeRangeStart || taskStartsAfterRangeEnd))
            hasOverlappingTask = true;
    }
    return !hasOverlappingTask;
})

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:128
> +        const filteredRanges = ranges.filter((range) => range.endPoint.time >= startTime && !range.overlappingAnalysisTasks.length)
> +            .sort((a, b) => a.endPoint.time - b.endPoint.time);</span >

Sounds like we should do this filtering before filtering ranges based on overlapping tasks since the latter computation is a lot more expensive.
You can merge that to the above filter code I wrote.
Also, there is no point in sorting them based on start/end time since we're picking the one with the largest change in the next loop.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:130
> +        let summary, rangeWithMostSignificantChange = null, valueChangeSummaryForRangeWithMostSignificantChange = null;</span >

Nit: Don't allocate multiple variables in a single statement like this.

<span class="quote">> Websites/perf.webkit.org/tools/run-analysis.js:143
> +            const currentChangeIsMoreSignificant = !rangeWithMostSignificantChange
> +                || (valueChangeSummaryForRangeWithMostSignificantChange.changeType === valueChangeSummary.changeType
> +                    && Math.abs(valueChangeSummaryForRangeWithMostSignificantChange.relativeChange) < Math.abs(valueChangeSummary.relativeChange))
> +                || (valueChangeSummaryForRangeWithMostSignificantChange.changeType === progressionString
> +                    && valueChangeSummary.changeType === regressionString);</span >

This is a very complicated logic.
1. We can instead, remember whether there was any regression in the previous filtering step,
2. Ignore all progressions if there were regressions
3. Sort them by diff.

But really, I'm not certain if we want to always prioritize regressions over progressions.
e.g. if we had 50% progression and 0.5% regression, we probably want to figure out where the progression occurred
since that probably means the test is broken somehow.

<span class="quote">> Websites/perf.webkit.org/unit-tests/measurement-set-tests.js:44
> -            assert.equal(requests[0].url, '../data/measurement-set-1-1.json');
> +            assert.equal(requests[0].url, '/data/measurement-set-1-1.json');</span >

Can we make this refactoring in a separate patch?</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>