ConditionalWeakTable causes a memory leak if one of their values references the table #12255
Comments
|
cc: @Maoni0 |
|
You are inducing gen2 GCs, so GC doesn't participate in deciding what's live and what's dead - it's purely determined by the user code. If you use the !gcroot sos command, it should shed some light as to what's holding an object alive. |
|
Hi @Maoni0, You can reproduce the leak by running the above code as a .NET Core Console Application (VS target ".NETCoreApp 1.1") on Windows: After some seconds, the programm will crash. Thanks! |
|
In a blocking gen2 GC, GC does not participate in determining object's lifetime at all. So if a blocking gen2 does not collect something, it means that something is held alive by user code. The fact that you keep doing blocking gen2's and the object is not going away, it simply means GC is being told that it's alive. Does this make sense? I was trying to help you to find out who's holding it live. You can use !gcroot (I think gcroot does look at the dependent handles); !gchandles will show you what handles there are and what objects they hold onto. |
|
@maonis There is no user (ie non-framework) code keeping anything alive in the example above. The ConditionalWeakTable is keeping itself alive because of how it is implemented using conditional handles. @kpreisser You should be able to workaround the issue by storing the back-references to ConditionalWeakTable in another weak reference, like |
|
@jkotas by user code I just meant not GC code... |
|
Hi @Maoni0, Hi @jkotas, Thanks! |
BTW: The Clear method was added as public API in .NET Core 2.0. |
|
@kpreisser I don't see this as a bug. I think it works exactly as expected. The whole purpose of The purpose of What you are doing - you are basically relying on the fact that when |
|
Hi @Torvin, However, I'm not sure if I follow: If it is expected that the value (right part) is tied to the life time of the key (left part), then I would have expected that the leak would also occur even if the value doesn't reference the However, if you change the line in the above code: table.Add(key, new Tuple<object, byte[]>(table, new byte[1000000]));to table.Add(key, new Tuple<object, byte[]>(null, new byte[1000000]));(so that the value doesn't contain a reference to the table), then the leak doesn't occur, meaning that the values are garbage-collected even though the key is still alive. Additionally, from reading the documentation of Note, that ECMAScript (JavaScript) defines a
However, if run the same test in ECMAScript implementations like SpiderMonkey (Mozilla Firefox) and V8 (Google Chrome, also used in Node.js), a leak doesn't occur (except for Chakra (Microsoft Edge) which funnily seems to have exactly the same behavior as in .NET where a leak only occurs if the value has a reference to the let key = new Object();
while (true) {
let map = new WeakMap();
map.set(key, [map, new ArrayBuffer(1000000)]);
}Now, because the Thanks! |
|
when you do this: table.Add(key, new Tuple<object, byte[]>(table, new byte[1000000]));a GC handle of the dependent type is created with its primary value as key and secondary value as the tuple obj. and this GC handle needs to be cleaned up by something - in the CWT case it's either by the finalizer or setting the primary of this handle to null (if you remove an entry). if you don't do either it'll stay and hold the secondary object live which means table will be alive. key can still be alive but if the handle is freed or if it's cleared as the primary for the GC handle it will not hold the secondary (tuple obj) alive. |
What I'm saying is - this behaviour (when collection of CWT also removes the GC handles) isn't documented and if I were you I wouldn't rely on it. |
|
Hi @Torvin (sorry for the late reply), OK, so this would mean that I need to clear the Otherwise, I don't think it is unreasonable to expect that GCing the table will also GC the handles since this is an implementation detail and the documentation doesn't indicate that special handling is required. Thanks! |
|
@jkotas @Maoni0 we could fix this in relatively simple way by creating a new struct to replace CWT usage of |
I do not think you would be able to maintain the CWT contract with this structure. It would be keeping the |
|
Ops, you're right: while the CWT is not collected values will still be alive. Since it is a very particular case it seems better to document the behavior and close the issue. |
|
I think it makes sense for this code to leak memory under the following view: CWT is meant to act as if it attached additional fields to existing objects. So |
|
I wonder if a memory leak of a UWP app is related to this. Whenever I open and close a page of a production app, ConditionalWeakTable count increases by 1k+. This is very consistent. I cannot find any instance of the page class in memory snapshots after it is closed, so I assume the page is disposed correctly. I created a repo with a few blank pages. Navigating among the pages keeps increasing ConditionalWeakTable count in memory snapshots. |
|
Hi all, |
|
Meet the same problem of @zipswich ,but have no idea to fix it. |
_corInfoImpls elements keeps _compilation reference which keeps reference to whole table _corInfoImpls. The circular referene together with issue dotnet#12255 potentionally leads to memory leak in crossgen2 if that table is created more than once. Dispose() method clears the table and prevents memory leak. Signed-off-by: Timur Mustafin <t.mustafin@partner.samsung.com>
* [crossgen2] Implement Dispose in Compilation. _corInfoImpls elements keeps _compilation reference which keeps reference to whole table _corInfoImpls. The circular referene together with issue #12255 potentionally leads to memory leak in crossgen2 if that table is created more than once. Dispose() method clears the table and prevents memory leak. Signed-off-by: Timur Mustafin <t.mustafin@partner.samsung.com> * Update src/coreclr/tools/aot/crossgen2/Program.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
|
@jkotas Could you let us know the status of this issue please? Is it blocked by some other work, or just of low priority? In terms of implementation, I wonder whether it would solve the problem if we change the I came into this issue when I was trying to implement the weak table of Lua on top of CLR. I think it's basically the same as the JS weak table problem mentioned above. Currently there is really no existing tool for us to do that. If there is, please let me know. Note that making the reference to the table a weakref won't solve this issue in general. Thanks! |
|
@acaly I agree that there is no way to implement full fidelity equivalent of JavaScript and Lua weak table in .NET.
Yes, it would work but it looks expensive as you have pointed out. Is it how the weak table is implemented in JavaScript GCs? |
Sorry, I am not familiar with the internals of JavaScript, but obviously they also had a difficult time when deciding how WeakMap should be implemented in JavaScript. I saw some people saying that the standard intentionally allows the implementation of WeakMap to directly associate the value to the key's storage (inverted mapping) (StackOverflow comment). There is an issue in ECMAScript spec on GitHub: tc39/ecma262#1657 (comment), which I would like quote here:
So unlike the current CWT in .NET, they do have an additional obligation (or expectation) that requires the table to be collected somehow when only the value references the table. Today .NET already has DependentHandle, which I think should be more close to the full ephemeron collection method mentioned in the comment (compared to the inverted mapping method that store a reference to the value object inside an internal list of the key object that could be directly discovered by GC). The only problem is this handle does not depend on the liveness of the table. |
|
Hmmm, there do not seem to be good obvious ways to implement it. I do not think that it would be acceptable to make dependent handle 50% more expensive for a rare scenario. To answer your original question, I expect that this will remain unresolved unless somebody comes up with a better way to implement it. |
|
@jkotas Thanks for clarifying that. If making the .NET implementation of CWT to release the circular-referenced tables is too hard, I am thinking again whether it is really impossible to make another implementation of CWT to do that, no matter how slow it is. In any case, it is really annoying for me if I cannot fully obey the specification of Lua when making a Lua interpreter. I spent whole day on this, and it seems that it is possible. Please see this gist: https://gist.github.com/acaly/380fb7ee48998983384ff10107a40e78. Here the key value pair is not kept alive by anyone from GC root, but from a short-lived object that recreates itself to resurrect the pair again and again, in every collection. It seems ugly but it works... It should work even if someone resurrects the key or the table in another finalizer object. Could you please have a brief look at the code to help me check whether there is any problem that I missed about the "correctness"? Specifically, could you explain:
Thank you so much! EDIT Seems I cannot use strong handle, as it will completely prevent the load from being collected. |
|
Seems that the code in my previous comment does not work at all. The weak handle (tracking resurrection) to the I think the |
|
Ouch. The problem here is that DependentHandles are weaker than a proper ephemeron. In a proper ephemeron implementation, the stuff in its value pointer cannot be the only thing keeping the ephemeron strongly reachable. The value pointer of an ephemeron is (strongly) reachable only if both the object pointed to by the key pointer and the ephemeron itself are strongly reachable. The ephemeron GC algorithm for finding strongly referenced objects is:
At this point you have traced out all strongly referenced objects. [Footnote 1] The above algorithm is quite similar to what DependentHandle does, except DependentHandle always scans the full list of DependentHandles each time in step 3, rather than just the ones previously found to be reachable. This is unavoidable, since these handles don't require a managed reification, so you can't track the reachability of the handle itself. A DependentHandle variation where two objects need to be referenced for the value to be considered referenced is at least as powerful as an ephemeron, and in some ways slightly more general (as the second object need not be a managed reification of the handle, allowing the handle to remain a struct for things like ConditionalWeakTable where the handle remains internal). And we probably do want to have either proper ephemeron or a doubly dependant handle if we ever expose this to user code (like is requested in #30759). The people asking for that almost certainly want to create custom weak keyed mappings, and would surely not be expecting that values that point back at the collection would keep the collection alive if any of the keys were still around. Footnote: |
Definitely not the main point for me, the point is that you don't want the overhead of a collection/mapping when you want to store a single reference directly in an object. It probably would suffer from the same flaws if the containing object is reachable from the value? Considering |
|
Fair enough, I should have said that many of those who want that feature likely want to create custom collections. And yeah, with the current Dependent Handle implementation, if the key is still reachable, everything on the value side is reachable, even if the object that owns the DependentHandle is only reachable through the value side. But I think I was mistaken. It turns out that the dependent handle is logically equally as powerful as an ephemeron, because you can construct one from such handles. But doing that is definitely not as efficient as a native doubly dependent handle. I will post more details this evening. This is definitely not an approach the team would really want to implement, but it is possible. |
Did you mean that without introducing a double dependent handle, the current DependentHandle alone can be used to implement a ephemeron? I can't think of how that can be done. |
|
Edit: This whole post appears to be incorrect Click to see the old content> > It turns out that the conditional handle is logically equally as powerful as an ephemeron, because you can construct one from such handles. > > Did you mean that without introducing a double dependent handle, the current DependentHandle alone can be used to implement a ephemeron? I can't think of how that can be done.I started off by trying to prove that a double dependent handle could be implemented with ephemerons. (To prove they are equally powerful, since going the other direction is easy enough.) It did not take me long to realize this is not hard. Just chain them. Have the value pointer of one ephemeron point to another. As long as there are no other references to that second ephemeron, the value of the second one is dependent on the keys of both. But then I realized that technically that also works with DependentHandle. Which means it is possible to implement a double dependent handle, and thus implement an ephemeron. So I've put together some code to both reproduce the memory leak from the first post using a single DependentHandle, and then show how it goes away if implemented with an ephemeron. To prove that ephemerons can be implemented with DependentHandle, let's start by recreating the memory leak from the parent issue using a class that is one of the simplest possible wrappers for a Dependent Handle. We start off by creating a version of the DependentHandle struct that is not internal, using private reflection to access the ECalls. See the gist linked at the end to see the source for this part, but it extremely straightforward. Now let us create a really simple wrapper class around DependentHandle: public static class DependentHandleExtensions
{
public static object? GetSecondary(this DependentHandle handle)
{
handle.GetPrimaryAndSecondary(out var secondary);
return secondary;
}
}
public sealed class DependentHandleWrapper : IDisposable
{
private DependentHandle handle;
public DependentHandleWrapper(object key, object? value) =>
handle = new DependentHandle(key, value);
public object? Key => handle.GetPrimary();
public object? Value
{
get => handle.GetSecondary();
set => handle.SetSecondary(value);
}
~DependentHandleWrapper() =>
handle.Free();
public void Dispose()
{
handle.Free();
GC.SuppressFinalize(this);
}
}Finally, we can use this to verify the memory leak. This is basically the code from the first post with slight modifications to account for using the Wrapper Class instead of CWT. I've also hange the loop to just a limited number of iterations, and have it print memory usage after each iteration. void Main()
{
object key = new object();
for (int i = 0; i < 100; i++)
{
var handle = new DependentHandleWrapper(key, null);
handle.Value = new Tuple<object, byte[]>(handle, new byte[10000000]);
GC.Collect();
Console.WriteLine(GC.GetTotalMemory(forceFullCollection: true));
}
}Running this, and it is pretty clear we are leaking memory. Change the tuple to have null instead of referening the handle wrapper, and voila, the memory goes flat. So we clearly have the same behavior occuring. So let's implment a proper emphemeron class, by chaining two dependent handles. The first handle has the real key, and points to the owner of the second handle. The second handle has the owner of the first handle in its key. This results in needing to reach both the key, and the owner of the first handle in order to reach the value. The implementation is not very complex: public sealed class Ephemeron : IDisposable
{
private DependentHandle handle;
public Ephemeron(object key, object? value) =>
handle = new DependentHandle(key, new DependentHandleWrapper(this, value));
private DependentHandleWrapper? Secondary => (DependentHandleWrapper?)handle.GetSecondary();
public object? Key => handle.GetPrimary();
public object? Value
{
get => Secondary?.Value;
set
{
var secondary = Secondary;
if (secondary != null)
secondary.Value = value;
}
}
~Ephemeron() =>
handle.Free();
public void Dispose()
{
Secondary?.Dispose();
handle.Free();
GC.SuppressFinalize(this);
}
}Changing the main method to use this new class instead, and memory stays flat. (You can also tweak the finalizers to write to the console to prove they actually run when you expect, etc.) See this gist for the full source. (Note: I was running this in LINQPad, so I'm probably missing some using directives). I really doubt that the team would actually want to use this sort of technique for CWT. The needing two handles, and an extra allocation for the owner of the second handle will hurt both memory and performance. But unless I am totally missing something it would work. |
This comment has been minimized.
This comment has been minimized.
|
@KevinCathcart I doubt it would work as a correct weak table. If the table is alive and the value directly references the key, then the key would be reachable and won't be collected. |
|
Ouch, I think you are right. This just reverses the problem. I guess the dependent handle really is weaker than a double dependent handle. I was so focused on the reverse case, that I forgot about making sure the forward case still worked. |
|
Edit: As @acaly pointed out, this results in incorrect behavior too. Click to see the old contentInspired by the double dependency ideas in comments, I created a Some basic testing shows that this resolves the memory leak. The new I hope that the code demonstrating the concept is useful, but please note that I have not tested it extensively. |
|
@kornelpal I believe you made the same mistake above: the value will keep key alive, making it no longer a weak dictionary. I think this really cannot be done with |
Hi,
I found an odd behavior of
System.Runtime.CompilerServices.ConditionalWeakTable<TKey, TValue>in both .NET Core and .NET Framework which looks like a bug to me: If you create multiple instances of theConditionalWeakTableand store a key-value pairs in them, where the key stays alive and the value contains a reference to theConditionalWeakTable, the values are not garbage-collected after they (and theConditionalWeakTables) are no longer referenced.For example, create a .NET Core Console application with the following code:
Expected behavior: The memory consumption of the program should stay in the same area, because when a new
ConditionalWeakTableinstance is created, there are no more references to the previousConditionalWeakTableand itsTuplevalue, so they should be able to be reclaimed by the Garbage Collector.Actual behavior: The memory consumption rises rapidly (4 GB after some seconds) until an
OutOfMemoryExceptionis thrown, as the byte arrays are not reclaimed by the garbage collector.However, if you remove the reference to the table by replacing
table.Add(...)withtable.Add(key, new Tuple<object, byte[]>(null, new byte[1000000])), the problem disappears.If the algorithm cannot be implemented such that it can detect that there are no more references to the table and its values, I think the
ConditionalWeakTableshould implement aDispose()method that allows to clear all key-value-pairs.The behavior is the same for .NET Core (.NETCoreAPP 1.1) and .NET Framework 4.6.2.
Thanks!
The text was updated successfully, but these errors were encountered: