Bug 157339 – Summary page should show warnings when current or baseline data is missing

• Component: Perf Dashboard
• Status: RESOLVED FIXED

Summary page enhancement.

Created attachment 278069 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 278069 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:8
> +        Set summary page to be the home page of v3 UI.

I'd call it the default page since that's the terminology used in the dashboard.

> Websites/perf.webkit.org/ChangeLog:9
> +        Show warning icon while part of data is missing.

when either baseline or current data is missing
would be clearer.

> Websites/perf.webkit.org/ChangeLog:11
> +        Update unit test for MeasurementSet.

You should explain what kind of updates you're making.

> Websites/perf.webkit.org/ChangeLog:14
> +        * public/v3/components/ratio-bar-graph.js:

Please add per-function change description below.

> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:33
> +        var children = [element('div', {class: 'seperator'}), element('div', {class: 'bar'}), element('div', {class: 'label'})];

Why don't we declare this after adding updating classes?

> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:37
> +        else if(this._ratio != null)

Nit: a space between "if" and "(".

> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:44
> +            var warningIcon = new WarningIcon;
> +            children.push(warningIcon);

We can do this in a single line.

> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:49
> +        if (this._ratio) {

You should check that this._ratio is not NaN here.

> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:51
> +            this.content().querySelector('.bar').style.width = Math.min(percent, 50) + '%';

We should just specify this as an inline style declaration as in:
var barStyle = '';
if (this._ratio) {...}
element('div', {class: 'bar', style: barStyle});

> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:53
> +        this.content().querySelector('.label').textContent = this._label;

We should just put the text above where we create the element as in:
element('div', {class: 'label'}, this._label).

> Websites/perf.webkit.org/public/v3/main.js:23
> +        var summaryPage = manifest.summary? new SummaryPage(manifest.summary) : null;

Nit: We need a space before ?.

> Websites/perf.webkit.org/public/v3/main.js:50
> -        if (dashboardPages)
> +        if (summaryPage)
> +            router.setDefaultPage(summaryPage);
> +        else if (dashboardPages)

Please mention in the change log that you're changing the default.

> Websites/perf.webkit.org/public/v3/models/measurement-set.js:58
>      fetchBetween(startTime, endTime, callback)

You should mention that you're rewriting these functions in the change log,
and what the differences between the old & the new implementations are.

> Websites/perf.webkit.org/public/v3/models/measurement-set.js:65
> +            .then(function() {

We usually put .then(function () { in the previous line.
Also note that we need a space between "function" and "()".

> Websites/perf.webkit.org/public/v3/models/measurement-set.js:122
> +            this._fetchedPrimary = true;

I don't think this instance variable is no longer used in the promise-based implementation.

> Websites/perf.webkit.org/public/v3/pages/heading.js:19
> +        group = group.filter(function (page) { return page; });

We should do this at the call site instead of magically filtering it in this function.
In general, we should try to keep helper functions, components, modules, etc... as simple as possible
and put as much convoluted logics at the application level (i.e. main.js, -page.js).
Otherwise, we'd end up having to second guess what each function/class *might* do.

ComponentBase.renderReplace (along with other methods on that class) is an exception
since the whole point of that function is helping each component instantiate DOM nodes easily.
(i.e. absorb the complexity in favor of simplifying call sites)

> Websites/perf.webkit.org/public/v3/pages/heading.js:20
>          for (var page of group)

We should add "console.assert(page instanceof Page)" here... (don't need to do it in this patch).

> Websites/perf.webkit.org/public/v3/pages/page-router.js:16
> +        if(!page)
> +            return;

The same thing here.  It would be weird for someone to be calling addPage and then this function silently ignored it.
We'd have to debug or read the code to figure out that page was null.
It's better if this code just threw an exception instead.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:15
> +        this._excludedConfigurations = summarySettings.excludedConfigurations;

Please mention this feature in the change log.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:81
> +                    headings = [element('th', {class: 'minorHeader'}, row.name)];
> +                    if (!rowIndex)
> +                        headings.unshift(element('th', {class: 'majorHeader', rowspan: rowGroup.rows.length}, rowGroup.name));

Also explain what this change does.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:97
> +        var anchor = link(ratioGraph, 'Open charts', this.router().url('charts', state));

Now that I think about more, we don't need to instantiate the link element here at all.
We just need to store 'Open charts' to a local variable like 'anchorLabel' and then use that at the last line of this function.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:187
> +        this._warnings = new Map;

We don't really need to use a Map here since we're only ever going to use string keys.
Also, I'd prefer initializing this to `null` in the case there are no warnings.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:197
> +                if (excludedConfigurations && excludedConfigurations.hasOwnProperty(platform.id()) && excludedConfigurations[platform.id()].includes(+metric.id()))

Just do "platform.id() in excludedConfigurations" instead.
Also, there's no need to convert metric.id() to a number.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:262
> +            var platformName = Platform.findById(set.platformId()).name();

Please just store the platform object instead of returning its name.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:264
> +                var missingBaselinePlatforms = self._warnings.get('Missing baseline') || new Set;

Why don't we add "Missing" when we construct the string in render instead?

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:266
> +                self._warnings.set('Missing baseline', missingBaselinePlatforms);

I'd prefer to have a pattern like:
if (!(y in x))
  x[y] = ~;
x[y].add(~).
since that's what we do elsewhere in the perf dashboard.

Now, there isn't really much reason to be using a Set here
because we're not gonna have same platforms appearing multiple times
(we should probably verify that somewhere)
so I'd suggest using an array instead to make the code to construct the string simpler.

Note that Set and Map are orders of magnitudes (2x to 200x) slower on almost all browsers.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:269
> +                var missingCurrentPlatforms = self._warnings.get('Missing current') || new Set;

Ditto.

> Websites/perf.webkit.org/unit-tests/measurement-set-tests.js:74
> +        function waitForMeasurementSet()

You should mention what this function does / why these test changes are needed.

Comment on attachment 278069 [details]
Patch

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

>> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:49
>> +        if (this._ratio) {
> 
> You should check that this._ratio is not NaN here.

I don't think it will satisfy the condition when this._ratio equals NaN.

>> Websites/perf.webkit.org/public/v3/pages/summary-page.js:187
>> +        this._warnings = new Map;
> 
> We don't really need to use a Map here since we're only ever going to use string keys.
> Also, I'd prefer initializing this to `null` in the case there are no warnings.

I agree that using Map is unnecessary. But if we initialize warnings to `null` we need to do null-check every time we use it. I think using {} will simplify the code.

>> Websites/perf.webkit.org/public/v3/pages/summary-page.js:197
>> +                if (excludedConfigurations && excludedConfigurations.hasOwnProperty(platform.id()) && excludedConfigurations[platform.id()].includes(+metric.id()))
> 
> Just do "platform.id() in excludedConfigurations" instead.
> Also, there's no need to convert metric.id() to a number.

[1].includes("1") returns false. And metric.id() returns a string instead of a number. So I think the conversion is necessary.

>> Websites/perf.webkit.org/public/v3/pages/summary-page.js:266
>> +                self._warnings.set('Missing baseline', missingBaselinePlatforms);
> 
> I'd prefer to have a pattern like:
> if (!(y in x))
>   x[y] = ~;
> x[y].add(~).
> since that's what we do elsewhere in the perf dashboard.
> 
> Now, there isn't really much reason to be using a Set here
> because we're not gonna have same platforms appearing multiple times
> (we should probably verify that somewhere)
> so I'd suggest using an array instead to make the code to construct the string simpler.
> 
> Note that Set and Map are orders of magnitudes (2x to 200x) slower on almost all browsers.

I can see same platform occurs multiple times here. Because pair of <platform, metric> is unique.

Created attachment 278151 [details]
Patch

Comment on attachment 278151 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:27
> +        (MeasurementSet.prototype.fetchBetween): Returns a promise. Fix the bug in previous implementation that we miss some callbacks sometimes. Basically, we will fetch primary cluster first, then secondary clusters. For each secondary cluster fetch, we will always invoke callback even when it fail.

This should be "when it fails".

[Ryosuke Niwa]

Comment on attachment 278151 [details]
Patch

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

>> Websites/perf.webkit.org/ChangeLog:27
>> +        (MeasurementSet.prototype.fetchBetween): Returns a promise. Fix the bug in previous implementation that we miss some callbacks sometimes. Basically, we will fetch primary cluster first, then secondary clusters. For each secondary cluster fetch, we will always invoke callback even when it fail.
> 
> This should be "when it fails".

Please break lines after 120 characters or so instead of putting the entire comment in a single line.

> Websites/perf.webkit.org/ChangeLog:41
> +        (SummaryPageConfigurationGroup.prototype.warnings):
> +        (SummaryPageConfigurationGroup.prototype.fetchAndComputeSummary.set return):
> +        (SummaryPageConfigurationGroup.return.set fetchBetween):
> +        (SummaryPageConfigurationGroup.set then): Deleted.
> +        (SummaryPageConfigurationGroup.set var): Deleted.

Please update this list with the list of actually modified functions and describe changes to computeSummary.

> Websites/perf.webkit.org/ChangeLog:42
> +        * unit-tests/measurement-set-tests.js: Add a helper function to wait for fetchBetween. Update unit tests since fetchBetween returns a promise now.

We should also mention that we need to wait longer for fetchBetween now due to promise chains.

> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:31
> +        var validClassNames = ['better', 'worse'];
> +        for (var name of validClassNames)
> +            this._ratioBarGraph.classList.remove(name);
>  

On my second thought, we should just put better/worse classes on div.bar & div.label and just CSS rules.
That'll simplify this code.

> Websites/perf.webkit.org/public/v3/models/measurement-set.js:66
> +            var promiseList = [];
> +            for (var clusterEndTime of self.findClusters(startTime, endTime)) {

I guess we should just use .map instead:
self.findClusters(startTime, endTime).map(function (clusterEndTime) {
...
})

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:97
> +        var anchor = link(ratioGraph, 'Open charts', this.router().url('charts', state));

I think omit 'Open charts' here:
link(ratioGraph, this.router().url('charts', state));

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:103
> +                warningText += 'Missing ' + type + ' for following platform(s): ' + Array.from(platforms).map(function (platform) { return platform.name(); }).join(', ');

Use template literal here: `Missing ${type} for following platform(s): ${platformList}` where platformList is the result of Array.from...

Created attachment 278155 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 278155 [details]
Patch for landing

Clearing flags on attachment: 278155

Committed r200449: <http://trac.webkit.org/changeset/200449>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.