Performance fixes for v2.0 and fix regression with out of order columns #691

pradeepnschrodinger · 2023-07-13T19:13:23Z

Description

This refactors some of the code in our redux logic to improve performance for v2.
The main changes are:

state.columnProps and state.columnGroupProps has been renamed to columnElements and columnGroupElements respectively.
Some usages of _.pick, _.forEach, _.map, _.reduce, etc have been hand picked and replaced with faster calls (mostly native) for performance.
columnWidths selector and convertColumnsToElements helper have been rewritten (keeping their structure same) to make them more efficient.
Immer (used by redux toolkit) deeply freezes our redux store state. This is pretty inefficient for large data structures (eg: elementTemplates). Luckily, we can workaround this by manually pre-freezing them shallowly.

The refactor This also fixes a regression where FDT renders the wrong renderers (cell templates) when columns are specified in a different order than what gets rendered.
eg:

<Column
  columnKey="A"
  fixed={true}
/>
<Column
  columnKey="B"
  fixedRight={true}
/>
<Column
  columnKey="C"
/>

causes FDT to mistakenly render column C using the renderer from column B (and vice-versa).

How Has This Been Tested?

Tested on local examples.
Particularly the "Fixed Right" example which is rendered by passing columns out of order.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

pradeepnschrodinger · 2023-07-16T05:30:37Z

src/FixedDataTableContainer.js

+      'columnGroupElements',
+      'columnElements',


state.columnProps has been replaced with state.columnElements.

One reason being that there's a modified version of columnProps returned by selectors, leading to confusion between state.columnProps and someSelector().columnProps.

pradeepnschrodinger · 2023-07-16T05:31:12Z

src/helper/convertColumnElementsToData.js

+// NOTE (pradeep): This can be simplified via _.pick()
+// However, _.pick is much slower than the hand written version here.


https://www.measurethat.net/Benchmarks/Show/6813/0/lodash-pick-vs-native-destructuring-vs-manual

pradeepnschrodinger · 2023-07-16T05:34:49Z

src/helper/convertColumnElementsToData.js

