Bug 2053685: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls #11001

jerolimov · 2022-02-03T23:51:54Z

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2053685

Tl;dr
First commit adds only tests, second commit adds caching for previously deep-cloned data.
This cache of the JSON data which are already used in the topology allows the topology and many other components to do less useMemo, useEffect, re-calculations and re-renderings. In topology, this happens because an object replacement in a GraphNode rerenders it.

Analysis / Root cause:
While analyzing our topology performance issues I found two (main) issues which are addressed in this PR. (This is not the first and not the last PR for topology.)

The topology components rerenders too much, also when only a part of the data has changed.
A lot of time is consumed in the immutable toJSON() method.

After further investigation I noticed that the toJSON function is called in Firehose, useK8sWatchResource and useK8sWatchResources when converting an immutable object to JSON. This happens whenever a component is rendered and uses one of the hooks.

It also rerenders or recalculates hooks to often with the same data because the created JSON object contains the same data in a new, deep-clone object or array. But Memod-components, hooks dependencies, and other optimizations like the topology mobx graph tree depends on an identity to check to rerender components..

Solution Description:
This PR improves the topology performance or the performance of many different pages with an "immutable toJSON" result cache in firehose.jsx and k8s-watcher.tsx

It adds a cache and saves the converted JSON data at the immutable object itself. (Thanks Christian for this great idea.)

♻️ To see and test what will change with this caching I first added some tests to the Firehose component and the hooks.

The first commit just adds test for the status quo.
The second commit adds the caching and updates the tests so that it should be easier to see which objects are reused now.

❓ Why not just remove Immutable from the Redux store?!

I think removing Immutable would be a good idea. For the future.

But adding this cache has some benefits for the moment: It's easier and safer to add/test/verify and I hope that we can backport this. I don't expect that we would backport a rewrite of the reducers, and selectors, etc.

⚠️ Potential risk: Reused data models (redux state) is now reused over time and by different components. Changing such a data object would be a bad practice similar to changing a props object.

Videos

Before - Load topology with many deployments (in my case 84)

before-topology-initial-load.mp4

After - Load topology with many deployments (in my case 84)

after-topology-initial-load.mp4

Before - Stay on the topology page with many deployments (here 84) and delete a random pod every 3 second

before-topology-deleting-pods.mp4

After - Stay on the topology page with many deployments (here 84) and delete a random pod every 3 second

after-topology-deleting-pods.mp4

Performance screenshots

Before - Load topology with many deployments (in my case 84)

After - Load topology with many deployments (in my case 84)

Before - Stay on the topology page with many deployments (here 84) and delete a random pod every 3 second

After - Stay on the topology page with many deployments (here 84) and delete a random pod every 3 second

todo

Screen shots / Gifs for design review:
UI isn't changed. Here are som

Unit test coverage report:
Added a lot of tests for the existing components Firehose, useK8sWatchResource and useK8sWatchResources to check the status quo and track the difference with this PR.

Test setup:

Create a project/namespace with a lot of Deployments (50+)
Open the topology
Delete some pods

You can find some scripts to create a cluster with a lot of load here: https://github.com/jerolimov/openshift/tree/master/loadtest

Browser conformance:

Chrome
Firefox
Safari
Edge

jerolimov · 2022-02-04T00:36:40Z

/cc @christianvogt @spadgett @jhadvig @invincibleJai
Hey, I will update the initial comment with more "numbers" and info next week. But I think this is already ready for an initial look.

jerolimov · 2022-02-04T00:51:33Z

/cc @sanketpathak @jeff-phillips-18

frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/k8s-watcher.ts

christianvogt · 2022-02-04T20:20:36Z

frontend/public/components/utils/firehose.jsx

@@ -19,7 +19,9 @@ const shallowMapEquals = (a, b) => {
  return a.every((v, k) => b.get(k) === v);
 };

