Fix ResolvedTopicReference memory leak #153
Conversation
The caching mechanism in `ResolvedTopicReference` never releases references to the `ResolvedTopicReference`s it holds. This has likely always been a problem since the introduction of the cache, but is just now appearing as a memory leak when profiling SwiftDocC because we recently added copy-on-write functionality to `ResolvedTopicReference`. It’s important to release unused `ResolvedTopicReference`s because We can create potentially tens of thousands of them when compiling large documentation catalogs or loading large navigator indexes. Resolves rdar://86035019.
ethan-kusters
requested review from
d-ronnqvist,
daniel-grumberg and
franklinsch
|
@swift-ci please test |
| #endif | ||
|
|
||
| // Verify there is no pool bucket for the bundle we're about to test | ||
| XCTAssertNil(ResolvedTopicReference.sharedPool.sync({ $0[#function] })) |
There was a problem hiding this comment.
This just duplicates the below test but without the addition of holding onto the bundle and context since the below test started failing with this change.
|
This may have some negative performance impact but I haven't had a chance to run performance tests yet. |
| */ | ||
|
|
||
| /// A wrapper that provides weak ownership of a value. | ||
| struct Weak<T: AnyObject> { |
There was a problem hiding this comment.
Is there any mechanism that drains the shared pool or otherwise removes unused elements? If not, I think we'll still end up with a shared pool full of empty weak boxes.
There was a problem hiding this comment.
I was worried about that too, but the added logic in the deinit of the Storage should handle this.
The test that asserts on the nil cache pool after converting a bundle should cover this.
| } | ||
|
|
||
| topicReference = nil | ||
| XCTAssertNil(topicReference) |
There was a problem hiding this comment.
I believe this will always hold true, regardless of the changes introduced by this PR. What are we actually trying to test here? That the underlying storage is no longer being referenced?
There was a problem hiding this comment.
This is just there to add a use of topicReference so that Swift doesn't throw a warning and/or potentially optimize this all out. I'll add a comment to clarify that.
|
It would also be interesting to see whether the cache is still useful at all (performance wise), given that |
We need to use a unique bundle identifier when testing the shared cache pool. Otherwise, when running test sequentially (in the same process, unlike what what happens when tests are run in parallel), we end up with collisions in the caching and flakey tests.
|
@swift-ci please test |
|
So we do see a significant performance hit here: |
|
Yes, with this change, we get about 50% more cache misses for these references, according to logs I added locally. |
|
I assume we get increased cache misses because as soon as a topic reference goes out of scope, we can't use a cached value instead. I assume that topic references that are still in scope somewhere could be created by copying them directly with the same benefits as using the cache. Maybe the cache should just be a LRU cache? |
|
Closing this in favor of which is a lower-risk and more targeted fix for this issue. |
4 participants
Bug/issue #, if applicable: rdar://92131596
Summary
The caching mechanism in
ResolvedTopicReferencenever releases references to theResolvedTopicReferences it holds.This has likely always been a problem since the introduction of the cache, but is now appearing as a memory leak when profiling SwiftDocC because we recently added copy-on-write functionality to
ResolvedTopicReferencewith #47 .It’s important to release unused
ResolvedTopicReferences because we can create potentially tens of thousands of them when compiling large documentation catalogs or loading large navigator indexes.Testing
Profile DocC after converting a documentation catalog and confirm that there are no references to unused
ResolvedTopicReferences.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded