perf(sqllab): Rendering perf improvement using immutable state #20877

justinpark · 2022-07-26T22:44:13Z

SUMMARY

The main rendering in SqlLab is managed by the queryEditors state from redux(state machine).
Each queryEditor state has been updated whenever an editor configuration has updated.

Since most components observes entire queryEditor state not specific attributes, any queryEditor updates trigger multiple repainting even if no visual changes impacted.

For example, when a user types any single character in the ace(sql) editor, it triggers the update for selectedText value in the active queryEditor state. This single change will trigger the following components repainted (though this change won't impact the left and bottom panels at all)

This unrelated repainting job causes the performance delay especially when table list is humongous.

The delay can be found in the following performance analysis due to the unnecessary repainting/redux post processes.

This commit fixes this performance issue by optimizing observers in each component to subscribe only related items.
To minimize the regression issue from existing persistence state, this commit keeps the existing queryEditors state but make it immutable during the tab is active and only updates when the tab is switched. It introduces unsavedQueryEditor state which stores all these active changes.

With this update, the page rendering performance improved 3.5x faster(from 4.5s to 1.3s) and verified all unnecessary tasks cleaned.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before (4.5s):

slow-sqllab.mov

After (1.3s):

TESTING INSTRUCTIONS

Go to SqlLab
Choose a database and a schema that includes a large set of table list
Open chrome console > Performance tab
Select Caption settings on right menu
Change CPU throttling to 6x slowdown
type a string in the sql editor and then watch the refresh icon on the left panel (it will blink during repainting)

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-07-27T00:19:27Z

Codecov Report

Merging #20877 (dfb30fc) into master (8005b7f) will increase coverage by 0.07%.
The diff coverage is 64.66%.

@@            Coverage Diff             @@
##           master   #20877      +/-   ##
==========================================
+ Coverage   66.27%   66.34%   +0.07%     
==========================================
  Files        1770     1772       +2     
  Lines       67543    67570      +27     
  Branches     7185     7187       +2     
==========================================
+ Hits        44764    44831      +67     
+ Misses      20940    20893      -47     
- Partials     1839     1846       +7

Flag	Coverage Δ
javascript	`52.15% <64.66%> (+0.15%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/SqlLab/App.jsx	`0.00% <0.00%> (ø)`
superset-frontend/src/SqlLab/types.ts	`57.14% <ø> (ø)`
...uperset-frontend/src/components/Dropdown/index.tsx	`100.00% <ø> (ø)`
...et-frontend/src/components/TableSelector/index.tsx	`78.26% <ø> (+2.17%)`	⬆️
superset-frontend/src/SqlLab/actions/sqlLab.js	`61.37% <41.66%> (+1.26%)`	⬆️
superset-frontend/src/SqlLab/reducers/sqlLab.js	`34.85% <43.75%> (+1.52%)`	⬆️
...frontend/src/SqlLab/components/SqlEditor/index.jsx	`50.58% <47.05%> (-1.05%)`	⬇️
...c/SqlLab/components/TemplateParamsEditor/index.tsx	`82.35% <50.00%> (+7.35%)`	⬆️
...et-frontend/src/SqlLab/reducers/getInitialState.js	`46.66% <50.00%> (+0.23%)`	⬆️
...d/src/SqlLab/components/SqlEditorLeftBar/index.tsx	`50.00% <66.66%> (+0.79%)`	⬆️
... and 17 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-actions · 2022-07-27T17:29:22Z

@justinpark Ephemeral environment creation is currently limited to committers.

ktmud · 2022-07-28T00:08:01Z

/testenv up

ktmud · 2022-07-28T20:38:32Z

/testenv up

github-actions · 2022-07-28T20:40:22Z

@ktmud Ephemeral environment spinning up at http://34.220.154.64:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

ktmud · 2022-08-02T23:18:23Z

/testenv up

github-actions · 2022-08-02T23:22:45Z

@ktmud Ephemeral environment spinning up at http://34.217.34.22:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

ktmud

All the SQL Lab code is a mystery to me. But I've tested this quite extensively locally and did see significant improvements in typing performance in SQL editor. I trust the test cases to make sure the majority of the happy paths are still happy.

hughhhh · 2022-08-04T16:24:37Z

superset-frontend/src/SqlLab/actions/sqlLab.test.js

    sql: 'SELECT *\nFROM\nWHERE',
    name: 'Untitled Query 1',
-    schemaOptions: [{ value: 'main', label: 'main', name: 'main' }],
+    schemaOptions: [{ value: 'main', label: 'main', title: 'main' }],


why are we changing this to title

@hughhhh This change(title -> name) accidentally happened here while the PR made the change for queryEditor title to name.

cc: @lyndsiWilliams

I reverted this change to pass the type role since I'm following this type definition

and it's also allocated as title here:

superset/superset-frontend/src/components/DatabaseSelector/index.tsx

Lines 224 to 228 in eb5369f

const options = json.result.map((s: string) => ({

value: s,

label: s,

title: s,

}));

hughhhh · 2022-08-05T17:06:58Z

/testenv up

hughhhh · 2022-08-05T17:12:46Z

superset-frontend/src/SqlLab/components/SaveQuery/index.tsx

@@ -178,7 +193,9 @@ export default function SaveQuery({
        onHide={() => setShowSaveDatasetModal(false)}
        buttonTextOnSave={t('Save & Explore')}
        buttonTextOnOverwrite={t('Overwrite & Explore')}
-        datasource={getDatasourceAsSaveableDataset(query)}
+        datasource={
+          getDatasourceAsSaveableDataset(query) as any as ISaveableDatasource


why are casting as any and then ISaveableDatasource here

oh it needed this custom type cast but no longer needed. I reverted this change.

github-actions · 2022-08-05T17:16:18Z

@hughhhh Ephemeral environment spinning up at http://34.217.110.42:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

eschutho

small nits, but one could pose a runtime error.

eschutho · 2022-08-08T22:39:21Z

superset-frontend/src/SqlLab/App.jsx

@@ -91,6 +94,12 @@ const sqlLabPersistStateConfig = {
      const result = {
        ...initialState,
        ...persistedState,
+        sqlLab: {
+          ...(persistedState && persistedState.sqlLab),


if persistedState is null or undefined, this will error when you try to spread it. I'd recommend a default of an empty object.. maybe something like

Suggested change

...(persistedState && persistedState.sqlLab),

...(persistedState?.sqlLab || {}),

eschutho · 2022-08-08T22:46:46Z

superset-frontend/src/SqlLab/actions/sqlLab.js

@@ -620,6 +677,7 @@ export function switchQueryEditor(queryEditor, displayLimit) {
  return function (dispatch) {
    if (
      isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE) &&
+      queryEditor &&


maybe use optional chaining here as well?

sounds good

eschutho · 2022-08-08T23:04:16Z

superset-frontend/src/SqlLab/actions/sqlLab.js

+    const qe = getUpToDateQuery(getState(), queryEditor, queryEditor.id);
+    const query = {
+      dbId: qe.dbId,
+      sql: qe.selectedText ? qe.selectedText : qe.sql,


nit:

Suggested change

sql: qe.selectedText ? qe.selectedText : qe.sql,

sql: qe.selectedText || qe.sql,

jinghua-qa

Cypress change LGTM

eschutho · 2022-08-10T20:28:34Z

superset-frontend/src/SqlLab/components/SqlEditorTabHeader/index.tsx

+
+  return (
+    <TabTitleWrapper>
+      <Dropdown


Not sure if this was expected, but can we bring back the trigger={['click']} on this component, too?

- keep queryEditors immutable during active state - add unsavedQueryEditor to store all active changes - refactor each component to subscribe the related unsaved editor state only

justinpark · 2022-08-17T23:07:54Z

@eschutho I updated the commit. Can you review the update? thanks!

EugeneTorap · 2022-08-23T15:15:59Z

@eschutho @michael-s-molina Can we merge the PR?

github-actions · 2022-08-23T15:17:42Z

Ephemeral environment shutdown and build artifacts deleted.

…e#20877) * perf(sqllab): Rendering perf improvement using immutable state - keep queryEditors immutable during active state - add unsavedQueryEditor to store all active changes - refactor each component to subscribe the related unsaved editor state only * revert ISaveableDatasource type cast * missing trigger prop * a default of an empty object and optional operator (cherry picked from commit ddb5fcdc0cb597c2fd947d39a05055c2949c29a7)

#20877)" This reverts commit f77b910.

apache#20877)" This reverts commit f77b910.

…ble state (apache#20877)"" This reverts commit 8b1886a.

pull-request-size bot added the size/XXL label Jul 26, 2022

ktmud requested review from ktmud and etr2460 July 28, 2022 00:21

justinpark force-pushed the perf--sqllab-rendering-delay-improvement branch from 4ca0186 to e4d3876 Compare August 2, 2022 18:24

justinpark requested a review from jinghua-qa as a code owner August 2, 2022 22:29

ktmud approved these changes Aug 2, 2022

View reviewed changes

hughhhh reviewed Aug 4, 2022

View reviewed changes

justinpark force-pushed the perf--sqllab-rendering-delay-improvement branch from a4dcfe2 to f282688 Compare August 5, 2022 03:37

hughhhh reviewed Aug 5, 2022

View reviewed changes

justinpark force-pushed the perf--sqllab-rendering-delay-improvement branch from f282688 to 7417f72 Compare August 5, 2022 17:50

eschutho requested changes Aug 8, 2022

View reviewed changes

eschutho reviewed Aug 8, 2022

View reviewed changes

jinghua-qa approved these changes Aug 8, 2022

View reviewed changes

eschutho reviewed Aug 10, 2022

View reviewed changes

justinpark added 4 commits August 17, 2022 09:12

perf(sqllab): Rendering perf improvement using immutable state

b01e756

- keep queryEditors immutable during active state - add unsavedQueryEditor to store all active changes - refactor each component to subscribe the related unsaved editor state only

revert ISaveableDatasource type cast

b799385

missing trigger prop

5920999

a default of an empty object and optional operator

dfb30fc

justinpark force-pushed the perf--sqllab-rendering-delay-improvement branch from 0b1fa32 to dfb30fc Compare August 17, 2022 16:19

justinpark requested a review from eschutho August 17, 2022 16:21

eschutho approved these changes Aug 22, 2022

View reviewed changes

michael-s-molina merged commit f77b910 into apache:master Aug 23, 2022

hughhhh added a commit that referenced this pull request Sep 1, 2022

Revert "perf(sqllab): Rendering perf improvement using immutable state (

15c713e

#20877)" This reverts commit f77b910.

hughhhh mentioned this pull request Sep 1, 2022

Revert "perf(sqllab): Rendering perf improvement using immutable stat… #21298

Closed

9 tasks

This was referenced Sep 1, 2022

fix(sqllab): invalid table metadata request #21304

Merged

fix(sqllab): Copy link doesn't apply the unsaved changes #21311

Merged

sadpandajoe added a commit to preset-io/superset that referenced this pull request Sep 2, 2022

Revert "perf(sqllab): Rendering perf improvement using immutable state (

8b1886a

apache#20877)" This reverts commit f77b910.

justinpark added a commit to airbnb/superset-fork that referenced this pull request Sep 16, 2022

Revert "Revert "perf(sqllab): Rendering perf improvement using immuta…

071d91b

…ble state (apache#20877)"" This reverts commit 8b1886a.

justinpark mentioned this pull request Oct 7, 2022

fix(sqllab): resultset disappeared on switching tabs #21741

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(sqllab): Rendering perf improvement using immutable state #20877

perf(sqllab): Rendering perf improvement using immutable state #20877

justinpark commented Jul 26, 2022 •

edited

codecov bot commented Jul 27, 2022 •

edited

github-actions bot commented Jul 27, 2022

ktmud commented Jul 28, 2022

ktmud commented Jul 28, 2022

github-actions bot commented Jul 28, 2022

ktmud commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

ktmud left a comment

hughhhh Aug 4, 2022

justinpark Aug 4, 2022

hughhhh commented Aug 5, 2022

hughhhh Aug 5, 2022

justinpark Aug 5, 2022

github-actions bot commented Aug 5, 2022

eschutho left a comment

eschutho Aug 8, 2022

eschutho Aug 8, 2022

justinpark Aug 17, 2022

eschutho Aug 8, 2022

jinghua-qa left a comment

eschutho Aug 10, 2022

justinpark commented Aug 17, 2022

EugeneTorap commented Aug 23, 2022

github-actions bot commented Aug 23, 2022

	const options = json.result.map((s: string) => ({
	value: s,
	label: s,
	title: s,
	}));

	...(persistedState && persistedState.sqlLab),
	...(persistedState?.sqlLab \|\| {}),

	sql: qe.selectedText ? qe.selectedText : qe.sql,
	sql: qe.selectedText \|\| qe.sql,

perf(sqllab): Rendering perf improvement using immutable state #20877

perf(sqllab): Rendering perf improvement using immutable state #20877

Conversation

justinpark commented Jul 26, 2022 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 27, 2022 • edited

Codecov Report

github-actions bot commented Jul 27, 2022

ktmud commented Jul 28, 2022

ktmud commented Jul 28, 2022

github-actions bot commented Jul 28, 2022

ktmud commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

ktmud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughhhh commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 5, 2022

eschutho 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

jinghua-qa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinpark commented Aug 17, 2022

EugeneTorap commented Aug 23, 2022

github-actions bot commented Aug 23, 2022

justinpark commented Jul 26, 2022 •

edited

codecov bot commented Jul 27, 2022 •

edited