-const processReduxId = ({ k8s }, props) => {
+const CACHE_SYMBOL = Symbol('_cachedToJSResult');


Safe to use the same symbol as the watcher. However since firehose is old and would probably be more grief to share the symbol with the sdk.

INTERNAL_REDUX_IMMUTABLE_TOJSON_CACHE_SYMBOL is fine for you? 🤣 🤣

I added this to the SDK because we have already an import from firehose.jsx to the SDK. I'm not sure what the "right' import direction here....?!

Having it in the SDK is correct. I dislike exporting such an internal thing but whatever I suppose since we own the console sdk and can manage it ¯\_(ツ)_/¯

christianvogt · 2022-02-04T20:22:01Z

@jerolimov I agree that removing immutable JS is something we should look at doing but it's a much larger change. This is a good first step and proves there's a larger issue to address with immutable js and our redux store.

vikram-raj · 2022-02-10T11:47:56Z

...kages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/__tests__/useK8sWatchResources.data.tsx

+    },
+  })),
+  metadata: { resourceVersion: '123' },
+};


Looks like this file is duplicate of frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/__tests__/useK8sWatchResource.data.tsx.

Yeah I duplicated the test-data files so that they don't depend on each other.

vikram-raj · 2022-02-10T11:52:45Z

frontend/public/components/utils/__tests__/firehose.data.tsx

+export const podData = {
+  apiVersion: 'v1',
+  kind: 'Pod',
+  metadata: {
+    name: 'my-pod',
+    namespace: 'default',
+    resourceVersion: '123',
+  },
+};
+
+export const podList = {
+  apiVersion: 'v1',
+  kind: 'PodList',
+  items: ['my-pod1', 'my-pod2', 'my-pod3'].map((name) => ({
+    apiVersion: 'v1',
+    kind: 'Pod',
+    metadata: {
+      name,
+      namespace: 'default',
+      resourceVersion: '123',
+    },
+  })),
+  metadata: { resourceVersion: '123' },
+};


we can reuse podList and podData from useK8sWatchResource.data.tsx. I think it is better to use podList and podData from here and delete useK8sWatchResource.data.tsx and useK8sWatchResources.data.tsx. wdyt?

This is a different package. Yes, we have already dependencies from public package (console/internal) to packages/console-dynamic-plugin-sdk but I would like to keep a copy here instead of adding more dependencies between both.

invincibleJai · 2022-02-11T07:57:30Z

Thanks @jerolimov I tried to verify it and the performance has improved, I verified topology with the below scenario

with 84-120 deployments: No significant issue was observed, interactions were smooth
with 150 deployments: Okay, a bit slow at times, and interactions were working but not sluggish.
with 200 deployments : app still works but bit sluggish on load and on interactions

Verified on

Chrome Version 98.0.4758.80
mac OSX - 2.6 GHz 6-Core Intel Core i7 / 16GB

spadgett

/approve

I'm OK with the approach as a short-term fix we can backport. @jerolimov Let's open a follow up issue to remove immutable, which seems like a better long-term approach. We should try to give it priority since this change will increase the in-memory size of the k8s resource data.

openshift-ci · 2022-02-11T18:49:04Z

@jerolimov: This pull request references Bugzilla bug 2053685, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

Bug 2053685: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2022-02-11T18:49:11Z

@jerolimov: This pull request references Bugzilla bug 2053685, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

Bug 2053685: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2022-02-11T18:49:27Z

@jerolimov: This pull request references Bugzilla bug 2053685, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

Bug 2053685: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

christianvogt · 2022-02-11T18:55:35Z

/approve

vikram-raj

Verified it using cluster bot. In Chrome and Safari browsers with 100 D and 80 Pods in Topology and it is faster than before.

/lgtm

openshift-ci · 2022-02-14T09:04:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jerolimov, spadgett, vikram-raj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt,jerolimov,spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2022-02-14T18:40:51Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2022-02-14T18:42:45Z

@jerolimov: All pull requests linked via external trackers have merged:

openshift/console#11001

Bugzilla bug 2053685 has been moved to the MODIFIED state.

In response to this:

Bug 2053685: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2022-02-14T18:42:56Z

@jerolimov: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jerolimov · 2022-02-15T16:24:56Z

/cherry-pick release-4.10

openshift-cherrypick-robot · 2022-02-15T16:25:37Z

@jerolimov: new pull request created: #11059

In response to this:

/cherry-pick release-4.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2022

openshift-ci bot requested review from jhadvig and vojtechszocs February 3, 2022 23:52

openshift-ci bot added component/core Related to console core functionality component/sdk Related to console-plugin-sdk approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 3, 2022

openshift-ci bot requested review from christianvogt, invincibleJai and spadgett February 4, 2022 00:36

