[Table] Batch Child Rendering #1201

themadcreator · 2017-06-07T21:50:46Z

Allows react components to add or remove a limited number of children at a time to reduce to DOM reconciliation time.

This removes jank while scrolling, but introduces a "scanline" render style for table cells. This is preferable to slow, janky frame rates.

Includes unit test for Batcher.

Batch renders are invoked using a polyfill for requestIdleCallback that has these semantics:

Invokes the provided callback on the next available frame after the stack frame is empty.

At most one callback per frame is invoked, and the callback may be delayed multiple frames until the page is idle.

Add or remove a limited number of children at a time to reduce to DOM reconcilation time. This removes jank from scrolling, but introduces a "scanline" render style for table cells. This is preferable to slow, janky framerates.

giladgray · 2017-06-07T23:23:54Z

packages/table/src/common/batcher.ts

+
+export interface IIndex<T> {
+    [key: string]: T;
+}


Dict<string, T> 😱

giladgray · 2017-06-07T23:24:15Z

packages/table/src/common/batcher.ts

+    [key: string]: T;
+}
+
+export type SimpleStringifyable = string | number | null;


undefined too?

giladgray · 2017-06-07T23:24:39Z

packages/table/src/common/batcher.ts

+ *
+ * A typical usage would be:
+ *
+ * ```typescript


"```tsx", and remove initial blank line

giladgray · 2017-06-07T23:25:32Z

packages/table/src/common/batcher.ts

