Perf dashboard can consume 50-70% of CPU on MacBook even if user is not interacting...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 9 Jul 2016 22:58:54 +0000 (22:58 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 9 Jul 2016 22:58:54 +0000 (22:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=159597

Reviewed by Chris Dumez.

TimeSeriesChart and InteractiveTimeSeriesChart had been relying on continually polling on requestAnimationFrame
to update itself in response to its canvas resizing. Even though there as an early exit in the case there was
nothing to update, this is still causing a significant power drain when the user is not interacting at all.

Let TimeSeriesChart use the regular top-down render path like other components with exceptions of listening to
window's resize eventas well as when new JSONs are fetched from the server. The render() call to the latter case
will be coerced into a single callback on requestAnimationFrame to avoid DOM-mutation-layout churn.

* public/v3/components/base.js:
(ComponentBase.isElementInViewport): Deleted.
* public/v3/components/chart-pane-base.js:
(ChartPaneBase.prototype.render): Enqueue charts to render.
* public/v3/components/chart-styles.js:
(ChartStyles.dashboardOptions): Removed updateOnRequestAnimationFrame which is no longer an available option.
* public/v3/components/time-series-chart.js:
(TimeSeriesChart): Replaced the code to register itself for rAF by the code to listen to resize events on window.
(TimeSeriesChart._updateOnRAF): Deleted.
(TimeSeriesChart._updateAllCharts): Added.
(TimeSeriesChart.prototype._enqueueToRender): Added.
(TimeSeriesChart._renderEnqueuedCharts): Added.
(TimeSeriesChart.prototype.fetchMeasurementSets): Avoid calling fetchBetween when the range had been fetched.
Without this change, we can incur a significant number of redundant calls to render() when adjusting the domain
in charts page by the slider. When no new JSON is fetched, simply enqueue this chart to render on rAF.
(TimeSeriesChart.prototype._didFetchMeasurementSet): Enqueue this chart to render on rAF.
(TimeSeriesChart.prototype.render): Removed the check for isElementInViewport since we no longer get render() call
when this chart moves into the viewport (as we no longer listen to every rAF or scroll event).
* public/v3/models/measurement-set.js:
(MeasurementSet.prototype.hasFetchedRange): Fixed various bugs revealed by the new use in fetchMeasurementSets.
* public/v3/pages/chart-pane-status-view.js:
(ChartPaneStatusView.prototype._updateRevisionListForNewCurrentRepository): Removed the dead code. It was probably
copied from when this code was in InteractiveTimeSeries chart. There is no this._forceRender in this component.
* public/v3/pages/dashboard-page.js:
(DashboardPage.prototype.render): Enqueue charts to render.
* public/v3/pages/heading.js:
(SummaryPage.prototype.open): Removed the call to isElementInViewport. This wasn't really doing anything useful in
this component.
* unit-tests/measurement-set-tests.js: Added tests for hasFetchedRange.
(.waitForMeasurementSet): Moved to be used in a newly added test case.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@203035 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/v3/components/base.js
Websites/perf.webkit.org/public/v3/components/chart-pane-base.js
Websites/perf.webkit.org/public/v3/components/chart-styles.js
Websites/perf.webkit.org/public/v3/components/time-series-chart.js
Websites/perf.webkit.org/public/v3/models/measurement-set.js
Websites/perf.webkit.org/public/v3/pages/chart-pane-status-view.js
Websites/perf.webkit.org/public/v3/pages/dashboard-page.js
Websites/perf.webkit.org/public/v3/pages/heading.js
Websites/perf.webkit.org/unit-tests/measurement-set-tests.js

index b99638f..b1eed94 100644 (file)
@@ -1,5 +1,51 @@
 2016-07-09  Ryosuke Niwa  <rniwa@webkit.org>
 
+        Perf dashboard can consume 50-70% of CPU on MacBook even if user is not interacting at all
+        https://bugs.webkit.org/show_bug.cgi?id=159597
+
+        Reviewed by Chris Dumez.
+
+        TimeSeriesChart and InteractiveTimeSeriesChart had been relying on continually polling on requestAnimationFrame
+        to update itself in response to its canvas resizing. Even though there as an early exit in the case there was
+        nothing to update, this is still causing a significant power drain when the user is not interacting at all.
+
+        Let TimeSeriesChart use the regular top-down render path like other components with exceptions of listening to
+        window's resize eventas well as when new JSONs are fetched from the server. The render() call to the latter case
+        will be coerced into a single callback on requestAnimationFrame to avoid DOM-mutation-layout churn.
+
+        * public/v3/components/base.js:
+        (ComponentBase.isElementInViewport): Deleted.
+        * public/v3/components/chart-pane-base.js:
+        (ChartPaneBase.prototype.render): Enqueue charts to render.
+        * public/v3/components/chart-styles.js:
+        (ChartStyles.dashboardOptions): Removed updateOnRequestAnimationFrame which is no longer an available option.
+        * public/v3/components/time-series-chart.js:
+        (TimeSeriesChart): Replaced the code to register itself for rAF by the code to listen to resize events on window.
+        (TimeSeriesChart._updateOnRAF): Deleted.
+        (TimeSeriesChart._updateAllCharts): Added.
+        (TimeSeriesChart.prototype._enqueueToRender): Added.
+        (TimeSeriesChart._renderEnqueuedCharts): Added.
+        (TimeSeriesChart.prototype.fetchMeasurementSets): Avoid calling fetchBetween when the range had been fetched.
+        Without this change, we can incur a significant number of redundant calls to render() when adjusting the domain
+        in charts page by the slider. When no new JSON is fetched, simply enqueue this chart to render on rAF.
+        (TimeSeriesChart.prototype._didFetchMeasurementSet): Enqueue this chart to render on rAF.
+        (TimeSeriesChart.prototype.render): Removed the check for isElementInViewport since we no longer get render() call
+        when this chart moves into the viewport (as we no longer listen to every rAF or scroll event).
+        * public/v3/models/measurement-set.js:
+        (MeasurementSet.prototype.hasFetchedRange): Fixed various bugs revealed by the new use in fetchMeasurementSets.
+        * public/v3/pages/chart-pane-status-view.js:
+        (ChartPaneStatusView.prototype._updateRevisionListForNewCurrentRepository): Removed the dead code. It was probably
+        copied from when this code was in InteractiveTimeSeries chart. There is no this._forceRender in this component.
+        * public/v3/pages/dashboard-page.js:
+        (DashboardPage.prototype.render): Enqueue charts to render. 
+        * public/v3/pages/heading.js:
+        (SummaryPage.prototype.open): Removed the call to isElementInViewport. This wasn't really doing anything useful in
+        this component.
+        * unit-tests/measurement-set-tests.js: Added tests for hasFetchedRange.
+        (.waitForMeasurementSet): Moved to be used in a newly added test case.
+
+2016-07-09  Ryosuke Niwa  <rniwa@webkit.org>
+
         REGRESSION: manifest.json generation takes multiple seconds on perf dashboard
         https://bugs.webkit.org/show_bug.cgi?id=159596
 
index 4f7ab83..bee9859 100644 (file)
@@ -75,16 +75,6 @@ class ComponentBase {
         }
     }
 
-    static isElementInViewport(element)
-    {
-        var viewportHeight = window.innerHeight;
-        var boundingRect = element.getBoundingClientRect();
-        if (viewportHeight < boundingRect.top || boundingRect.bottom < 0
-            || !boundingRect.width || !boundingRect.height)
-            return false;
-        return true;
-    }
-
     static defineElement(name, elementInterface)
     {
         if (!ComponentBase._map)
index 95bfaba..04b75e3 100644 (file)
@@ -197,6 +197,12 @@ class ChartPaneBase extends ComponentBase {
 
         super.render();
 
+        if (this._overviewChart)
+            this._overviewChart.enqueueToRender();
+
+        if (this._mainChart)
+            this._mainChart.enqueueToRender();
+
         if (this._errorMessage) {
             this.renderReplace(this.content().querySelector('.chart-pane-main'), this._errorMessage);
             return;
index 09dfd2b..ef4a8d6 100644 (file)
@@ -87,7 +87,6 @@ class ChartStyles {
     static dashboardOptions(valueFormatter)
     {
         return {
-            updateOnRequestAnimationFrame: true,
             axis: {
                 yAxisWidth: 4, // rem
                 xAxisHeight: 2, // rem
index ba00f86..459c1cf 100644 (file)
@@ -21,12 +21,11 @@ class TimeSeriesChart extends ComponentBase {
         this._contextScaleY = 1;
         this._rem = null;
 
-        if (this._options.updateOnRequestAnimationFrame) {
-            if (!TimeSeriesChart._chartList)
-                TimeSeriesChart._chartList = [];
-            TimeSeriesChart._chartList.push(this);
-            TimeSeriesChart._updateOnRAF();
+        if (!TimeSeriesChart._chartList) {
+            TimeSeriesChart._chartList = [];
+            window.addEventListener('resize', TimeSeriesChart._updateAllCharts.bind(TimeSeriesChart));
         }
+        TimeSeriesChart._chartList.push(this);
     }
 
     _ensureCanvas()
@@ -51,14 +50,25 @@ class TimeSeriesChart extends ComponentBase {
         return document.createElement('canvas');
     }
 
-    static _updateOnRAF()
+    static _updateAllCharts()
     {
-        var self = this;
-        window.requestAnimationFrame(function ()
-        {
-            TimeSeriesChart._chartList.map(function (chart) { chart.render(); });
-            self._updateOnRAF();
-        });
+        TimeSeriesChart._chartList.map(function (chart) { chart.render(); });
+    }
+
+    enqueueToRender()
+    {
+        if (!TimeSeriesChart._chartQueue) {
+            TimeSeriesChart._chartQueue = new Set;
+            window.requestAnimationFrame(TimeSeriesChart._renderEnqueuedCharts.bind(TimeSeriesChart));
+        }
+        TimeSeriesChart._chartQueue.add(this);
+    }
+
+    static _renderEnqueuedCharts()
+    {
+        for (var chart of TimeSeriesChart._chartQueue)
+            chart.render();
+        TimeSeriesChart._chartQueue = null;
     }
 
     setDomain(startTime, endTime)
@@ -77,13 +87,21 @@ class TimeSeriesChart extends ComponentBase {
 
     fetchMeasurementSets(noCache)
     {
+        var fetching = false;
         for (var source of this._sourceList) {
-            if (source.measurementSet)
+            if (source.measurementSet) {
+                if (source.measurementSet.hasFetchedRange(this._startTime, this._endTime))
+                    continue;
                 source.measurementSet.fetchBetween(this._startTime, this._endTime, this._didFetchMeasurementSet.bind(this, source.measurementSet), noCache);
+                fetching = true;
+            }
+
         }
         this._sampledTimeSeriesData = null;
         this._valueRangeCache = null;
         this._annotationRows = null;
+        if (!fetching)
+            this.enqueueToRender();
     }
 
     _didFetchMeasurementSet(set)
@@ -92,6 +110,8 @@ class TimeSeriesChart extends ComponentBase {
         this._sampledTimeSeriesData = null;
         this._valueRangeCache = null;
         this._annotationRows = null;
+
+        this.enqueueToRender();
     }
 
     // FIXME: Figure out a way to make this readonly.
@@ -135,8 +155,6 @@ class TimeSeriesChart extends ComponentBase {
 
         // FIXME: Also detect horizontal scrolling.
         var canvas = this._ensureCanvas();
-        if (!TimeSeriesChart.isElementInViewport(canvas))
-            return;
 
         var metrics = this._layout();
         if (!metrics.doneWork)
index bdac82c..e00d7d6 100644 (file)
@@ -150,18 +150,23 @@ class MeasurementSet {
     hasFetchedRange(startTime, endTime)
     {
         console.assert(startTime < endTime);
-        var hasHole = false;
+        var foundStart = false;
         var previousEndTime = null;
         for (var cluster of this._sortedClusters) {
-            if (cluster.startTime() < startTime && startTime < cluster.endTime())
-                hasHole = false;
-            if (previousEndTime !== null && previousEndTime != cluster.startTime())
-                hasHole = true;
-            if (cluster.startTime() < endTime && endTime < cluster.endTime())
-                break;
+            var containsStart = cluster.startTime() <= startTime && startTime <= cluster.endTime();
+            var containsEnd = cluster.startTime() <= endTime && endTime <= cluster.endTime();
+            var preceedingClusterIsMissing = previousEndTime !== null && previousEndTime != cluster.startTime();
+            if (containsStart && containsEnd)
+                return true;
+            if (containsStart)
+                foundStart = true;
+            if (foundStart && preceedingClusterIsMissing)
+                return false;
+            if (containsEnd)
+                return foundStart; // Return true iff there were not missing clusters from the one that contains startTime
             previousEndTime = cluster.endTime();
         }
-        return !hasHole;
+        return false; // Didn't find a cluster that contains startTime or endTime
     }
 
     fetchedTimeSeries(configType, includeOutliers, extendToFuture)
index 91b2c16..59ce2fe 100644 (file)
@@ -117,7 +117,6 @@ class ChartPaneStatusView extends ChartStatusView {
     {
         this.updateStatusIfNeeded();
 
-        this._forceRender = true;
         for (var info of this._revisionList) {
             if (info.repository == this._currentRepository) {
                 this._setRevisionRange(false, info.repository, info.from, info.to);
index b2d5d44..1f72264 100644 (file)
@@ -109,6 +109,9 @@ class DashboardPage extends PageWithHeading {
             this._needsTableConstruction = false;
         }
 
+        for (var chart of this._charts)
+            chart.enqueueToRender();
+
         if (this._needsStatusUpdate) {
             for (var statusView of this._statusViews)
                 statusView.render();
index 003ac1e..dae1ef4 100644 (file)
@@ -52,8 +52,7 @@ class Heading extends ComponentBase {
         if (this._toolbar)
             this._toolbar.render();
 
-        // Workaround the bounding rects being 0x0 when the content is empty.
-        if (this._renderedOnce && !Heading.isElementInViewport(this.element()))
+        if (this._renderedOnce)
             return;
 
         var title = this.content().querySelector('.heading-title a');
index 0ac4fc5..f1db938 100644 (file)
@@ -14,6 +14,15 @@ describe('MeasurementSet', function () {
         MeasurementSet._set = null;
     });
 
+    function waitForMeasurementSet()
+    {
+        return Promise.resolve().then(function () {
+            return Promise.resolve();
+        }).then(function () {
+            return Promise.resolve();
+        });
+    }
+
     describe('findSet', function () {
         it('should create a new MeasurementSet for a new pair of platform and matric', function () {
             assert.notEqual(MeasurementSet.findSet(1, 1, 3000), MeasurementSet.findSet(1, 2, 3000));
@@ -71,15 +80,6 @@ describe('MeasurementSet', function () {
             });
         });
 
-        function waitForMeasurementSet()
-        {
-            return Promise.resolve().then(function () {
-                return Promise.resolve();
-            }).then(function () {
-                return Promise.resolve();
-            });
-        }
-
         it('should invoke the callback and fetch a secondary cluster when the cached primary cluster is up-to-date and within in the requested range', function (done) {
             var set = MeasurementSet.findSet(1, 1, 3000);
             var callCount = 0;
@@ -517,4 +517,168 @@ describe('MeasurementSet', function () {
 
     });
 
+    describe('hasFetchedRange', function () {
+
+        it('should return false when no clusters had been fetched', function () {
+            var set = MeasurementSet.findSet(1, 1, 3000);
+            assert(!set.hasFetchedRange(2000, 3000));
+        });
+
+        it('should return true when a single cluster contains the entire range', function (done) {
+            var set = MeasurementSet.findSet(1, 1, 3000);
+            var promise = set.fetchBetween(2000, 3000);
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, '../data/measurement-set-1-1.json');
+
+            requests[0].resolve({
+                'clusterStart': 1000,
+                'clusterSize': 1000,
+                'formatMap': [],
+                'configurations': {current: []},
+                'startTime': 2000,
+                'endTime': 3000,
+                'lastModified': 3000,
+                'clusterCount': 2,
+                'status': 'OK'});
+
+            promise.then(function () {
+                assert(set.hasFetchedRange(2001, 2999));
+                assert(set.hasFetchedRange(2000, 3000));
+                done();
+            }).catch(function (error) {
+                done(error);
+            });
+        });
+
+        it('should return false when the range starts before the fetched cluster', function (done) {
+            var set = MeasurementSet.findSet(1, 1, 3000);
+            var promise = set.fetchBetween(2000, 3000);
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, '../data/measurement-set-1-1.json');
+
+            requests[0].resolve({
+                'clusterStart': 1000,
+                'clusterSize': 1000,
+                'formatMap': [],
+                'configurations': {current: []},
+                'startTime': 2000,
+                'endTime': 3000,
+                'lastModified': 3000,
+                'clusterCount': 2,
+                'status': 'OK'});
+
+            promise.then(function () {
+                assert(!set.hasFetchedRange(1500, 3000));
+                done();
+            }).catch(function (error) {
+                done(error);
+            });
+        });
+
+        it('should return false when the range ends after the fetched cluster', function (done) {
+            var set = MeasurementSet.findSet(1, 1, 3000);
+            var promise = set.fetchBetween(2000, 3000);
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, '../data/measurement-set-1-1.json');
+
+            requests[0].resolve({
+                'clusterStart': 1000,
+                'clusterSize': 1000,
+                'formatMap': [],
+                'configurations': {current: []},
+                'startTime': 2000,
+                'endTime': 3000,
+                'lastModified': 3000,
+                'clusterCount': 2,
+                'status': 'OK'});
+
+            promise.then(function () {
+                assert(!set.hasFetchedRange(2500, 3500));
+                done();
+            }).catch(function (error) {
+                done(error);
+            });
+        });
+
+        it('should return true when the range is within two fetched clusters', function (done) {
+            var set = MeasurementSet.findSet(1, 1, 5000);
+            var promise = set.fetchBetween(2000, 3000);
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, '../data/measurement-set-1-1.json');
+
+            requests[0].resolve({
+                'clusterStart': 1000,
+                'clusterSize': 1000,
+                'formatMap': [],
+                'configurations': {current: []},
+                'startTime': 3000,
+                'endTime': 4000,
+                'lastModified': 5000,
+                'clusterCount': 2,
+                'status': 'OK'});
+
+            waitForMeasurementSet().then(function () {
+                assert.equal(requests.length, 2);
+                assert.equal(requests[1].url, '../data/measurement-set-1-1-3000.json');
+                requests[1].resolve({
+                    'clusterStart': 1000,
+                    'clusterSize': 1000,
+                    'formatMap': [],
+                    'configurations': {current: []},
+                    'startTime': 2000,
+                    'endTime': 3000,
+                    'lastModified': 5000,
+                    'clusterCount': 2,
+                    'status': 'OK'});                
+            }).then(function () {
+                assert(set.hasFetchedRange(2500, 3500));
+                done();
+            }).catch(function (error) {
+                done(error);
+            });
+        });
+
+        it('should return false when there is a cluster missing in the range', function (done) {
+            var set = MeasurementSet.findSet(1, 1, 5000);
+            var promise = set.fetchBetween(2000, 5000);
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, '../data/measurement-set-1-1.json');
+
+            requests[0].resolve({
+                'clusterStart': 1000,
+                'clusterSize': 1000,
+                'formatMap': [],
+                'configurations': {current: []},
+                'startTime': 4000,
+                'endTime': 5000,
+                'lastModified': 5000,
+                'clusterCount': 2,
+                'status': 'OK'});
+
+            waitForMeasurementSet().then(function () {
+                assert.equal(requests.length, 3);
+                assert.equal(requests[1].url, '../data/measurement-set-1-1-3000.json');
+                assert.equal(requests[2].url, '../data/measurement-set-1-1-4000.json');
+                requests[1].resolve({
+                    'clusterStart': 1000,
+                    'clusterSize': 1000,
+                    'formatMap': [],
+                    'configurations': {current: []},
+                    'startTime': 2000,
+                    'endTime': 3000,
+                    'lastModified': 5000,
+                    'clusterCount': 2,
+                    'status': 'OK'});
+            }).then(function () {
+                assert(!set.hasFetchedRange(2500, 4500));
+                assert(set.hasFetchedRange(2100, 2300));
+                assert(set.hasFetchedRange(4000, 4800));
+                done();
+            }).catch(function (error) {
+                done(error);
+            });
+        });
+
+    });
+
 });