Improve performance with large numbers of untracked or modified files

[parthokunda]

• PR Description
  BuildTreeFromFiles used a linear complexity lookup to find if the children has already been added. Use maps to get constant time lookup for children.

For the test scenario in #3798 (90000 untracked files) this reduces the time of BuildTreeFromFiles from something like 10s down to about 30ms on my machine.

Fixes #3798.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[parthokunda]

BuildTreeFromFiles CPU Profiling result for a initial git repo with data directory having 40000 unstaged files

• without PR: 620ms(1.6%)
• with PR: 20ms(0.09%)

When I profile memory for BuildTreeFromFiles, with PR take 4.46MB (8.4%) whereas without PR one takes 6.50MB (10%). My assumption was that my fix would take more memory. Not sure why it is happening. Would love to know the reason.

[parthokunda]

I have to update the tests. Made a mistake with git when testing.

[stefanhaller]

Thanks for this, it's a great improvement, and the change looks very straight-forward (more than I expected, to be honest).

However, I wonder if we should keep the ChildrenPathMap out of the Node struct. It is really only needed inside the BuildTreeFromFiles function, no need to hold on to it afterwards. This will also solve the test failures automatically.

I'm thinking of a diff like this, on top of yours:

diff --git a/pkg/gui/filetree/build_tree.go b/pkg/gui/filetree/build_tree.go
index 3d0eb713c..926c5ea04 100644
--- a/pkg/gui/filetree/build_tree.go
+++ b/pkg/gui/filetree/build_tree.go
@@ -10,6 +10,8 @@ import (
 func BuildTreeFromFiles(files []*models.File) *Node[models.File] {
 	root := &Node[models.File]{}
 
+	childrenMapsByNode := make(map[*Node[models.File]]map[string]*Node[models.File])
+
 	var curr *Node[models.File]
 	for _, file := range files {
 		splitPath := split(file.Name)
@@ -24,7 +26,13 @@ func BuildTreeFromFiles(files []*models.File) *Node[models.File] {
 
 			path := join(splitPath[:i+1])
 
-			child, isFound := curr.ChildrenPathMap[path]
+			var childrenMap map[string]*Node[models.File]
+			var ok bool
+			if childrenMap, ok = childrenMapsByNode[curr]; !ok {
+				childrenMap = make(map[string]*Node[models.File])
+				childrenMapsByNode[curr] = childrenMap
+			}
+			child, isFound := childrenMap[path]
 			if isFound {
 				curr = child
 				continue outer
@@ -37,10 +45,7 @@ func BuildTreeFromFiles(files []*models.File) *Node[models.File] {
 
 			curr.Children = append(curr.Children, newChild)
 
-			if curr.ChildrenPathMap == nil {
-				curr.ChildrenPathMap = make(map[string]*Node[models.File])
-			}
-			curr.ChildrenPathMap[path] = newChild
+			childrenMap[path] = newChild
 
 			curr = newChild
 		}
diff --git a/pkg/gui/filetree/node.go b/pkg/gui/filetree/node.go
index 9ec5b0ada..e38a1c5de 100644
--- a/pkg/gui/filetree/node.go
+++ b/pkg/gui/filetree/node.go
@@ -18,10 +18,6 @@ type Node[T any] struct {
 	// otherwise it's nil.
 	Children []*Node[T]
 
-	// every child node is accessibe using their path string. Giving O(1) lookup
-	// for their given their path
-	ChildrenPathMap map[string]*Node[T]
-
 	// path of the file/directory
 	Path string
 

I only whipped this up quickly without testing it much, I hope it even works 😄. I also didn't measure if the performance is as good as yours, but I would expect it to be close.

[parthokunda]

You are right, we should surely separate out this map; otherwise it would create confusion. Also, I would not need to rewrite all the unit tests. But in future we should try to make the children field a map itself, maybe? We could use it instead of sorting the list everytime.

Can you make a commit to my branch? Or should I include your edits myself.

[stefanhaller]

But in future we should try to make the children field a map itself, maybe? We could use it instead of sorting the list everytime.

I don't see how this would help with sorting, go's maps are unsorted.

Can you make a commit to my branch? Or should I include your edits myself.

I pushed a fixup to your branch, please squash it in and reword the commit message. And test it a bit more than I did! Thanks. 😄

[stefanhaller]

Oh, and there's probably some more potential for cleaning up the code, I really didn't pay much attention when I wrote it. For example, we now have two bool variables ok and isFound. Also, I wonder if we can avoid doing the childrenMapsByNode[curr] lookup every time through the loop; we really only need to do it again when the value of curr changes. Not sure how much this complicates the code.

[parthokunda]

Hey, I have updated the PR. I changed the variable names to make them a bit more easy to understand.

Also, I wonder if we can avoid doing the childrenMapsByNode[curr] lookup every time through the loop; we really only need to do it again when the value of curr changes. Not sure how much this complicates the code.

I think we should not overcomplicate this code. The lookup should be fast enough anyway.

[stefanhaller]

Awesome, thanks. Great improvement!