jerolimov changed the title ~~[WIP] Performance improvement by reducing rerenderings and deep-copy toJSON() calls~~ [WIP] (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls Feb 4, 2022

openshift-ci bot requested review from jeff-phillips-18 and sanketpathak February 4, 2022 00:51

christianvogt reviewed Feb 4, 2022

View reviewed changes

jerolimov added 2 commits February 10, 2022 11:29

Add unit tests for the status quo

73b31cf

Performance improvement by reducing rerenderings and deep toJSON() calls

f387c48

jerolimov force-pushed the improve-immutable-tojs branch from 6489f6d to f387c48 Compare February 10, 2022 10:31

vikram-raj reviewed Feb 10, 2022

View reviewed changes

jerolimov changed the title ~~[WIP] (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls~~ (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls Feb 11, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2022

spadgett approved these changes Feb 11, 2022

View reviewed changes

jerolimov changed the title ~~(Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls~~ Bug 2053685: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls Feb 11, 2022

openshift-ci bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Feb 11, 2022

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 11, 2022

jerolimov requested a review from christianvogt February 11, 2022 18:50

vikram-raj reviewed Feb 14, 2022

View reviewed changes

openshift-ci bot assigned vikram-raj Feb 14, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2022

openshift-merge-robot merged commit 902446d into openshift:master Feb 14, 2022

openshift-cherrypick-robot mentioned this pull request Feb 15, 2022

[release-4.10] Bug 2054757: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls #11059

Merged

jerolimov mentioned this pull request Mar 15, 2022

[release-4.9] Bug 2064454: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls #11184

Merged

invincibleJai mentioned this pull request Mar 30, 2022

Bug 2064454: return process data as array for list true in firehose as done in hooks #11256

Merged

jerolimov deleted the improve-immutable-tojs branch September 22, 2023 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2053685: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls #11001

Bug 2053685: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls #11001

jerolimov commented Feb 3, 2022 •

edited

Loading

jerolimov commented Feb 4, 2022 •

edited

Loading

jerolimov commented Feb 4, 2022

christianvogt Feb 4, 2022

jerolimov Feb 10, 2022

jerolimov Feb 10, 2022

christianvogt Feb 10, 2022

christianvogt commented Feb 4, 2022

vikram-raj Feb 10, 2022

jerolimov Feb 11, 2022

vikram-raj Feb 10, 2022

jerolimov Feb 11, 2022

invincibleJai commented Feb 11, 2022

spadgett left a comment

openshift-ci bot commented Feb 11, 2022

openshift-ci bot commented Feb 11, 2022

openshift-ci bot commented Feb 11, 2022

christianvogt commented Feb 11, 2022

vikram-raj left a comment

openshift-ci bot commented Feb 14, 2022

openshift-bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

jerolimov commented Feb 15, 2022

openshift-cherrypick-robot commented Feb 15, 2022

Bug 2053685: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls #11001

Bug 2053685: (Topology) Performance improvement by reducing rerenderings and deep-copy toJSON() calls #11001

Conversation

jerolimov commented Feb 3, 2022 • edited Loading

Videos

Before - Load topology with many deployments (in my case 84)

After - Load topology with many deployments (in my case 84)

Before - Stay on the topology page with many deployments (here 84) and delete a random pod every 3 second

After - Stay on the topology page with many deployments (here 84) and delete a random pod every 3 second

Performance screenshots

Before - Load topology with many deployments (in my case 84)

After - Load topology with many deployments (in my case 84)

Before - Stay on the topology page with many deployments (here 84) and delete a random pod every 3 second

After - Stay on the topology page with many deployments (here 84) and delete a random pod every 3 second

jerolimov commented Feb 4, 2022 • edited Loading

jerolimov commented Feb 4, 2022

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

christianvogt commented Feb 4, 2022

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

invincibleJai commented Feb 11, 2022

spadgett left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 11, 2022

openshift-ci bot commented Feb 11, 2022

openshift-ci bot commented Feb 11, 2022

christianvogt commented Feb 11, 2022

vikram-raj left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 14, 2022

openshift-bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

jerolimov commented Feb 15, 2022

openshift-cherrypick-robot commented Feb 15, 2022

jerolimov commented Feb 3, 2022 •

edited

Loading

jerolimov commented Feb 4, 2022 •

edited

Loading