Changed style-cache key strategy

Also alpha-sorted, added some unit tests, fixed a small bug or two (eg reset style cache in recomputeGridSize)
bvaughn · Dec 14, 2016 · 25293e1 · 25293e1
1 parent 17d4a61
commit 25293e1
Show file tree

Hide file tree

Showing 7 changed files with 210 additions and 28 deletions.
diff --git a/docs/Grid.md b/docs/Grid.md
@@ -97,9 +97,7 @@ This function accepts the following named parameters:
 
 ```js
 function cellRangeRenderer ({
-  cellCache,                    // Temporary cell cache for use while scrolling
-  styleCache,                   // Cache of style objects sent to cells, to
-                                // ensure referential equality
+  cellCache,                    // Temporary cell cache used while scrolling
   cellRenderer,                 // Cell renderer prop supplied to Grid
   columnSizeAndPositionManager, // @see CellSizeAndPositionManager,
   columnStartIndex,             // Index of first column (inclusive) to render
@@ -111,6 +109,7 @@ function cellRangeRenderer ({
   rowStopIndex,                 // Index of last column (inclusive) to render
   scrollLeft,                   // Current horizontal scroll offset of Grid
   scrollTop,                    // Current vertical scroll offset of Grid
+  styleCache,                   // Temporary style (size & position) cache used while scrolling
   verticalOffsetAdjustment      // Vertical pixel offset (required for scaling)
 }) {
   const renderedCells = []

diff --git a/package.json b/package.json
@@ -120,7 +120,7 @@
     "karma-phantomjs-launcher": "^1.0.0",
     "karma-sourcemap-loader": "^0.3.6",
     "karma-webpack": "^1.7.0",
-    "phantomjs-prebuilt": "^2.1.7",
+    "phantomjs-prebuilt": "^2.1.14",
     "postcss": "^5.0.14",
     "postcss-cli": "^2.3.3",
     "postcss-loader": "^0.9.1",

diff --git a/playground/render-counters.html b/playground/render-counters.html
@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>foo</title>
+  <style type="text/css">
+    body, html, #mount {
+      width: 100%;
+      height: 100%;
+      margin: 0;
+      padding: 0;
+      font-family: -apple-system, BlinkMacSystemFont,  "Segoe UI", "Roboto", "Oxygen", "Ubuntu", "Cantarell",  "Fira Sans", "Droid Sans", "Helvetica Neue",  sans-serif;
+      font-size: 12px;
+    }
+    * {
+      box-sizing: border-box;
+    }
+    .item {
+      height: 100%;
+      display: flex;
+      justify-content: center;
+      align-items: center;
+      border: 1px solid #eee;
+    }
+    .item:hover {
+      background-color: rgba(0, 0, 0, .2);
+    }
+    .hoveredItem {
+      background-color: rgba(0, 0, 0, .1);
+    }
+  </style>
+</head>
+<body>
+  <div id="mount"></div>
+
+  <script src="utils.js"></script>
+  <script src="helper.js"></script>
+  <script>
+    loadReact();
+    loadScriptsAndStyles('render-counters.js');
+  </script>
+</body>
+</html>
diff --git a/playground/render-counters.js b/playground/render-counters.js
@@ -0,0 +1,76 @@
+const { Component } = React
+const { shallowCompare } = React.addons
+const { AutoSizer, List } = ReactVirtualized
+
+class ListExample extends Component {
+  render() {
+    return React.createElement(
+      AutoSizer,
+      null,
+      ({ height, width }) => React.createElement(List, {
+        height: height,
+        overscanRowCount: 0,
+        rowCount: 1000,
+        rowHeight: 30,
+        rowRenderer: this._rowRenderer,
+        width: width
+      })
+    );
+  }
+
+  shouldComponentUpdate(nextProps, nextState) {
+    return shallowCompare(this, nextProps, nextState);
+  }
+
+  _rowRenderer({ index, isScrolling, key, style }) {
+    return React.createElement(Row, {
+      index: index,
+      key: key,
+      style: style
+    });
+  }
+}
+
+class Row extends Component {
+  constructor(props, context) {
+    super(props, context);
+
+    this.state = {
+      counter: 0
+    };
+
+    this._renderCount = 0;
+  }
+
+  render() {
+    this._renderCount++;
+
+    const { counter } = this.state;
+    const { index, style } = this.props;
+
+    return React.createElement(
+      'div',
+      {
+        onClick: () => {
+          this.setState(state => {
+            counter: state.counter++;
+          });
+        },
+        style: style
+      },
+      'Row ',
+      index,
+      ', ',
+      this._renderCount
+    );
+  }
+
+  shouldComponentUpdate(nextProps, nextState) {
+    return shallowCompare(this, nextProps, nextState);
+  }
+}
+
+ReactDOM.render(
+  React.createElement(ListExample),
+  document.querySelector('#mount')
+)
diff --git a/source/Grid/Grid.js b/source/Grid/Grid.js
@@ -208,7 +208,6 @@ export default class Grid extends Component {
   constructor (props, context) {
     super(props, context)
 
-    this._styleCache = {}
     this.state = {
       isScrolling: false,
       scrollDirectionHorizontal: SCROLL_DIRECTION_FORWARD,
@@ -222,7 +221,7 @@ export default class Grid extends Component {
     this._onScrollMemoizer = createCallbackMemoizer(false)
 
     // Bind functions to instance so they don't lose context when passed around
-    this._enablePointerEventsAfterDelayCallback = this._enablePointerEventsAfterDelayCallback.bind(this)
+    this._debounceScrollEndedCallback = this._debounceScrollEndedCallback.bind(this)
     this._invokeOnGridRenderedHelper = this._invokeOnGridRenderedHelper.bind(this)
     this._onScroll = this._onScroll.bind(this)
     this._updateScrollLeftForScrollToColumn = this._updateScrollLeftForScrollToColumn.bind(this)
@@ -242,8 +241,9 @@ export default class Grid extends Component {
       estimatedCellSize: this._getEstimatedRowSize(props)
     })
 
-    // See defaultCellRangeRenderer() for more information on the usage of this cache
+    // See defaultCellRangeRenderer() for more information on the usage of these caches
     this._cellCache = {}
+    this._styleCache = {}
   }
 
   /**
@@ -273,6 +273,7 @@ export default class Grid extends Component {
     // Clear cell cache in case we are scrolling;
     // Invalid row heights likely mean invalid cached content as well.
     this._cellCache = {}
+    this._styleCache = {}
 
     this.forceUpdate()
   }
@@ -664,7 +665,6 @@ export default class Grid extends Component {
       this._childrenToDisplay = cellRangeRenderer({
         cellCache: this._cellCache,
         cellRenderer,
-        styleCache: this._styleCache,
         columnSizeAndPositionManager: this._columnSizeAndPositionManager,
         columnStartIndex: this._columnStartIndex,
         columnStopIndex: this._columnStopIndex,
@@ -675,6 +675,7 @@ export default class Grid extends Component {
         rowStopIndex: this._rowStopIndex,
         scrollLeft,
         scrollTop,
+        styleCache: this._styleCache,
         verticalOffsetAdjustment,
         visibleColumnIndices,
         visibleRowIndices
@@ -687,24 +688,38 @@ export default class Grid extends Component {
    * This flag is used to disable pointer events on the scrollable portion of the Grid.
    * This prevents jerky/stuttery mouse-wheel scrolling.
    */
-  _enablePointerEventsAfterDelay () {
+  _debounceScrollEnded () {
     const { scrollingResetTimeInterval } = this.props
 
     if (this._disablePointerEventsTimeoutId) {
       clearTimeout(this._disablePointerEventsTimeoutId)
     }
 
     this._disablePointerEventsTimeoutId = setTimeout(
-      this._enablePointerEventsAfterDelayCallback,
+      this._debounceScrollEndedCallback,
       scrollingResetTimeInterval
     )
   }
 
-  _enablePointerEventsAfterDelayCallback () {
+  _debounceScrollEndedCallback () {
     this._disablePointerEventsTimeoutId = null
 
-    // Throw away cell cache once scrolling is complete
+    const styleCache = this._styleCache
+
+    // Reset cell and style caches once scrolling stops.
+    // This makes Grid simpler to use (since cells commonly change).
+    // And it keeps the caches from growing too large.
+    // Performance is most sensitive when a user is scrolling.
     this._cellCache = {}
+    this._styleCache = {}
+
+    // Copy over the visible cell styles so avoid unnecessary re-render.
+    for (let rowIndex = this._rowStartIndex; rowIndex <= this._rowStopIndex; rowIndex++) {
+      for (let columnIndex = this._columnStartIndex; columnIndex <= this._columnStopIndex; columnIndex++) {
+        let key = `${rowIndex}-${columnIndex}`
+        this._styleCache[key] = styleCache[key]
+      }
+    }
 
     this.setState({
       isScrolling: false
@@ -852,7 +867,7 @@ export default class Grid extends Component {
     }
 
     // Prevent pointer events from interrupting a smooth scroll
-    this._enablePointerEventsAfterDelay()
+    this._debounceScrollEnded()
 
     // When this component is shrunk drastically, React dispatches a series of back-to-back scroll events,
     // Gradually converging on a scrollTop that is within the bounds of the new, smaller height.

diff --git a/source/Grid/Grid.test.js b/source/Grid/Grid.test.js
@@ -1423,5 +1423,53 @@ describe('Grid', () => {
 
       expect(componentUpdates).toEqual(0)
     })
+
+    it('should clear all but the visible rows from the style cache once :isScrolling is false', async (done) => {
+      const props = {
+        columnWidth: 50,
+        height: 100,
+        rowHeight: 50,
+        width: 100
+      }
+
+      const grid = render(getMarkup(props))
+
+      expect(Object.keys(grid._styleCache).length).toBe(4)
+
+      simulateScroll({ grid, scrollTop: 50 })
+
+      expect(Object.keys(grid._styleCache).length).toBe(6)
+
+      // Allow scrolling timeout to complete so that cell cache is reset
+      await new Promise(resolve => setTimeout(resolve, DEFAULT_SCROLLING_RESET_TIME_INTERVAL * 2))
+
+      expect(Object.keys(grid._styleCache).length).toBe(4)
+
+      done()
+    })
+
+    it('should clear style cache if :recomputeGridSize is called', () => {
+      const props = {
+        columnWidth: 50,
+        height: 100,
+        rowHeight: 50,
+        width: 100
+      }
+
+      const grid = render(getMarkup(props))
+
+      expect(Object.keys(grid._styleCache).length).toBe(4)
+
+      render(getMarkup({
+        ...props,
+        scrollTop: 50
+      }))
+
+      expect(Object.keys(grid._styleCache).length).toBe(6)
+
+      grid.recomputeGridSize()
+
+      expect(Object.keys(grid._styleCache).length).toBe(4)
+    })
   })
 })
diff --git a/source/Grid/defaultCellRangeRenderer.js b/source/Grid/defaultCellRangeRenderer.js
@@ -5,7 +5,6 @@
  */
 export default function defaultCellRangeRenderer ({
   cellCache,
-  styleCache,
   cellRenderer,
   columnSizeAndPositionManager,
   columnStartIndex,
@@ -17,6 +16,7 @@ export default function defaultCellRangeRenderer ({
   rowStopIndex,
   scrollLeft,
   scrollTop,
+  styleCache,
   verticalOffsetAdjustment,
   visibleColumnIndices,
   visibleRowIndices
@@ -35,22 +35,21 @@ export default function defaultCellRangeRenderer ({
         rowIndex <= visibleRowIndices.stop
       )
       let key = `${rowIndex}-${columnIndex}`
-
-      const height = rowDatum.size
-      const left = columnDatum.offset + horizontalOffsetAdjustment
-      const top = rowDatum.offset + verticalOffsetAdjustment
-      const width = columnDatum.size
-
-      const styleKey = `x${left}-y${top}-w${width}-h${height}`
       let style
 
-      // avoid creating new style objects at all times to maintain referential
-      // equality, so shallowCompare components don't rerun render() unnecessarily
-      if (styleCache[styleKey]) {
-        style = styleCache[styleKey]
+      // Cache style objects so shallow-compare doesn't re-render unnecessarily.
+      if (styleCache[key]) {
+        style = styleCache[key]
       } else {
-        style = {height, width, left, top, position: 'absolute'}
-        styleCache[styleKey] = style
+        style = {
+          height: rowDatum.size,
+          left: columnDatum.offset + horizontalOffsetAdjustment,
+          position: 'absolute',
+          top: rowDatum.offset + verticalOffsetAdjustment,
+          width: columnDatum.size
+        }
+
+        styleCache[key] = style
       }
 
       let cellRendererParams = {
@@ -79,7 +78,9 @@ export default function defaultCellRangeRenderer ({
         if (!cellCache[key]) {
           cellCache[key] = cellRenderer(cellRendererParams)
         }
+
         renderedCell = cellCache[key]
+
       // If the user is no longer scrolling, don't cache cells.
       // This makes dynamic cell content difficult for users and would also lead to a heavier memory footprint.
       } else {
@@ -99,7 +100,6 @@ export default function defaultCellRangeRenderer ({
 
 type DefaultCellRangeRendererParams = {
   cellCache: Object,
-  styleCache: Object,
   cellRenderer: Function,
   columnSizeAndPositionManager: Object,
   columnStartIndex: number,
@@ -111,6 +111,7 @@ type DefaultCellRangeRendererParams = {
   rowStopIndex: number,
   scrollLeft: number,
   scrollTop: number,
+  styleCache: Object,
   verticalOffsetAdjustment: number,
   visibleColumnIndices: Object,
   visibleRowIndices: Object