+    for (const columnGroupReactElement of children) {
+      const cellGroupType = getCellGroupType(columnGroupReactElement);
+      const columnGroupProps = _extractProps(columnGroupReactElement);
+      columnGroupProps.index = columnGroupIndex;


Also calculate and store the index while extracting the column/column-group.

pradeepnschrodinger · 2023-07-16T05:35:33Z

src/helper/convertColumnElementsToData.js

+    columnGroupElements,
+    columnElements,


columnGroupElements groups column props by cellGroupType, where as columnProps is a flat ordered list of column prop objects.

pradeepnschrodinger · 2023-07-16T05:35:54Z

src/helper/widthHelper.js

@@ -12,16 +12,55 @@

 'use strict';

-import reduce from 'lodash/reduce';
+import { createSelector } from 'reselect';


createSelector is used here to memoize the selectors.

pradeepnschrodinger · 2023-07-16T05:37:24Z

src/helper/widthHelper.js

-}
+const cacheOptions = {
+  memoizeOptions: {
+    maxSize: 12, // 3 cell group types * 4 template types


This ensures we can call the selectors for each cell group type for each template type without having to recompute the widths.

pradeepnschrodinger · 2023-07-16T05:39:22Z

src/selectors/columnWidths.js

@@ -164,55 +175,18 @@ function flexWidths(columnGroupProps, columnProps, viewportWidth) {
 *   scrollableColumns: !Array.<columnDefinition>
 * }}
 */
-function groupElements(elements) {


Besides grouping, groupElements also assigned index and offset.
The extra work is no longer needed.

pradeepnschrodinger · 2023-07-16T05:41:04Z

src/selectors/columnWidths.js

-      // if no flexGrow is specified, column defaults to original width
-      if (!flexGrow) {
-        return column;
+function flexWidths(columnGroupElements, columnElements, viewportWidth) {


The main change here is that the flexWidths also calculates the offsets of the column based on the new flex width.

This was previously done by groupElements() below.

pradeepnschrodinger · 2023-07-17T05:24:58Z

test/helper/convertColumnElementsToData-test.js

@@ -6,6 +6,26 @@ import forEach from 'lodash/forEach';

 import convertColumnElementsToData from '../../src/helper/convertColumnElementsToData';

+function column(overrides) {


helper to create a column element

pradeepnschrodinger · 2023-07-17T05:26:30Z

yarn.lock

@@ -7818,7 +7818,12 @@ requires-port@^1.0.0:
  resolved "https://registry.yarnpkg.com/requires-port/-/requires-port-1.0.0.tgz#925d2601d39ac485e091cf0da5c6e694dc3dcaff"
  integrity sha1-kl0mAdOaxIXgkc8NpcbmlNw9yv8=

-reselect@^4.0.0, reselect@^4.1.5:
+reselect@^4.0.0:
+  version "4.1.8"


Bumped to latest version which supports cache customization for the selectors.

karry08 · 2023-07-28T10:57:47Z

src/reducers/index.js

@@ -152,7 +152,7 @@ const slice = createSlice({
    },
    propChange(state, action) {
      const { newProps, oldProps } = action.payload;
-      const oldState = clone(state);
+      const oldState = clone(original(state));


what does original do

state is proxy object from immer that points to the actual redux state.
Iterating over this proxy object is slightly slower than iterating over the original state.

original(proxyObject) returns back the original object pointed to by the proxy.
(See original docs)

karry08 · 2023-07-28T10:59:06Z

src/reducers/index.js

+  Object.freeze(state.columnElements);
+  Object.freeze(state.columnGroupElements);
+  Object.freeze(state.elementTemplates);
+


karry08 · 2023-07-31T18:18:04Z

src/helper/convertColumnElementsToData.js

-  };
+  const columnElements = getElementsContainer();
+  const columnGroupElements = getElementsContainer();
+  const elementTemplates = getElementTemplates();


just a thought shouldnt the function name be getEmptyElementContainer/ getDefaultElementContainer

or what you have done is fine?

hmm both seem fine to me, but I'll rename it to getEmptyElementsContainer to be explicit.

karry08 · 2023-07-31T18:35:46Z

LGTM

AmanGupta2708 · 2023-07-31T11:04:22Z

src/helper/convertColumnElementsToData.js

+  const columnGroupElements = getElementsContainer();
+  const elementTemplates = getElementTemplates();
+  const useGroupHeader = !!(
+    children.length && children[0].type.__TableColumnGroup__


This can also be simplified by optional chaining.
const useGroupHeader = children[0]?.type.__TableColumnGroup__ ?? false

AmanGupta2708 · 2023-08-01T06:50:21Z

src/reducers/index.js

+  //     pre-freeze data using the freeze utility.
+  Object.freeze(state.columnElements);
+  Object.freeze(state.columnGroupElements);
+  Object.freeze(state.elementTemplates);


Can convertColumnElementsToData itself return these objects as frozen???

We could, but I like to keep convertColumnElementsToData as a pure helper method that shouldn't know about immer/parent reducer details.

AmanGupta2708 · 2023-08-01T06:56:00Z

will it merge to column-virtualization as well ?

pradeepnschrodinger · 2023-08-03T09:19:41Z

will it merge to column-virtualization as well ?

@AmanGupta2708 , yep this should be upmerged manually.
I don't think it'll be an easy merge though.
There are also lots of other commits in v2 that needs upmerging.
It makes sense to do them all, and perform another round of performance benchmarks.

Allow columns from different cell groups to be specified out of order

2f51acc

pradeepnschrodinger added bug regression issue that was "fixed" but got reopened later on labels Jul 13, 2023

pradeepnschrodinger added this to the v2.0 milestone Jul 13, 2023

pradeepnschrodinger requested review from abhisheksingh2410, karry08, AmanGupta2708 and somavara July 13, 2023 19:13

pradeepnschrodinger assigned karry08 and AmanGupta2708 Jul 13, 2023

Fix peformance issues

72aa01c

pradeepnschrodinger changed the title ~~Allow columns from different cell groups to be specified out of order~~ Performance fixes for v2.0 and fix regression with out of order columns Jul 16, 2023

pradeepnschrodinger added performance v2.0-beta Tickets targeting the v2.0-beta branch. labels Jul 16, 2023

pradeepnschrodinger commented Jul 16, 2023

View reviewed changes

Fix tests

8375b51

pradeepnschrodinger commented Jul 17, 2023

View reviewed changes

karry08 reviewed Jul 28, 2023

View reviewed changes

karry08 reviewed Jul 31, 2023

View reviewed changes

AmanGupta2708 reviewed Aug 1, 2023

View reviewed changes

pradeepnschrodinger added 3 commits August 3, 2023 14:28

Fix out of order columns

055b9ae

Use null coalescing

4190640

Rename vars

2637d50

pradeepnschrodinger merged commit 8229e50 into v2.0-beta Aug 5, 2023
6 checks passed

pradeepnschrodinger deleted the v20-beta-fix-outoforder-columns branch August 5, 2023 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance fixes for v2.0 and fix regression with out of order columns #691

Performance fixes for v2.0 and fix regression with out of order columns #691

pradeepnschrodinger commented Jul 13, 2023 •

edited

Loading

pradeepnschrodinger Jul 16, 2023

pradeepnschrodinger Jul 16, 2023

pradeepnschrodinger Jul 16, 2023

pradeepnschrodinger Jul 16, 2023

pradeepnschrodinger Jul 16, 2023

pradeepnschrodinger Jul 16, 2023

pradeepnschrodinger Jul 16, 2023

pradeepnschrodinger Jul 16, 2023

pradeepnschrodinger Jul 17, 2023

pradeepnschrodinger Jul 17, 2023

karry08 Jul 28, 2023

pradeepnschrodinger Jul 29, 2023

karry08 Jul 28, 2023

karry08 Jul 31, 2023

pradeepnschrodinger Aug 3, 2023

karry08 commented Jul 31, 2023

AmanGupta2708 Jul 31, 2023

pradeepnschrodinger Aug 3, 2023

AmanGupta2708 Aug 1, 2023

pradeepnschrodinger Aug 3, 2023

AmanGupta2708 commented Aug 1, 2023

pradeepnschrodinger commented Aug 3, 2023

		// NOTE (pradeep): This can be simplified via _.pick()
		// However, _.pick is much slower than the hand written version here.

		@@ -6,6 +6,26 @@ import forEach from 'lodash/forEach';

		import convertColumnElementsToData from '../../src/helper/convertColumnElementsToData';

		function column(overrides) {

Performance fixes for v2.0 and fix regression with out of order columns #691

Performance fixes for v2.0 and fix regression with out of order columns #691

Conversation

pradeepnschrodinger commented Jul 13, 2023 • edited Loading

Description

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karry08 commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmanGupta2708 commented Aug 1, 2023

pradeepnschrodinger commented Aug 3, 2023

pradeepnschrodinger commented Jul 13, 2023 •

edited

Loading