[Table] Eliminate unnecessary re-renders everywhere #998

cmslewis · 2017-04-17T19:43:00Z

Checklist

Redux of #988 that fixes some of the nefarious bugs that popped up.

Include tests

Changes proposed in this pull request:

Goal: Improve table perf by eliminating unnecessary re-renders, particularly when clicking+dragging.

Add @PureRender decorator throughout packages/table.
Add custom shouldComponentUpdate to GuideLayer to avoid unnecessary re-renders.
Add custom shouldComponentUpdate to RegionLayer to avoid unnecessary re-renders.
- 🔥 Also, change RegionLayer props to accept pre-computed regionStyles instead of getRegionStyle function. Before, we were creating this styling function inline in table.tsx on each render, causing shallow comparison between this.props.getRegionStyle === nextProps.regionStyle to always return false in RegionLayer.shouldComponentUpdate. Ultimately, this led to tons of unnecessary re-renders of the region layer (e.g. as you were clicking + dragging). LMK if you want to know more.
Extend Utils.shallowCompareKeys (+ tests) to compare entire objects, not just subset of keys.
Add Utils.arraysEqual (+ tests) to compare arrays using an optional compare callback.

Reviewers should focus on:

Does perf improve noticeably enough?
Any weird bugs lingering with selected regions or guides? shouldComponentUpdate is tricky to get right.

…owCompareKeys

…ecessary re-renders!

blueprint-bot · 2017-04-17T19:59:19Z

Make shallowCompareKeys return false for two arrays

Preview: documentation | table
Coverage: core | datetime

llorca · 2017-04-17T20:06:53Z

The table with reordering feature feels super snappy, though it's only 5 rows.

We should enable reordering and other fancy features on the full page spreadsheet table, cause this is where the real stress test is.

giladgray · 2017-04-17T20:05:36Z

packages/table/src/common/utils.ts

