From b7f4539cdb259406fe505a355d6d34951b8ce8b8 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 26 Jan 2015 15:09:56 -0800 Subject: [PATCH 1/6] [Performance] Add watch indicator Add a watch indicator to the example bundles as a developer support tool. Added to help support sluggish plots, WTD-717. --- example/profiling/bundle.json | 10 ++++ example/profiling/src/WatchIndicator.js | 75 +++++++++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 example/profiling/bundle.json create mode 100644 example/profiling/src/WatchIndicator.js diff --git a/example/profiling/bundle.json b/example/profiling/bundle.json new file mode 100644 index 0000000000..b6090717d2 --- /dev/null +++ b/example/profiling/bundle.json @@ -0,0 +1,10 @@ +{ + "extensions": { + "indicators": [ + { + "implementation": "WatchIndicator.js", + "depends": ["$interval", "$rootScope"] + } + ] + } +} \ No newline at end of file diff --git a/example/profiling/src/WatchIndicator.js b/example/profiling/src/WatchIndicator.js new file mode 100644 index 0000000000..61fa917984 --- /dev/null +++ b/example/profiling/src/WatchIndicator.js @@ -0,0 +1,75 @@ +/*global define*/ + +define( + [], + function () { + "use strict"; + + /** + * Updates a count of currently-active Angular watches. + * @constructor + * @param $interval Angular's $interval + */ + function WatchIndicator($interval, $rootScope) { + var watches = 0; + + function count(scope) { + if (scope) { + watches += (scope.$$watchers || []).length; + count(scope.$$childHead); + count(scope.$$nextSibling); + } + } + + function update() { + watches = 0; + count($rootScope); + } + + // Update state every second + $interval(update, 1000); + + // Provide initial state, too + update(); + + return { + /** + * Get the glyph (single character used as an icon) + * to display in this indicator. This will return ".", + * which should appear as a dataflow icon. + * @returns {string} the character of the database icon + */ + getGlyph: function () { + return "A"; + }, + /** + * Get the name of the CSS class to apply to the glyph. + * This is used to color the glyph to match its + * state (one of ok, caution or err) + * @returns {string} the CSS class to apply to this glyph + */ + getGlyphClass: function () { + return undefined; + }, + /** + * Get the text that should appear in the indicator. + * @returns {string} brief summary of connection status + */ + getText: function () { + return "Watches " + watches; + }, + /** + * Get a longer-form description of the current connection + * space, suitable for display in a tooltip + * @returns {string} longer summary of connection status + */ + getDescription: function () { + return ""; + } + }; + } + + return WatchIndicator; + + } +); \ No newline at end of file From a9f7cc9658c7b8777a0b142f04db266d0c82e413 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 26 Jan 2015 17:48:33 -0800 Subject: [PATCH 2/6] [Performance] Deactive mct-resize on destroy Deactive mct-resize when its containing scope is destroyed; that is, don't go on polling for size changes indefinitely. Issue discovered/addressed in the context of investigating sluggishness of plotting in WTD-717. --- .../commonUI/general/src/directives/MCTResize.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/platform/commonUI/general/src/directives/MCTResize.js b/platform/commonUI/general/src/directives/MCTResize.js index 10e4256c50..9e6d5a29f1 100644 --- a/platform/commonUI/general/src/directives/MCTResize.js +++ b/platform/commonUI/general/src/directives/MCTResize.js @@ -35,7 +35,8 @@ define( // Link; start listening for changes to an element's size function link(scope, element, attrs) { - var lastBounds; + var lastBounds, + active = true; // Determine how long to wait before the next update function currentInterval() { @@ -62,9 +63,19 @@ define( width: element[0].offsetWidth, height: element[0].offsetHeight }); - $timeout(onInterval, currentInterval()); + if (active) { + $timeout(onInterval, currentInterval()); + } } + // Stop running in the background + function deactivate() { + active = false; + } + + // Unregister once out-of-scope + scope.$on("$destroy", deactivate); + // Handle the initial callback onInterval(); } From 5ee872713f32aafc883c03bbc7510c0dda719feb Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 26 Jan 2015 17:56:52 -0800 Subject: [PATCH 3/6] [Performance] Release interval from mct-chart Stop checking for size changes after scope is destroyed in mct-chart; part of ongoing resolution of resource leaks which may contribute to WTD-717. --- platform/features/plot/src/MCTChart.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/platform/features/plot/src/MCTChart.js b/platform/features/plot/src/MCTChart.js index 5c01bc6efc..586d581fc7 100644 --- a/platform/features/plot/src/MCTChart.js +++ b/platform/features/plot/src/MCTChart.js @@ -46,6 +46,7 @@ define( function linkChart(scope, element) { var canvas = element.find("canvas")[0], + releaseInterval, chart; // Try to initialize GLChart, which allows drawing using WebGL. @@ -111,11 +112,14 @@ define( } // Check for resize, on a timer - $interval(drawIfResized, 1000); + releaseInterval = $interval(drawIfResized, 1000); // Watch "draw" for external changes to the set of // things to be drawn. scope.$watchCollection("draw", doDraw); + + // Stop checking for resize when scope is destroyed + scope.$on("$destroy", releaseInterval); } return { From d192d611f919aa4f2f944c84e4dd33694a4bb821 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Tue, 27 Jan 2015 08:38:18 -0800 Subject: [PATCH 4/6] [Performance] Colorize watch counter Colorize the watch counter when it exceeds 2000, to aid in detecting/diagnosing issues like WTD-717. --- example/profiling/src/WatchIndicator.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/example/profiling/src/WatchIndicator.js b/example/profiling/src/WatchIndicator.js index 61fa917984..03a484927c 100644 --- a/example/profiling/src/WatchIndicator.js +++ b/example/profiling/src/WatchIndicator.js @@ -40,7 +40,7 @@ define( * @returns {string} the character of the database icon */ getGlyph: function () { - return "A"; + return "E"; }, /** * Get the name of the CSS class to apply to the glyph. @@ -49,14 +49,16 @@ define( * @returns {string} the CSS class to apply to this glyph */ getGlyphClass: function () { - return undefined; + return (watches > 2000) ? "caution" : + (watches < 1000) ? "ok" : + undefined; }, /** * Get the text that should appear in the indicator. * @returns {string} brief summary of connection status */ getText: function () { - return "Watches " + watches; + return watches + " watches"; }, /** * Get a longer-form description of the current connection From 6947e0aa42ab21785c7be1f899628a85b5c8fd71 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Tue, 27 Jan 2015 08:50:09 -0800 Subject: [PATCH 5/6] [Performance] Update specs Add tests to verify that directives stop polling after their scope is destroyed (to prevent resource leaks); those changes address resource leaks identified in the context of WTD-717. --- .../general/test/directives/MCTResizeSpec.js | 28 ++++++++++++++++++- platform/features/plot/test/MCTChartSpec.js | 25 ++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/platform/commonUI/general/test/directives/MCTResizeSpec.js b/platform/commonUI/general/test/directives/MCTResizeSpec.js index 4f07701d50..b320d5e881 100644 --- a/platform/commonUI/general/test/directives/MCTResizeSpec.js +++ b/platform/commonUI/general/test/directives/MCTResizeSpec.js @@ -14,7 +14,7 @@ define( beforeEach(function () { mockTimeout = jasmine.createSpy("$timeout"); - mockScope = jasmine.createSpyObj("$scope", ["$eval"]); + mockScope = jasmine.createSpyObj("$scope", ["$eval", "$on"]); testElement = { offsetWidth: 100, offsetHeight: 200 }; testAttrs = { mctResize: "some-expr" }; @@ -63,6 +63,32 @@ define( ); }); + it("stops size checking for size changes after destroy", function () { + mctResize.link(mockScope, [testElement], testAttrs); + + // First, make sure there's a $destroy observer + expect(mockScope.$on) + .toHaveBeenCalledWith("$destroy", jasmine.any(Function)); + + // Should have scheduled the first timeout + expect(mockTimeout.calls.length).toEqual(1); + + // Fire the timeout + mockTimeout.mostRecentCall.args[0](); + + // Should have scheduled another timeout + expect(mockTimeout.calls.length).toEqual(2); + + // Broadcast a destroy event + mockScope.$on.mostRecentCall.args[1](); + + // Fire the timeout + mockTimeout.mostRecentCall.args[0](); + + // Should NOT have scheduled another timeout + expect(mockTimeout.calls.length).toEqual(2); + }); + }); } ); \ No newline at end of file diff --git a/platform/features/plot/test/MCTChartSpec.js b/platform/features/plot/test/MCTChartSpec.js index 84d6a27368..837f8d0ef7 100644 --- a/platform/features/plot/test/MCTChartSpec.js +++ b/platform/features/plot/test/MCTChartSpec.js @@ -15,6 +15,7 @@ define( mockElement, mockCanvas, mockGL, + mockCancelInterval, mctChart; beforeEach(function () { @@ -23,9 +24,10 @@ define( mockLog = jasmine.createSpyObj("$log", ["warn", "info", "debug"]); mockScope = - jasmine.createSpyObj("$scope", ["$watchCollection"]); + jasmine.createSpyObj("$scope", ["$watchCollection", "$on"]); mockElement = jasmine.createSpyObj("element", ["find"]); + mockCancelInterval = jasmine.createSpy("cancelInterval"); // mct-chart uses GLChart, so it needs WebGL API @@ -70,6 +72,7 @@ define( mockElement.find.andReturn([mockCanvas]); mockCanvas.getContext.andReturn(mockGL); + mockInterval.andReturn(mockCancelInterval); mctChart = new MCTChart(mockInterval, mockLog); }); @@ -150,6 +153,26 @@ define( expect(mockLog.warn).not.toHaveBeenCalled(); }); + // Avoid resource leaks + it("stops polling for size changes on destroy", function () { + mctChart.link(mockScope, mockElement); + + // Should be listening for a destroy event + expect(mockScope.$on).toHaveBeenCalledWith( + "$destroy", + jasmine.any(Function) + ); + + // Precondition - interval still active + expect(mockCancelInterval).not.toHaveBeenCalled(); + + // Broadcast a $destroy + mockScope.$on.mostRecentCall.args[1](); + + // Should have stopped the interval + expect(mockCancelInterval).toHaveBeenCalled(); + }); + }); } ); \ No newline at end of file From a0f216764b6b211eefe42690301e149de956c1d8 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Wed, 28 Jan 2015 15:53:13 -0800 Subject: [PATCH 6/6] [Plot] Cancel interval correctly Fix approach used for interval cancellation associated with resource leak closing for WTD-717; this avoids the exception observed in WTD-750. --- platform/features/plot/src/MCTChart.js | 11 +++++++++-- platform/features/plot/test/MCTChartSpec.js | 11 ++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/platform/features/plot/src/MCTChart.js b/platform/features/plot/src/MCTChart.js index 586d581fc7..43ebaa891f 100644 --- a/platform/features/plot/src/MCTChart.js +++ b/platform/features/plot/src/MCTChart.js @@ -46,7 +46,7 @@ define( function linkChart(scope, element) { var canvas = element.find("canvas")[0], - releaseInterval, + activeInterval, chart; // Try to initialize GLChart, which allows drawing using WebGL. @@ -111,8 +111,15 @@ define( } } + // Stop watching for changes to size (scope destroyed) + function releaseInterval() { + if (activeInterval) { + $interval.cancel(activeInterval); + } + } + // Check for resize, on a timer - releaseInterval = $interval(drawIfResized, 1000); + activeInterval = $interval(drawIfResized, 1000); // Watch "draw" for external changes to the set of // things to be drawn. diff --git a/platform/features/plot/test/MCTChartSpec.js b/platform/features/plot/test/MCTChartSpec.js index 837f8d0ef7..a8ddc24096 100644 --- a/platform/features/plot/test/MCTChartSpec.js +++ b/platform/features/plot/test/MCTChartSpec.js @@ -15,7 +15,7 @@ define( mockElement, mockCanvas, mockGL, - mockCancelInterval, + mockPromise, mctChart; beforeEach(function () { @@ -27,7 +27,8 @@ define( jasmine.createSpyObj("$scope", ["$watchCollection", "$on"]); mockElement = jasmine.createSpyObj("element", ["find"]); - mockCancelInterval = jasmine.createSpy("cancelInterval"); + mockInterval.cancel = jasmine.createSpy("cancelInterval"); + mockPromise = jasmine.createSpyObj("promise", ["then"]); // mct-chart uses GLChart, so it needs WebGL API @@ -72,7 +73,7 @@ define( mockElement.find.andReturn([mockCanvas]); mockCanvas.getContext.andReturn(mockGL); - mockInterval.andReturn(mockCancelInterval); + mockInterval.andReturn(mockPromise); mctChart = new MCTChart(mockInterval, mockLog); }); @@ -164,13 +165,13 @@ define( ); // Precondition - interval still active - expect(mockCancelInterval).not.toHaveBeenCalled(); + expect(mockInterval.cancel).not.toHaveBeenCalled(); // Broadcast a $destroy mockScope.$on.mostRecentCall.args[1](); // Should have stopped the interval - expect(mockCancelInterval).toHaveBeenCalled(); + expect(mockInterval.cancel).toHaveBeenCalledWith(mockPromise); }); });