[JEWEL-1148] Fix BasicLazyTree Memory Leak#3468
Open
DanielSouzaBertoldi wants to merge 1 commit intoJetBrains:masterfrom
Open
[JEWEL-1148] Fix BasicLazyTree Memory Leak#3468DanielSouzaBertoldi wants to merge 1 commit intoJetBrains:masterfrom
DanielSouzaBertoldi wants to merge 1 commit intoJetBrains:masterfrom
Conversation
be5f312 to
08759e5
Compare
| @@ -178,8 +179,19 @@ public fun <T> BasicLazyTree( | |||
| ) { | |||
| val scope = rememberCoroutineScope() | |||
|
|
|||
| val flattenedTree = | |||
| remember(tree, treeState.openNodes, treeState.allNodes) { tree.roots.flatMap { it.flattenTree(treeState) } } | |||
| val flattenedTree = remember(tree, treeState.openNodes) { | |||
Collaborator
Author
There was a problem hiding this comment.
Read the final collapsed section in the PR description. This is part of the Extra Issue 1.
Also, we don't need treeState.allNodes keyed here because it was a self-defeating loop:
openNodes changes → remember reruns → flattenTree adds new IDs to allNodes → allNodes changes → remember reruns again → flattenTree finds nothing new to add → stable
| itemsIndexed( | ||
| items = flattenedTree, | ||
| key = { _, item -> item.id }, | ||
| contentType = { _, item -> item.data?.let { it::class } }, |
Collaborator
Author
There was a problem hiding this comment.
The actual "real" fix
| @@ -422,8 +436,10 @@ private fun Tree.Element<*>.flattenTree(state: TreeState): MutableList<Tree.Elem | |||
| } | |||
|
|
|||
| private infix fun MutableSet<Any>.getAllSubNodes(node: Tree.Element.Node<*>) { | |||
| node.children?.filterIsInstance<Tree.Element.Node<*>>()?.forEach { | |||
Collaborator
Author
There was a problem hiding this comment.
Using filterIsInstance here is part of the "Extra issue 2" described in the PR
08759e5 to
d614cac
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
We received a report of severe OutOfMemory (OOM) issues and CPU bottlenecks when using
LazyTreewith a dynamically recalculating tree (e.g., updating based on frequent background processes). Profiling revealed that after just 1,000 process spawns, the tree was retaining over 230k references to old nodes.The workaround in the user's scenario was to use the
treeitself as a key torememberso that it forced a recomposition and cleared the references to old nodes.No need to say, this smells heavily like a memory leak in the
LazyTree, which demands a proper investigation.Pick up your magnifying glass, choose the most appropriate trench coat and let's dissect this 😎
Investigation
Note
TL;DR: Fixed severe OOM issues by using
item.data::classforcontentTypeto properly recycle Compose nodes and purging stale IDs fromTreeStateupon rebuilds. We also eliminated hidden list allocations during tree traversal, dropping the retained heap by over ~70% and smoothing out UI stutter.Findings
The bug report had a reproducible code we could use for profiling, so let's use it.
After adding about 7k entries to the table:
Dumping the heap (by running
jcmd <PID> GC.heap_dump test-dump.hprof) and using Eclipse's Memory Analyzer tool, in the histogram tab we could see that bothorg.jetbrains.jewel.samples.showcase.components.TestTreeNodeandorg.jetbrains.jewel.foundation.lazy.tree.Tree$Element$Nodehad a really big retained heap size:If you'd rather see a screenshot, click here!
Which is honestly insane. A little over 1MB in retained objects for just ~7k lines of text. Something is really wrong here. Let's go a little bit deeper and check the path to GC roots to some of the
Tree$Element$Nodeobjects (btw, some of these objects retain only 88 bytes and others 168 bytes in the heap. The thing though, is that there are 7,168 objects 😬).Alright, let's take a closer look at what each object is retaining and why GC is not cleaning them up:
There's a lot of things here but my main point is: all classes that retain most of the heap (all selected rows besides the first three) are from Compose itself, which is holding our
datahostage:RootNodeOwner$OwnerImpl(The root of the Compose UI tree)LayoutNode(A generic Compose UI element)BasicLazyTreeKt$$Lambda...)Tree$Element$Node.We can rule out any wrongdoing on Compose's side, since this is just its inner workings. It's the LazyColumn item reuse pool in action. If you don't aren't familiar, when you scroll an item off-screen, or when you rebuild the tree with new items, Compose doesn't want to destroy the expensive
LayoutNodesjust to recreate them a millisecond later. Instead, it detaches them and puts them in a cache (the reuse pool) so they can be recycled for the next item that scrolls into view.The main problem though, is that the Compose is being unable to reuse
LayoutNodes. It should NOT have a pool of 7,168 entries at all. This means that each and every entry in the list is being treated as a completely unique UI layout.Also, there's something shady going on with our
org.jetbrains.jewel.foundation.lazy.SelectableLazyColumnKt$SelectableLazyColumn$notifyingPointerEventActionsclass. It's holding about 63kB in the heap, which isn't a lot but still, it shouldn't be doing that. Maybe they have the same root cause (foreshadowing intensifies).Finding the Culprit
Given our findings, the only way for Compose to be treating all entries as unique layouts is due to a sneaky line of code:
Given that each data has a unique test in this reproducible (e.g, "bar , foo foofoo"), every single entry has a 100% unique content type. Because the content types are unique,
LazyColumncreates a brand new reuse bucket for every single item that is ever removed. It caches exactly 1 node per bucket, and since there are hundreds of thousands of unique buckets, it holds onto every single oldTree.Elementforever 😬The Fix
By simply just using the data class as the content type, we get rid of the OOM. This way, Compose can cap the reuse pools to exactly two buckets (
Tree.Element.Node::classandTree.Element.Leaf::class, the only two types ofTree.Element<T>ourTree.kthas). Now Compose will only ever recycle an oldNodelayout for a newNode, and an oldLeaflayout for a newLeaf.Let's fix the problematic line, add the same amount of rows to the tree and check the dump once again:
And a table for comparison:
This by itself is a HUGE improvement over what we had before. This is the main fix for this PR.
Also, note that the path doesn't reference
BasicLazyTreeanymore: since the UI side is no longer leaking, the shortest path to GC defaults to the State side of Compose (the SlotTable).Instead of seeing the click listener lambda, you can now see
SelectableLazyListScopeKt$$Lambda. This is the lambda generated by theitemsIndexedblock inside theSelectableLazyListScopeDSL. TheSlotTablekeeps this lambda around because it needs to know how to build the list structure, but it only keeps the active state, not thousands of dead copies.However, you might be thinking "well, a 37% decrease in retained heap for
TestTreeNode(our test Composable) is still not ideal" and I totally agree with you! But given that this is not the main issue for this PR, I'll add what we can do to decrease the retained heap for this case even further, just check the collapsed section below!Optimizing the code even further
Take a look at the retained heap for the selected
Objectsin the last screenshot. You can see that the second row has a retained heap of 101,024 bytes and 2896 instances. The first row has a retained heap of 4,952 and 954 instances. This is a good indication that we can improve things further.Extra issue 1 (
flattenTree):There were actually two separate issues in the
flattenTreecode:1. The Stale ID Leak (Memory)
In
BasicLazyTree,TreeState(allNodesandopenNodes) would accumulate IDs from older, discarded trees indefinitely. When users provide their own objects as node IDs (via addNode(data, id = myObject)), this caused those objects to be kept alive in memory long after the tree was replaced, leading to unbounded memory growth.We fixed this with a per-flattening cleanup:
allNodesis cleared before each traversal and repopulated only with nodes still present in the active tree. Afterward,openNodesis intersected with the surviving IDs, dropping stale references while preserving expansion state for nodes that still exist.2. The
.mapBottleneck (CPU/GC Churn)Take a look at this sneaky line inside
flattenTree:Because this runs for every single node during flattening, calling
.mapcreates a brand new temporaryArrayListin memory every single iteration. For 7,000 nodes, we were allocating 7,000 lists, causing massive GC churn and possibly UI frame drops.We fixed this by changing it to
.none { it.first == id }, which performs the exact same check with zero allocations.Extra issue 2:
The problem with this function is that
filterIsInstancealways creates a brand newArrayListunder the hood to hold the filtered results. Because this function is recursive, we are allocating a new, temporary list in memory for every single node in the tree that has children. If you close a node with 1,000 descendants, you just created 1,000 temporary lists that immediately need to be garbage collected.The fix to eliminate these allocations entirely is just use a standard
forEachand check the type inside the loop. This makes the function zero-allocation.Result
Yeah, that's some pretty awesome stuff. The second selected
Objectwhen down to only 80 bytes in the heap and 12 instances, while the first row kept the retained heap of 4,952 bytes but is down to only 40 instances 🤯 With this, here's the new result forTestTreeNode:Release notes
Bug fixes
LazyTreedoes not retain references to each and every single entry in the list anymore, properly reusing the UI and not eating a insane amount of heap in the process, preventing OOM crashes and CPU bottlenecks.