fix(5007): Reduced the amount of digest cycles initiated by the grid. #6072

mportuga · 2017-03-13T20:22:12Z

Re-implemented some of @csvan's work with PR #5829 and improved refreshCanvas function.
Also, update the All features tutorial to add a toggle button for the filter row and improved coverage in ui-grid.js

5007

@csvan

Re-implemented some of @csvan's work with PR angular-ui#5829 and improved refreshCanvas function. 5007

mportuga · 2017-03-13T20:25:01Z

misc/tutorial/401_AllFeatures.ngdoc

-        return row.id;
+       var gridApi;
+
+      $scope.gridOptions = {


I decided to combine the multiple calls to $scope.gridOptions.[something] into one object definition.

mportuga · 2017-03-13T20:25:28Z

misc/tutorial/401_AllFeatures.ngdoc


-      $scope.gridOptions.columnDefs = [


This moved to line 31

mportuga · 2017-03-13T20:26:00Z

misc/tutorial/401_AllFeatures.ngdoc

      };
-      $scope.gridOptions.getRowIdentity = function(row) {
+
+      function getRowId(row) {


This function was repeated for getRowIndentity and rowIdentity, so I combined them.

mportuga · 2017-03-13T20:26:50Z

misc/tutorial/401_AllFeatures.ngdoc

    }]);
  </file>
  <file name="index.html">
    <div ng-controller="MainCtrl">
+      <button id="filterToggle" type="button" class="btn btn-success" ng-click="toggleFilterRow()">Toggle Filter</button>


I find myself needing to test this a lot and I frequently see question about how to implement something like this, so I added it to this demo.

mportuga · 2017-03-13T20:28:22Z

src/js/core/directives/ui-grid.js

