Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Desktop, Mobile: Fixes #10674: Fix sidebar performance regression with many nested notebooks #10676

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Jul 3, 2024

Summary

This pull request fixes a performance regression related to the time complexity of side-menu-shared/renderFolders.

In particular, f19b1c5 changed how parent folders are determined. In particular, it added an additional loop over all folders (with .find) when determining the parent of an item in renderFolders.

Fixes #10674, #9889.

More detailed explanation

Based on code changes, the regression seems to have been introduced by f19b1c5, with the change in how folders' display parent ID is calculated. For example, this change added an additional loop within folderHasChildren_:

for (let i = 0; i < folders.length; i++) {
	const folder = folders[i];
-	if (folder.parent_id === folderId) return true;
+	const folderParentId = getDisplayParentId(folder, folders.find(f => f.id === folder.parent_id));
+	if (folderParentId === folderId) return true;
}

This change caused folderHasChildren_ to be in $\mathcal{O}(n^2)$ time, where $n$ is folders.length. Previously, it was in $\mathcal{O}(n)$.

folderHasChildren_ is itself used in a loop over all folders:

for (let i = 0; i < folders.length; i++) {
const folder = folders[i];
const folderParentId = getDisplayParentId(folder, props.folders.find(f => f.id === folder.parent_id));
if (!Folder.idsEqual(folderParentId, parentId)) continue;
if (folderIsCollapsed(props.folders, folder.id, props.collapsedFolderIds)) continue;
const hasChildren = folderHasChildren_(folders, folder.id);

If the folder has children, it then recurses, passing the same props:

order.push(folder.id);
items.push(renderItem(folder, hasChildren, depth));
if (hasChildren) {
const result = renderFoldersRecursive_(props, renderItem, items, folder.id, depth + 1, order);

The recursive call then iterates over all folders and uses a nested loop (with .find) to determine the parent ID of each:

for (let i = 0; i < folders.length; i++) {
const folder = folders[i];
const folderParentId = getDisplayParentId(folder, props.folders.find(f => f.id === folder.parent_id));

Based on this, the running time of renderFoldersRecursive_ went from $\mathcal{O}\big((n)(n +T(n))\big)$ (where $T(n)$ represents the recursive case) to $\mathcal{O}((n)(n^2 + T(n)))$.

Testing plan

On desktop:

  1. Create a folder tree several thousand folders and with up to three levels of nesting, with most nested under a single notebook.
  2. Expand the toplevel notebook.
  3. Verify that step 2 completed quickly.
  4. Collapse the toplevel notebook.
  5. Verify that step 4 completed quickly.

This has been tested successfully on Ubuntu 24.04.

Screen recording:

folders.mp4

On mobile

  1. Create 2-3 folders.
  2. Nest one folder under another.
  3. Collapse and expand the toplevel folder.

…ssion with many nested notebooks

Fixes a regression that seems to be introduced by
f19b1c5. This seems related to
`folderHasChildren_` and related methods going from O(n) to O(n^2) (new
`folders.find(f => f.id === parent_id)` logic to find original parent
items).

This commit fixes the issue by caching parent items in a `Map`.
@laurent22 laurent22 merged commit 320d0df into laurent22:release-3.0 Jul 4, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants