You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "justinpark (via GitHub)" <gi...@apache.org> on 2023/11/08 00:42:45 UTC
[PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
justinpark opened a new pull request, #25901:
URL: https://github.com/apache/superset/pull/25901
### SUMMARY
Following up #25282, rendering performance can still be improved by reducing unnecessary rerenders (also known as over-rendering).
This commit improves performance in four areas.
#### 1. Inefficient uniqWith and object equality check in uniqueOptions
https://github.com/apache/superset/blob/d6fde3cdd5987e3bf9a37d1fcde2dc82ef695747/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx#L221-L224
To get the list of unique values, it compares the entire data list (`arrayIncludesWith`) using object equality comparison (`isEqual`). This is slow because the data contains several attributes, and object equality comparison is slow.
As it shown in the above metric, `uniqueOptions` operation triggers four times every time a filter component re-renders, which takes more than 2 seconds per re-render for 10 filters (i.e. 50ms * 4 * 10 filters = 2s).
Using a hashmap approach (with unnecessary equality check) reduced the delay from 50ms * 4 to 10ms (95% reduction)
#### 2. Redundant filter re-renders due to the dynamic sizing
|Initially renders at a minimum width and then expands to its full width.|
|--|
||
Since the native filter's allocated to 100% of its width using SuperChart, which is wrapped by `@vx/responsive`, it triggers re-renders every time the screen size is updated.
This commit removes the resize observer (by assigning the static width) and updates the filter component wrapper to the full width using CSS instead. As a result, the filters no longer re-render when the page size changes.
#### 3. descendants over-rendering
React's inherent behavior of rerendering all descendant components, coupled with the Redux container's rerendering upon state changes, leads to excessive rerendering of dashboard components, including filter components, whenever the chart fetch status updates the Redux store.
This commit replaces DashboardBuilder to the identical descendant (children) which prevents the rerenders when the container component rerenders.
To further optimize performance, the dashboard state synchronization logic was moved to a separate component, effectively decoupling it from the DashboardPage component and preventing unnecessary rerenders of the DashboardPage.
#### 4. Redundant `__cache` attribute
An unnecessary __cache value was introduced in the SelectFilterPlugin in [PR#14869](https://github.com/apache/superset/pull/14869/commits/1749ed542a4f4cedc6847316bf65fc2b82eb1fbd#diff-b4d2de68c8a3b04879772237085ede1b49a6908b6f2890a9ab1aaeae9e18b854R65), but it was never utilized and resulted in superfluous rerenders of filter components.
This commit removes this redundant `__cache` value to resolve the problem.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Successfully optimized the application's performance, resulting in a 10% decrease in rendering count (255 to 228) and a remarkable 60% reduction in loading time (10.9 seconds to 4.5 seconds).
|Before|After|
|--|--|
|||
### TESTING INSTRUCTIONS
1. Go to any dashboard
2. Add more than 10 select filters
3. Reload dashboard
4. See that filters take a lot of time for before render.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #25901:
URL: https://github.com/apache/superset/pull/25901#issuecomment-1808111881
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "jinghua-qa (via GitHub)" <gi...@apache.org>.
jinghua-qa commented on PR #25901:
URL: https://github.com/apache/superset/pull/25901#issuecomment-1813744694
Do some regression test around native filter , did not find issues, LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #25901:
URL: https://github.com/apache/superset/pull/25901#discussion_r1391072758
##########
superset-frontend/src/dashboard/components/SyncDashboardState/index.tsx:
##########
@@ -0,0 +1,103 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect } from 'react';
+import pick from 'lodash/pick';
+import { shallowEqual, useSelector } from 'react-redux';
+import { DashboardContextForExplore } from 'src/types/DashboardContextForExplore';
+import {
+ getItem,
+ LocalStorageKeys,
+ setItem,
+} from 'src/utils/localStorageHelpers';
+import { RootState } from 'src/dashboard/types';
+import { getActiveFilters } from 'src/dashboard/util/activeDashboardFilters';
+
+type Props = { dashboardPageId: string };
+
+const EMPTY_OBJECT = {};
+
+export const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ // A new dashboard tab id is generated on each dashboard page opening.
+ // We mark ids as redundant when user leaves the dashboard, because they won't be reused.
+ // Then we remove redundant dashboard contexts from local storage in order not to clutter it
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardPageId: string,
+ dashboardContext: DashboardContextForExplore,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardPageId]: dashboardContext,
+ });
+};
+
+const SyncDashboardState: React.FC<Props> = ({ dashboardPageId }) => {
Review Comment:
Shouldn't this be a hook rather than a component?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -322,7 +323,7 @@ const FilterValue: React.FC<FilterControlProps> = ({
) : (
<SuperChart
height={HEIGHT}
- width="100%"
+ width={RESPONSIVE_WIDTH}
Review Comment:
What's the advantage of using this `RESPONSIVE_WIDTH` instead of `100%`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #25901:
URL: https://github.com/apache/superset/pull/25901#issuecomment-1808111675
Amazing improvement! Left 2 minor comments for the code.
As often is the case in such refactors, there's a high risk of introducing regressions. Is there any chance we could get some QA help @sadpandajoe @jinghua-qa ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25901:
URL: https://github.com/apache/superset/pull/25901#discussion_r1396536395
##########
superset-frontend/src/dashboard/components/SyncDashboardState/index.tsx:
##########
@@ -0,0 +1,103 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect } from 'react';
+import pick from 'lodash/pick';
+import { shallowEqual, useSelector } from 'react-redux';
+import { DashboardContextForExplore } from 'src/types/DashboardContextForExplore';
+import {
+ getItem,
+ LocalStorageKeys,
+ setItem,
+} from 'src/utils/localStorageHelpers';
+import { RootState } from 'src/dashboard/types';
+import { getActiveFilters } from 'src/dashboard/util/activeDashboardFilters';
+
+type Props = { dashboardPageId: string };
+
+const EMPTY_OBJECT = {};
+
+export const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ // A new dashboard tab id is generated on each dashboard page opening.
+ // We mark ids as redundant when user leaves the dashboard, because they won't be reused.
+ // Then we remove redundant dashboard contexts from local storage in order not to clutter it
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardPageId: string,
+ dashboardContext: DashboardContextForExplore,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardPageId]: dashboardContext,
+ });
+};
+
+const SyncDashboardState: React.FC<Props> = ({ dashboardPageId }) => {
Review Comment:
The reason is related to the topic no.3 `3. descendants over-rendering`
> To further optimize performance, the dashboard state synchronization logic was moved to a separate component, effectively decoupling it from the DashboardPage component and preventing unnecessary rerenders of the DashboardPage.
+ In order to observe state updates and sync them to local storage, it is necessary to include a component with the hook. The most effective approach would be to implement a middleware. In this case, it is skipped to minimize any potential regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #25901:
URL: https://github.com/apache/superset/pull/25901#issuecomment-1815549915
> Thanks jinghua-qa! justinpark Would you rather address my comments in this PR or merge it and do a follow up if needed?
@kgabryje please see my comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #25901:
URL: https://github.com/apache/superset/pull/25901#discussion_r1397345249
##########
superset-frontend/src/dashboard/components/SyncDashboardState/index.tsx:
##########
@@ -0,0 +1,103 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect } from 'react';
+import pick from 'lodash/pick';
+import { shallowEqual, useSelector } from 'react-redux';
+import { DashboardContextForExplore } from 'src/types/DashboardContextForExplore';
+import {
+ getItem,
+ LocalStorageKeys,
+ setItem,
+} from 'src/utils/localStorageHelpers';
+import { RootState } from 'src/dashboard/types';
+import { getActiveFilters } from 'src/dashboard/util/activeDashboardFilters';
+
+type Props = { dashboardPageId: string };
+
+const EMPTY_OBJECT = {};
+
+export const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ // A new dashboard tab id is generated on each dashboard page opening.
+ // We mark ids as redundant when user leaves the dashboard, because they won't be reused.
+ // Then we remove redundant dashboard contexts from local storage in order not to clutter it
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardPageId: string,
+ dashboardContext: DashboardContextForExplore,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardPageId]: dashboardContext,
+ });
+};
+
+const SyncDashboardState: React.FC<Props> = ({ dashboardPageId }) => {
Review Comment:
I see, thank you for explanation!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "jinghua-qa (via GitHub)" <gi...@apache.org>.
jinghua-qa commented on PR #25901:
URL: https://github.com/apache/superset/pull/25901#issuecomment-1813030784
> Amazing improvement! Left 2 minor comments for the code. As often is the case in such refactors, there's a high risk of introducing regressions. Is there any chance we could get some QA help @sadpandajoe @jinghua-qa ?
I will take a look for the regression
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25901:
URL: https://github.com/apache/superset/pull/25901#issuecomment-1808114967
@kgabryje Ephemeral environment spinning up at http://35.87.180.132:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #25901:
URL: https://github.com/apache/superset/pull/25901#issuecomment-1814549554
Thanks @jinghua-qa! @justinpark Would you rather address my comments in this PR or merge it and do a follow up if needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25901:
URL: https://github.com/apache/superset/pull/25901#discussion_r1396399064
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -322,7 +323,7 @@ const FilterValue: React.FC<FilterControlProps> = ({
) : (
<SuperChart
height={HEIGHT}
- width="100%"
+ width={RESPONSIVE_WIDTH}
Review Comment:
The RESPONSE_WIDTH change is related to the topic no.2 (`2. Redundant filter re-renders due to the dynamic sizing`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25901:
URL: https://github.com/apache/superset/pull/25901#issuecomment-1802418602
CC @kgabryje who has looked into this sort of thing in this area before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25901:
URL: https://github.com/apache/superset/pull/25901#issuecomment-1819557696
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native filters): rendering performance improvement by reduce overrendering [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark merged PR #25901:
URL: https://github.com/apache/superset/pull/25901
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org