-        $attrs.$observe('uiGridColumns', function(value) {
-          self.grid.options.columnDefs = value;
+        deregFunctions.push( $attrs.$observe('uiGridColumns', function(value) {
+          self.grid.options.columnDefs = angular.isString(value) ? angular.fromJson(value) : value;


While writing unit tests, I realized there was a mistake with this watch. Since this is an attribute watch, the value of it might be a string that needs to be converted to an object.

mportuga · 2017-03-13T20:29:08Z

src/js/core/directives/ui-grid.js

      }

+      // prevents an error from being thrown when the array is not defined yet and fastWatch is on
+      function getSize(array) {


Again, while writing unit tests, I realized we don't handle the case for where data is undefined and fastWatch is on. This will prevent issues with that.

mportuga · 2017-03-13T20:29:47Z

src/js/core/directives/ui-grid.js

-            else {
-              $timeout(init);
+            } else {
+              $scope.$applyAsync(init);


This should prevent an unnecessary digest cycle by pig backing on the next available cycle.

mportuga · 2017-03-13T20:30:06Z

src/js/core/directives/ui-grid.js

            // Bind to window resize events
            angular.element($window).on('resize', gridResize);

            // Unbind from window resize events when the grid is destroyed
            $elm.on('$destroy', function () {
              angular.element($window).off('resize', gridResize);
+              deregisterLeftWatcher();


Cleaning up possible memory leaks

mportuga · 2017-03-13T20:32:23Z

src/js/core/factories/Grid.js

@@ -595,10 +595,10 @@ angular.module('ui.grid')
           callback.types.indexOf( type ) !== -1 ||
           type === uiGridConstants.dataChange.ALL ) {
        if (callback._this) {
-           callback.callback.apply(callback._this,this);
+           callback.callback.apply(callback._this, this, options);


Giving us a way to pass options into a registered callback. This allowed me to prevent buildColumns from being triggered multiple time during initialization. See line 74 of ui-grid.js

mportuga · 2017-03-13T20:34:05Z

src/js/core/directives/ui-grid.js

-
-              self.grid.callDataChangeCallbacks(uiGridConstants.dataChange.COLUMN);
-            }).catch(angular.noop);
+          self.grid.callDataChangeCallbacks(uiGridConstants.dataChange.COLUMN, {


Prevent buildColumns from being triggered multiple time during initialization. See lines 598 and 601 of Grid.js.

mportuga · 2017-03-13T20:35:12Z

src/js/core/directives/ui-grid.js

@@ -84,12 +84,10 @@
        // gridUtil.logDebug('dataWatch fired');
        var promises = [];

-        if ( self.grid.options.fastWatch ){


Removed the fastWatch condition for expecting data to be a string because based on line 62 of this file, strings for data are supported without fastWatch.

mportuga · 2017-03-13T20:36:39Z

src/js/core/directives/ui-grid.js

-
-              self.grid.preCompileCellTemplates();
-
-              self.grid.callDataChangeCallbacks(uiGridConstants.dataChange.COLUMN);


This line was causing buildColumns to be triggered a second time.

mportuga · 2017-03-13T20:37:31Z

src/js/core/factories/Grid.js

-  Grid.prototype.columnRefreshCallback = function columnRefreshCallback( grid ){
-    grid.buildColumns();
+  Grid.prototype.columnRefreshCallback = function columnRefreshCallback(grid, options){
+    grid.buildColumns(options);


This allows the columnRefreshCallback to pre-compile the cell templates when needed.

mportuga · 2017-03-13T20:39:14Z

src/features/selection/js/selection.js

@@ -664,7 +664,7 @@
                  allowCellFocus: true
                };

-                uiGridCtrl.grid.addRowHeaderColumn(selectionRowHeaderDef, 0);
+                uiGridCtrl.grid.addRowHeaderColumn(selectionRowHeaderDef, 0, true);


I added a new attribute to prevent build columns from being triggered multiple times in a row. ColumnDefs is now responsible for building the columns. See line 763 of Grid.js

mportuga · 2017-03-13T20:40:02Z

src/features/tree-base/js/tree-base.js

@@ -715,7 +715,7 @@
        };

        rowHeaderColumnDef.visible = grid.options.treeRowHeaderAlwaysVisible;
-        grid.addRowHeaderColumn( rowHeaderColumnDef, -100 );
+        grid.addRowHeaderColumn(rowHeaderColumnDef, -100, true);


I added a new attribute to prevent build columns from being triggered multiple times in a row. ColumnDefs is now responsible for building the columns. See line 763 of Grid.js

derekm · 2017-03-13T21:56:28Z

I was just noticing the massive amount of digest cycles that ui-grid forces, especially in my case when I'm swapping large datasets in and out while using ui-grid-auto-resize to make all rows visible.

csvan · 2017-03-14T06:36:38Z

@mportuga great work!

dlgski · 2017-03-14T13:29:34Z

misc/tutorial/401_AllFeatures.ngdoc

-        return row.id;
+       var gridApi;
+
+      $scope.gridOptions = {


dlgski · 2017-03-14T13:31:50Z

misc/tutorial/401_AllFeatures.ngdoc

+      $scope.toggleFilterRow = function() {
+        $scope.gridOptions.enableFiltering = !$scope.gridOptions.enableFiltering;
+        gridApi.core.notifyDataChange(uiGridConstants.dataChange.COLUMN);
+      };


where is this used? Is it a new button designed into the grid? Why do you need to notifyDataChange?

Yes, it is a new button. And the grid will not update unless the event is fired.

fix(5007): Reduced the amount of digest cycles initiated by the grid.

75f98c3

Re-implemented some of @csvan's work with PR angular-ui#5829 and improved refreshCanvas function. 5007

mportuga requested review from dlgski and jeremytowne March 13, 2017 20:22

mportuga commented Mar 13, 2017

View reviewed changes

misc/tutorial/401_AllFeatures.ngdoc

$scope.gridOptions.columnDefs = [

Copy link

Member Author

mportuga Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This moved to line 31

mportuga commented Mar 13, 2017

View reviewed changes

derekm mentioned this pull request Mar 13, 2017

dynamically calculate height of the grid not working #3622

Closed

dlgski approved these changes Mar 14, 2017

View reviewed changes

dlgski added the Looks Good To Me label Mar 14, 2017

jeremytowne approved these changes Mar 14, 2017

View reviewed changes

mportuga merged commit bd51855 into angular-ui:master Mar 15, 2017

mportuga deleted the fix/performance branch March 15, 2017 01:44


		self.grid.preCompileCellTemplates();

		self.grid.callDataChangeCallbacks(uiGridConstants.dataChange.COLUMN);

fix(5007): Reduced the amount of digest cycles initiated by the grid. #6072

fix(5007): Reduced the amount of digest cycles initiated by the grid. #6072

Uh oh!

Conversation

mportuga commented Mar 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekm commented Mar 13, 2017

Uh oh!

csvan commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!