perf: faster legacy table chart #385

ktmud · 2020-02-29T06:34:35Z

This commit tries to optimize the performance of the legacy data table.

Still use the jquery.databables plugin.
Instead of d3 and reactify, we create the DOM with React itself and initialize jquery.DataTables with a useEffect hook.
Removed dependency on dompurify. Now that HTML tags will be rendered as plain text (by React) instead of innerHTML.
Removed a couple of not-so-popular features. See notes below.
Added a script to build only specific plugin for faster development.
Restyled the pagination so it looks nicer in dashboards.

Plus some minor changes to fix linting and building.

Related: apache/superset#9234

Screenshots

Tested with a ~10,000 rows, 7 columns table in Superset, pagination enabled:

Before

Takes about 6 secs to load.

After

Takes only 3 secs to load!

🏆 Enhancements

netlify · 2020-02-29T06:45:06Z

Deploy preview for superset-ui-plugins ready!

Built with commit 2c903f3

https://deploy-preview-385--superset-ui-plugins.netlify.com

ktmud

Some notes.

package.json

packages/superset-ui-plugins-demo/package.json

packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx

packages/superset-ui-plugins-demo/.storybook/storybook.css

packages/superset-ui-plugins-demo/storybook/stories/legacy-plugin-chart-table/Stories.tsx

packages/superset-ui-preset-chart-xy/src/encodeable/AxisAgent.ts

packages/superset-ui-legacy-plugin-chart-table/src/Table.css