-            if (objA[key] !== objB[key]) {
+    shallowCompareKeys(objA: any, objB: any, keys?: string[]) {
+        if (keys != null) {
+            return shallowCompareKeys(objA, objB, keys);


how is this not an immediate infinite loop every time?

can we please rename the internal function? might be legit to use _ prefix here.

giladgray · 2017-04-17T20:06:45Z

packages/table/src/common/utils.ts

+     * will be compared using the custom `compare` function if one is provided.
+     */
+    arraysEqual(arrA: any[], arrB: any[], compare?: (a: any, b: any) => boolean) {
+        if (bothUndefined(arrA, arrB) || bothNull(arrA, arrB)) {


i think arraysEqual(null, undefined) should be true.

giladgray · 2017-04-17T20:07:32Z

packages/table/src/common/utils.ts

+     * Returns true if the arrays are equal. Elements will be shallowly compared by default, or they
+     * will be compared using the custom `compare` function if one is provided.
+     */
+    arraysEqual(arrA: any[], arrB: any[], compare?: (a: any, b: any) => boolean) {


how about compare = (a, b) => a === b as a default arg here? then line 357 gets real simple

giladgray · 2017-04-17T20:08:07Z

packages/table/src/common/utils.ts

+    }
+    return keys
+        .map((key) => objA.hasOwnProperty(key) === objB.hasOwnProperty(key) && objA[key] === objB[key])
+        .every((isEqual) => isEqual);


keys.every((key) => objA.hasOwnProperty(key) === objB.hasOwnProperty(key) && objA[key] === objB[key])

please

Whoops, fixed this elsewhere but missed this one

giladgray · 2017-04-17T20:09:18Z

packages/table/src/layers/guides.tsx

+        // array contents explicitly.
+        const verticalGuidesEqual = Utils.arraysEqual(this.props.verticalGuides, nextProps.verticalGuides);
+        const horizontalGuidesEqual = Utils.arraysEqual(this.props.horizontalGuides, nextProps.horizontalGuides);
+        return !verticalGuidesEqual || !horizontalGuidesEqual;


may be a minor optimization here to inline both vars in the ||. then if the first one is true it won't compute the second one (i think that's how js works).

return !Utils.arraysEqual(this.props.verticalGuides, nextProps.verticalGuides) || !Utils.arraysEqual(this.props.horizontalGuides, nextProps.horizontalGuides);

Noice. That's my understanding too - did this same thing in RegionLayer.

yeah i saw! sounds good

giladgray · 2017-04-17T20:10:45Z

packages/table/src/layers/regions.tsx

@@ -21,13 +21,26 @@ export interface IRegionLayerProps extends IProps {
    regions?: IRegion[];

    /**
-     * A callback interface for applying CSS styles to the regions.
+     * The array of CSS styles to apply to each region.


how do we know which styles belong to which region? just by index? worth noting in docs here i think.

Done. LMK if the wording needs to be massaged though.

giladgray · 2017-04-17T20:11:13Z

packages/table/src/layers/regions.tsx

+    public shouldComponentUpdate(nextProps: IRegionLayerProps) {
+        // shallowly comparable props like "className" tend not to change in the default table
+        // implementation, so do that check last with hope that we return earlier and avoid it
+        // altogether.


giladgray · 2017-04-17T20:12:34Z

packages/table/src/regions.ts

@@ -528,6 +528,11 @@ export class Regions {
        return regionGroups;
    }

+    public static regionsEqual(regionA: IRegion, regionB: IRegion) {


why move this? the previous location seems more logical to me: next to all the other comparison checks.

It's public now, so it has to move up.

ohhh public gotcha ok

giladgray · 2017-04-17T20:13:44Z

packages/table/src/table.tsx

+        const { viewportRect } = this.state;
+        if (viewportRect == null) {
+            return {};
+        }


this viewportRect check seems to happen in every one of these functions. maybe it can be moved up a level to the function that calls these style*Region() functions?

Played with this. Looks like viewportRect is not considered in styleBodyRegion, so we can't fully generalize. Will leave as is for now.

giladgray · 2017-04-17T20:17:01Z

packages/table/test/utilsTests.ts

+                runTest(false, [3], []);
+                runTest(false, [3, 1, 2], [3, 1]);
+                runTest(false, [{}], [{}]);
+                runTest(false, [{ x: 1 }], [{ x: 1 }]);


changes in this file generally feel like too much testing. most of these cases are equivalent: they're different instances of the same thing so of course they're not shallow equal. [] is the same as [3], from my perspective.

i would do this with like 1-3 cases in each describe but not worry about testing every possible permutation.

blueprint-bot · 2017-04-17T22:30:43Z

Oops, undo describe.only

Preview: documentation | table
Coverage: core | datetime

adidahiya

clicking and dragging feels faster, but scrolling feels about the same

adidahiya · 2017-04-17T23:12:50Z

packages/table/src/common/utils.ts

+/**
+ * Partial shallow comparison between objects using the given list of keys.
+ */
+function _shallowCompareKeys(objA: any, objB: any, keys: string[]) {


any way you can simplify this to all just one function?

shallowCompareKeys: function shallowCompareKeys(objA: any, objB: any, keys?: string[]) { ... // recurse shallowCompareKeys(...); }

Would literally be the second time I've actually had to use recursion in the wild, ha. Let me dust off my knowledge and give it a try.

recursion is a tree's best friend

Except for this kid. https://www.youtube.com/watch?v=1TZCP6OqRlE

@adidahiya ended up pulling the duplicate validation checks into one place and greatly simplifying the helper function. Didn't do the recursive approach though. This look okay now?

gscshoyru

Looks good to me, though agree with Adi that scrolling seems really really slow on the formats table. Wish we knew why...

cmslewis · 2017-04-18T17:42:09Z

@gscshoyru I definitely see the symptoms in the code. Will spend today getting to the bottom of it. (If you're in a masochistic mood, you should try clicking a column header in that formats table with why-did-you-update enabled.)

blueprint-bot · 2017-04-18T18:54:25Z

Reduce duplicate logic in shallowCompareKeys

Preview: documentation | table
Coverage: core | datetime

giladgray

great refactor

cmslewis added 11 commits April 14, 2017 11:24

Add Utils.shallowCompare plus unit tests for shallowCompare and shall…

771bdd1

…owCompareKeys

PureRender everywhere

287c001

Add shouldComponentUpdate to GuideLayer

07dd7a6

Modify shallowCompareKeys to optionally check all keys

7a46219

Add Utils.arraysEqual + tests

0d13d00

Refactor RegionLayer to receive pre-computed regionStyles. avoids unn…

244366b

…ecessary re-renders!

Minor cleanups throughout

af8d0b2

Eek, was testing arraysEqual improperly. Fixed a bug.

6dffba3

Fix bug with className comparison in GuideLayer.shouldComponentUpdate

10b410e

Use bothUndefined and bothNull helpers in arraysEqual

f26744f

Make shallowCompareKeys return false for two arrays

8c1ed0e

giladgray suggested changes Apr 17, 2017

View reviewed changes

giladgray reviewed Apr 17, 2017

View reviewed changes

cmslewis added 3 commits April 17, 2017 13:44

Respond to Gilad CR feedback

f3019c3

Delete unnecessary tests

f749f57

Oops, undo describe.only

9db6978

adidahiya reviewed Apr 17, 2017

View reviewed changes

gscshoyru approved these changes Apr 18, 2017

View reviewed changes

Reduce duplicate logic in shallowCompareKeys

3b7c105

giladgray approved these changes Apr 18, 2017

View reviewed changes

cmslewis merged commit 9844bd4 into master Apr 18, 2017

cmslewis deleted the cl/table-perf-v2 branch April 18, 2017 18:58

giladgray mentioned this pull request Apr 18, 2017

Prepare Release 1.15.0 #1005

Merged

adidahiya mentioned this pull request Apr 20, 2017

[Table] Truncated icons persist in cells even when text isn't truncated #1022

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Table] Eliminate unnecessary re-renders everywhere #998

[Table] Eliminate unnecessary re-renders everywhere #998

cmslewis commented Apr 17, 2017 •

edited

Loading

blueprint-bot commented Apr 17, 2017

llorca commented Apr 17, 2017

giladgray Apr 17, 2017

cmslewis Apr 17, 2017

giladgray Apr 17, 2017

cmslewis Apr 17, 2017

giladgray Apr 17, 2017

cmslewis Apr 17, 2017

giladgray Apr 17, 2017

cmslewis Apr 17, 2017

giladgray Apr 17, 2017

cmslewis Apr 17, 2017 •

edited

Loading

giladgray Apr 17, 2017

giladgray Apr 17, 2017

cmslewis Apr 17, 2017

giladgray Apr 17, 2017

giladgray Apr 17, 2017

cmslewis Apr 17, 2017

giladgray Apr 17, 2017

giladgray Apr 17, 2017

cmslewis Apr 17, 2017

giladgray Apr 17, 2017

cmslewis Apr 17, 2017

blueprint-bot commented Apr 17, 2017

adidahiya left a comment

adidahiya Apr 17, 2017

cmslewis Apr 18, 2017

giladgray Apr 18, 2017

cmslewis Apr 18, 2017

gscshoyru left a comment

cmslewis commented Apr 18, 2017

blueprint-bot commented Apr 18, 2017

giladgray left a comment

[Table] Eliminate unnecessary re-renders everywhere #998

[Table] Eliminate unnecessary re-renders everywhere #998

Conversation

cmslewis commented Apr 17, 2017 • edited Loading

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

blueprint-bot commented Apr 17, 2017

Make shallowCompareKeys return false for two arrays

llorca commented Apr 17, 2017

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

cmslewis Apr 17, 2017 • edited Loading

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

blueprint-bot commented Apr 17, 2017

Oops, undo describe.only

adidahiya left a 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

gscshoyru left a comment

Choose a reason for hiding this comment

cmslewis commented Apr 18, 2017

blueprint-bot commented Apr 18, 2017

Reduce duplicate logic in shallowCompareKeys

giladgray left a comment

Choose a reason for hiding this comment

cmslewis commented Apr 17, 2017 •

edited

Loading

cmslewis Apr 17, 2017 •

edited

Loading