+ *     batcher.startNewBatch();
+ *
+ *     allChildrenKeys.forEach((prop1, index) => {
+ *         batcher.addArgsToBatch(prop1, "prop2", index);


prop1, "prop2" 1, 2 intentional?

giladgray · 2017-06-07T23:27:09Z

packages/table/src/common/batcher.ts

+     * The arguments must be simple stringifyable objects.
+     */
+    public addArgsToBatch(...args: SimpleStringifyable[]) {
+        this.batchArgs[args.join("-")] = args;


if you must join-hash, at least use a less common character, such as | or 😱

giladgray · 2017-06-07T23:28:06Z

packages/table/src/common/batcher.ts

+     * part of the "batch" set but not the "current" set are candidates for
+     * addition.
+     *
+     * The number of objects added and removed maybe limited with the `..Limit`


may be limited

ok comments in this file need some general editing, i'm not going to call out each typo here 📖

giladgray · 2017-06-07T23:29:34Z

packages/table/src/headers/header.tsx

+    "startIndex",
+    "style",
+    "viewportRect",
+] as Array<keyof IInternalHeaderProps>;


declare type up front instead of casting. const X: Array<...> = [...]

we should expand https://palantir.github.io/tslint/rules/no-object-literal-type-assertion/ to cover this kind of thing

giladgray · 2017-06-07T23:30:08Z

packages/table/src/headers/header.tsx

 @PureRender
 export class Header extends React.Component<IInternalHeaderProps, IHeaderState> {
    public state: IHeaderState = {
        hasSelectionEnded: false,
    };

    protected activationIndex: number;
+    private batcher = new Batcher<React.ReactElement<any>>();


use JSX.Element instead--it's equivalent and shorter

giladgray · 2017-06-07T23:30:13Z

packages/table/src/headers/header.tsx

 @PureRender
 export class Header extends React.Component<IInternalHeaderProps, IHeaderState> {
    public state: IHeaderState = {
        hasSelectionEnded: false,
    };

    protected activationIndex: number;
+    private batcher = new Batcher<React.ReactElement<any>>();


use JSX.Element instead--it's equivalent and shorter

giladgray · 2017-06-07T23:30:15Z

packages/table/src/headers/header.tsx

-    public componentWillReceiveProps(nextProps?: IHeaderProps) {
+    public componentWillReceiveProps(nextProps?: IInternalHeaderProps) {
+        const resetKeysBlacklist = { exclude: RESET_CELL_KEYS_BLACKLIST };
+        const resetBatcher = !Utils.shallowCompareKeys(this.props, nextProps, resetKeysBlacklist);


shouldResetBatcher?

…le-perf

blueprint-bot · 2017-06-09T20:35:27Z

fix isotest

Preview: documentation | table
Coverage: core | datetime

themadcreator · 2017-06-09T20:44:08Z

Performance comparison:

Prior to these changes:
https://2997-71939872-gh.circle-artifacts.com/0/home/ubuntu/blueprint/packages/table/preview/index.html

With these changes:
https://3003-71939872-gh.circle-artifacts.com/0/home/ubuntu/blueprint/packages/table/preview/index.html

cmslewis · 2017-06-09T20:39:56Z

packages/table/src/common/batcher.ts

+/**
+ * This class helps batch updates to large lists.
+ *
+ * For example, if your react component has many children, updating them all at


ubernit: React

cmslewis · 2017-06-09T20:45:47Z

packages/table/src/common/batcher.ts

+     * The arguments must be simple stringifyable objects.
+     */
+    public addArgsToBatch(...args: SimpleStringifyable[]) {
+        this.batchArgs[args.join("|")] = args;


nit: going off of @giladgray's earlier comment, would be clearer for me as a newcomer to this code to see this character pulled out as a DELIMITER or RARE_DELIMITER constant.

cmslewis · 2017-06-09T20:52:17Z

packages/table/src/common/batcher.ts

+            addNewLimit = Batcher.DEFAULT_ADD_LIMIT,
+            removeOldLimit = Batcher.DEFAULT_REMOVE_LIMIT,
+            updateLimit = Batcher.DEFAULT_UPDATE_LIMIT,
+    ) {


Cool formatting. 👍

cmslewis · 2017-06-09T20:52:27Z

packages/table/src/common/batcher.ts

+        const toRemove = this.setKeysOperation(this.currentObjects, this.batchArgs, "difference", removeOldLimit);
+        toRemove.forEach((key) => delete this.currentObjects[key]);
+
+        // remove ALL old object not in batch


nit: objects

cmslewis · 2017-06-09T20:57:29Z

packages/table/src/common/batcher.ts

+        toRemove.forEach((key) => delete this.currentObjects[key]);
+
+        // remove ALL old object not in batch
+        const toRemoveOld = this.setKeysOperation(this.oldObjects, this.batchArgs, "difference", -1);


nit: a null limit seems more intuitive to me than a -1 limit.

i disagree. I think it's more natural (or at least more common) to use a number than a null value

cmslewis · 2017-06-09T21:17:22Z

packages/table/src/common/requestIdleCallback.ts

+     */
+    requestAnimationFrame(() => {
+        requestAnimationFrame(() => {
+            postMessage(MESSAGE_EVENT_DATA, "*");


Putting the BILL in BrILLiant. 👏 👏 👏

cmslewis · 2017-06-09T21:18:03Z

packages/table/src/common/requestIdleCallback.ts

+ * At most one callback per frame is invoked, and the callback may be delayed
+ * multiple frames until the page is idle.
+ *
+ * TODO: return a token from this method that allows you to cancel the callback


Can we track this TODO in a Github issue if it's not going to get done immediately?

cmslewis · 2017-06-09T21:18:50Z

packages/table/src/headers/header.tsx

+        let shouldResetBatcher = !Utils.shallowCompareKeys(this.props, nextProps, resetKeysBlacklist);
+        // const stateDiff = Utils.shallowCompareKeys(this.state, nextState);
+        // //tslint:disable-next-line
+        // console.log(stateDiff, shouldResetBatcher, this.state, nextState);


remove commented code

cmslewis · 2017-06-09T21:20:48Z

packages/table/src/tableBody.tsx

+            return this.renderCell(row, col, extremaClasses, isGhost);
+        });
+        if (!this.batcher.isDone()) {
+            this.batcher.idleCallback(() => this.forceUpdate());


To avoid instantiating a new anonymous function on each render, can we just do this.batcher.idleCallback(this.forceUpdate)?

the forceUpdate method not bound to this and this.forceUpdate.bind(this) is equivalent.

cmslewis · 2017-06-09T21:21:15Z

packages/table/src/tableBody.tsx

@@ -146,6 +180,7 @@ export class TableBody extends React.Component<ITableBodyProps, {}> {
                </ContextMenuTargetWrapper>
            </DragSelectable>
        );
+


nit: don't need this newline

blueprint-bot · 2017-06-09T21:55:23Z

lint

Preview: documentation | table
Coverage: core | datetime

adidahiya · 2017-06-10T14:35:55Z

strangely, the big table stops rendering at 838,861 rows. this seems to be a problem on master and #1209 too though, so outside the scope of this PR.

themadcreator · 2017-06-12T17:57:11Z

Well:
https://stackoverflow.com/questions/38905916/content-in-google-chrome-larger-than-16777216-px-not-being-rendered
https://bugs.chromium.org/p/chromium/issues/detail?id=401762

themadcreator · 2017-06-12T18:02:57Z

Furthermore:

blueprint-bot · 2017-06-12T18:10:29Z

Merge remote-tracking branch 'origin/master' into bd/table-perf

Preview: documentation | table
Coverage: core | datetime

blueprint-bot · 2017-06-12T18:20:16Z

Merge remote-tracking branch 'origin/master' into bd/table-perf

Preview: documentation | table
Coverage: core | datetime

blueprint-bot · 2017-06-12T19:54:08Z

PR Comments

Preview: documentation | table
Coverage: core | datetime

cmslewis

Looks and feels good to me. Approving! 😬

themadcreator added 4 commits June 7, 2017 14:28

Batch child rendering

2ea0ecf

Add or remove a limited number of children at a time to reduce to DOM reconcilation time. This removes jank from scrolling, but introduces a "scanline" render style for table cells. This is preferable to slow, janky framerates.

Merge remote-tracking branch 'origin/master' into bd/table-perf

cd67e87

Use blacklist to reset batcher

5352057

lint

12d8675

giladgray reviewed Jun 7, 2017

View reviewed changes

adidahiya changed the title ~~Batch Child Rendering~~ [Table] Batch Child Rendering Jun 8, 2017

themadcreator added 7 commits June 8, 2017 10:51

Merge remote-tracking branch 'origin/master' into bd/table-perf

6cabc56

Fix tests by reset batcher on state change

f67337d

Merge remote-tracking branch 'origin/master' into bd/table-perf

df28c5a

PR Comments

1752ff1

Fix isotest

cb9a509

fix merge issue

8d7f506

Also batch UPDATES. Add unit test to confirm.

3750624

cmslewis mentioned this pull request Jun 9, 2017

[Table] Fix shouldComponentUpdate issues in *Header's #1215

Merged

themadcreator added 2 commits June 9, 2017 12:47

Merge remote-tracking branch 'refs/remotes/origin/master' into bd/tab…

43969bc

…le-perf

fix isotest

ee759cc

cmslewis reviewed Jun 9, 2017

View reviewed changes

themadcreator added 2 commits June 9, 2017 14:37

Fix resize updates. Update test to confirm

7b9bece

lint

9ca216c

Merge remote-tracking branch 'origin/master' into bd/table-perf

f14cdf2

Merge remote-tracking branch 'origin/master' into bd/table-perf

b8b9e63

PR Comments

9f671dd

cmslewis approved these changes Jun 12, 2017

View reviewed changes

themadcreator merged commit 560d791 into master Jun 12, 2017

themadcreator deleted the bd/table-perf branch June 12, 2017 21:05

cmslewis mentioned this pull request Jun 13, 2017

Prepare Release 1.20.0 #1228

Merged

henrify mentioned this pull request Jul 20, 2017

Table performance degradation with 1.20 (batching malfunctioning?) #1368

Closed

cmslewis mentioned this pull request Aug 30, 2017

Prepare Release 1.27.0 #1513

Merged

[Table] Batch Child Rendering #1201

[Table] Batch Child Rendering #1201

Conversation

themadcreator commented Jun 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jun 9, 2017

fix isotest

themadcreator commented Jun 9, 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

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

themadcreator Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jun 9, 2017

lint

adidahiya commented Jun 10, 2017

themadcreator commented Jun 12, 2017

themadcreator commented Jun 12, 2017

blueprint-bot commented Jun 12, 2017

Merge remote-tracking branch 'origin/master' into bd/table-perf

blueprint-bot commented Jun 12, 2017

Merge remote-tracking branch 'origin/master' into bd/table-perf

blueprint-bot commented Jun 12, 2017

PR Comments

cmslewis left a comment

Choose a reason for hiding this comment

themadcreator Jun 12, 2017 •

edited

Loading