This commit tries to optimize the performance of the legacy data table. 1. Converting everything to Typescript. 2. Create a native React component instead of `reactify` (although all DOM operaions still happen in the jQuery.DataTables plugin. 3. Remove dependency on d3, optimize how bars are rendered. Plus some minor changes to fix linting and building. Also added a script to build only specific plugin for faster development.

Unfortunately jquery.datatables uses innerHTML to create cell content, and all rows are created even with pagination. https://github.com/DataTables/DataTables/blob/83657a29e33ce93ee940ce25684940eb3acb2913/media/js/jquery.dataTables.js#L3113-L3117 This is slow and insecure. We are reverting to DOM data source as in previous implementation, but instead of using D3 to create the table rows, we use React. This also renders `dompurify.sanitize` unnecessary since React will take care of it.

kristw

Removed dependency on dompurify. Now that HTML tags will be rendered as plain text (by React) instead of innerHTML.

There are columns values that are html strings, such as html links. You would need dangerouslySetInnerHTML and dompurify or use interweave.

packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx

kristw · 2020-03-02T21:56:36Z

Could we add bottom padding for the pagination?
Also, could you share a screenshot of how does it look in Superset? I think Superset override some css and the table may look slightly different.

packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx

Also 1. improve height adjustment 2. add page size control to storybook 3. hide rows from later pages at initial rendering

packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx

packages/superset-ui-legacy-plugin-chart-table/src/index.ts

kristw · 2020-03-03T22:34:37Z

The build seems to be broken on xss

ktmud · 2020-03-03T22:44:19Z

Addition information on js-xss vs dompurify, on the same 10k row sample data in Storybook, it takes ~12 secs for dompurify to finish rendering React DOM nodes and 3.5 secs for js-xss.

DOMPurify:

xss:

Plus minor variable name and comment update

kristw · 2020-03-03T23:53:24Z

Code looks good. Some eslint errors breaking ci.
Have to do something with the title.

codecov · 2020-03-04T02:46:15Z

Codecov Report

Merging #385 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #385      +/-   ##
=========================================
+ Coverage    1.47%   1.48%   +<.01%     
=========================================
  Files         186     185       -1     
  Lines        5824    5805      -19     
  Branches      342     370      +28     
=========================================
  Hits           86      86              
+ Misses       5726    5707      -19     
  Partials       12      12

Impacted Files	Coverage Δ
...ui-legacy-plugin-chart-table/src/transformProps.ts	`0% <0%> (ø)`
...set-ui-preset-chart-xy/src/encodeable/AxisAgent.ts	`0% <0%> (ø)`	⬆️
...i-legacy-plugin-chart-table/src/ReactDataTable.tsx	`0% <0%> (ø)`
...superset-ui-legacy-plugin-chart-table/src/index.ts	`0% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb7ef62...2c903f3. Read the comment docs.

packages/superset-ui-legacy-plugin-chart-table/package.json

packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx

kristw

just a few nits

Upgrade table chart `@superset-ui/legacy-plugins-chart-table` to apache-superset/superset-ui-plugins#385

* perf(table-chart): upgrade to 0.11.6 Upgrade table chart `@superset-ui/legacy-plugins-chart-table` to apache-superset/superset-ui-plugins#385 * refactor: use ternary instead of if * fix: rename variables

* feat: faster legacy table chart This commit tries to optimize the performance of the legacy data table. 1. Converting everything to Typescript. 2. Create a native React component instead of `reactify` (although all DOM operaions still happen in the jQuery.DataTables plugin. 3. Remove dependency on d3, optimize how bars are rendered. Plus some minor changes to fix linting and building. Also added a script to build only specific plugin for faster development. * feat(legacy-table-chart): use React to render DOM Unfortunately jquery.datatables uses innerHTML to create cell content, and all rows are created even with pagination. https://github.com/DataTables/DataTables/blob/83657a29e33ce93ee940ce25684940eb3acb2913/media/js/jquery.dataTables.js#L3113-L3117 This is slow and insecure. We are reverting to DOM data source as in previous implementation, but instead of using D3 to create the table rows, we use React. This also renders `dompurify.sanitize` unnecessary since React will take care of it. * feat(legacy-table-chart): support html in data cells Also 1. improve height adjustment 2. add page size control to storybook 3. hide rows from later pages at initial rendering * fix(legacy-data-table): minor formatting fixes * chore(legacy-table): add xss dependency Plus minor variable name and comment update * fix(legacy-table): linting errors * refactor(legacy-table): more predictable metric labels * feat(legacy-table): also display title for percent metric * fix(legacy-table): typos, var names, etc * docs: update notes for metric.label * refactor(legacy-table): upgrade number-format * fix(legacy-table): upgrade dependency for storybook and yarn

ktmud requested a review from a team as a code owner February 29, 2020 06:34

ktmud force-pushed the react-legacy-table branch from d20f949 to 82b3f76 Compare February 29, 2020 06:38

ktmud force-pushed the react-legacy-table branch 2 times, most recently from e0ad5ef to 1c1f41d Compare February 29, 2020 06:58

ktmud commented Feb 29, 2020

View reviewed changes

ktmud force-pushed the react-legacy-table branch from 1c1f41d to a37bd8c Compare February 29, 2020 07:22

kristw reviewed Mar 2, 2020

View reviewed changes

packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx Outdated Show resolved Hide resolved

kristw reviewed Mar 2, 2020

View reviewed changes

packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx Outdated Show resolved Hide resolved

ktmud added 2 commits March 3, 2020 12:14

feat(legacy-table-chart): support html in data cells

5f0308d

Also 1. improve height adjustment 2. add page size control to storybook 3. hide rows from later pages at initial rendering

fix(legacy-data-table): minor formatting fixes

866d80c

kristw reviewed Mar 3, 2020

View reviewed changes

chore(legacy-table): add xss dependency

f20e01b

Plus minor variable name and comment update

ktmud mentioned this pull request Mar 3, 2020

feat: improve loading speed for legacy table chart apache/superset#9234

Merged

7 tasks

ktmud added 2 commits March 3, 2020 17:10

fix(legacy-table): linting errors

5ab1e43

refactor(legacy-table): more predictable metric labels

3ec8414

ktmud force-pushed the react-legacy-table branch from b642b14 to 3ec8414 Compare March 4, 2020 02:42

feat(legacy-table): also display title for percent metric

b6b7e1c

kristw reviewed Mar 4, 2020

View reviewed changes

packages/superset-ui-legacy-plugin-chart-table/package.json Outdated Show resolved Hide resolved

kristw reviewed Mar 4, 2020

View reviewed changes

packages/superset-ui-legacy-plugin-chart-table/package.json Outdated Show resolved Hide resolved

kristw reviewed Mar 4, 2020

View reviewed changes

packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx Outdated Show resolved Hide resolved

packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx Outdated Show resolved Hide resolved

fix(legacy-table): typos, var names, etc

5cb7730

kristw reviewed Mar 4, 2020

View reviewed changes

packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx Outdated Show resolved Hide resolved

kristw approved these changes Mar 4, 2020

View reviewed changes

ktmud added 2 commits March 4, 2020 11:28

docs: update notes for metric.label

95eb028

refactor(legacy-table): upgrade number-format

b1619d2

ktmud changed the title ~~feat: faster legacy table chart~~ perf: faster legacy table chart Mar 4, 2020

fix(legacy-table): upgrade dependency for storybook and yarn

2c903f3

kristw approved these changes Mar 5, 2020

View reviewed changes

kristw merged commit 8b1a7ac into apache-superset:master Mar 5, 2020

ktmud added a commit to ktmud/superset that referenced this pull request Mar 6, 2020

perf(table-chart): upgrade to 0.11.6

8243b91

Upgrade table chart `@superset-ui/legacy-plugins-chart-table` to apache-superset/superset-ui-plugins#385

ktmud mentioned this pull request Mar 11, 2020

fix(legacy-table): container height on tall headers #398

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: faster legacy table chart #385

perf: faster legacy table chart #385

ktmud commented Feb 29, 2020 •

edited

netlify bot commented Feb 29, 2020 •

edited

ktmud left a comment

kristw left a comment

kristw commented Mar 2, 2020 •

edited

kristw commented Mar 3, 2020

ktmud commented Mar 3, 2020 •

edited

kristw commented Mar 3, 2020

codecov bot commented Mar 4, 2020 •

edited

kristw left a comment

perf: faster legacy table chart #385

perf: faster legacy table chart #385

Conversation

ktmud commented Feb 29, 2020 • edited

Screenshots

Before

After

netlify bot commented Feb 29, 2020 • edited

ktmud left a comment

Choose a reason for hiding this comment

kristw left a comment

Choose a reason for hiding this comment

kristw commented Mar 2, 2020 • edited

kristw commented Mar 3, 2020

ktmud commented Mar 3, 2020 • edited

kristw commented Mar 3, 2020

codecov bot commented Mar 4, 2020 • edited

Codecov Report

kristw left a comment

Choose a reason for hiding this comment

ktmud commented Feb 29, 2020 •

edited

netlify bot commented Feb 29, 2020 •

edited

kristw commented Mar 2, 2020 •

edited

ktmud commented Mar 3, 2020 •

edited

codecov bot commented Mar 4, 2020 •

edited