From 4b47e2d7e96c9bc19b3fd5f52cd5ea56e37c5bf1 Mon Sep 17 00:00:00 2001
From: Wenzhao Hu <wzhudev@gmail.com>
Date: Thu, 14 Nov 2024 14:54:20 +0800
Subject: [PATCH] docs: guidance on how to fix memory leaks (#4058)

---
 docs/FIX_MEMORY_LEAK.md                       | 53 +++++++++++++++++++
 .../views/sheet-container/SheetContainer.tsx  |  3 +-
 2 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 docs/FIX_MEMORY_LEAK.md

diff --git a/docs/FIX_MEMORY_LEAK.md b/docs/FIX_MEMORY_LEAK.md
new file mode 100644
index 0000000000..a709b39afb
--- /dev/null
+++ b/docs/FIX_MEMORY_LEAK.md
@@ -0,0 +1,53 @@
+# How to Fix Memory Leak
+
+## How to Investigate Memory Leak
+
+First you need to run the demo in E2E mode:
+
+```shell
+pnpm dev:e2e
+```
+
+Then, you can use the Chrome DevTools to investigate the memory leak.
+
+You can create a `Workbook` and then dispose it from Console by running:
+
+```javascript
+E2EControllerAPI.loadAndRelease()
+```
+
+And you can take profiles of the application and see if `Workbook` instances are being retained.
+
+You can also see if the `Univer` instance is being retained after we dispose it by running in Console:
+
+```javascript
+E2EControllerAPI.disposeUniver()
+```
+
+## Frequent Reasons for Memory Leak
+
+### Forget to call dispose subscriptions
+
+For example: dream-num/univer@6423ff8/packages/sheets-drawing-ui/src/controllers/sheet-drawing-update.controller.ts#L244
+
+**HOW TO FIX**: Please remember to dispose subscriptions.
+
+### Get current unit in singleton modules
+
+It is very common to cause memory leak if you get the current unit in singleton modules and subscribe to it. Singleton modules are defined as modules that are registered in the Univer root injector instead of injectors held by render units.
+
+For example: https://github.com/dream-num/univer/blob/dev/packages/sheets-drawing-ui/src/services/canvas-float-dom-manager.service.ts#L433.
+
+**HOW TO FIX**: Please consider extracting the related logic to an `IRenderModule` instead.
+
+### Has big objects in dep arrays of `useEffect` or `useMemo`
+
+React would keep the dep arrays into memory, so if you put a big object into an array, it would cause memory leak.
+
+For example: https://github.com/dream-num/univer/blob/6423ff8ede75ae7b2e003fff99fd9866aa18f1dd/packages/sheets-ui/src/views/sheet-container/SheetContainer.tsx#L88
+
+**HOW TO FIX**: Please consider using `unitId` in dep arrays instead.
+
+## How to Investigate Memory Leak for Node.js
+
+TODO @wzhudev
diff --git a/packages/sheets-ui/src/views/sheet-container/SheetContainer.tsx b/packages/sheets-ui/src/views/sheet-container/SheetContainer.tsx
index 4ef1c21d4f..b9f343db5c 100644
--- a/packages/sheets-ui/src/views/sheet-container/SheetContainer.tsx
+++ b/packages/sheets-ui/src/views/sheet-container/SheetContainer.tsx
@@ -82,9 +82,10 @@ export function RenderSheetContent() {
 function useHasWorkbook(): boolean {
     const univerInstanceService = useDependency(IUniverInstanceService);
     const workbook = useObservable(() => univerInstanceService.getCurrentTypeOfUnit$<Workbook>(UniverInstanceType.UNIVER_SHEET), null, false, []);
+    const hasWorkbook = !!workbook;
     return useMemo(
         () => univerInstanceService.getAllUnitsForType(UniverInstanceType.UNIVER_SHEET).length > 0,
         // eslint-disable-next-line react-hooks/exhaustive-deps
-        [univerInstanceService, workbook]
+        [univerInstanceService, hasWorkbook]
     );
 }