38

I've started to review some code in a project and found something like this:

GC.Collect();
GC.WaitForPendingFinalizers();

Those lines usually appear on methods that are conceived to destruct the object under the rationale of increase efficiency. I've made this remarks:

  1. To call garbage collection explicitly on the destruction of every object decreases performance because doing so does not take into account if it is absolutely necessary for CLR performance.
  2. Calling those instructions in that order causes every object to be destroyed only if other objects are being finalized. Therefore, an object that could be destroyed independently has to wait for another object's destruction without a real necessity.
  3. It can generate a deadlock (see: this question)

Are 1, 2 and 3 true? Can you give some reference supporting your answers?

Although I'm almost sure about my remarks, I need to be clear in my arguments in order to explain to my team why is this a problem. That's the reason I'm asking for confirmation and reference.

TamaMcGlinn
  • 2,840
  • 23
  • 34
JPCF
  • 2,232
  • 5
  • 28
  • 50
  • 2
    Who would argue that this is good code? It clearly is inappropriate. – usr Sep 04 '12 at 14:28
  • In terms of deadlocks, that only occurs in certain circumstances. Is the code in those circumstances? If not, #3 isn't an issue. – Peter Ritchie Sep 04 '12 at 14:29
  • 2
    #1 is definitely a factory, according to the MSDN: "It is possible to force garbage collection by calling Collect, but most of the time, this should be avoided because it may create performance issues." - http://msdn.microsoft.com/en-us/library/66x5fx1b.aspx – James Michael Hare Sep 04 '12 at 14:41
  • It can be a good idea to call `GC.Collect` (but without `WaitForPendingFinalizers`), if there is a lot of data that can be freed. For example, after the user closes a document. – CodesInChaos Sep 04 '12 at 14:44
  • 1
    @CodesInChaos Perhaps, but if that memory isn't needed, there's no harm in waiting for the GC, and if it is needed, then the GC itself will fire sooner. It has been fine-tuned and optimized to cover just about all code scenarios (and in .NET 4.5 it's been tuned even more to cover newer scenarios), so this is a case where I'd rely on the wisdom of the Microsoft engineers. – MCattle Sep 04 '12 at 14:47
  • @MCattle it can help in practice if there's a brief period of **lots** of such heavy memory use and release, but it isn't normal for the lifetime of the project. I'd still measure to show that it did help, rather than assuming though. Baseline assumption should be that GC does best left alone, because that's true 99.999% of the time. – Jon Hanna Sep 05 '12 at 09:14
  • 2
    Piffle and pish... Try scaling and cropping a folder with 10 x 4MB images (down to about 150k) in a loop... if you don't manually call GC.Collect() after every iteration you WILL get an out of memory exception!... there's one example where it is not only relevant but needed... 0.001% of the time? LOL Now try it again with 50 images, getting any better? – Paul Zahra Mar 02 '16 at 14:10
  • 2
    At least on one occurence `GT.Collect(); GC.WaitForPendingFinalizers()` was necessary and sufficient to fix an error. Our program copies a template *Word* file from a network location to a local directory, uses *Word* interop to create another *Word* file from the template, closes the *Word* application calling the `Quit()` method, and deletes the template copy. We had to put a delay of four seconds between calling `Quit()` and deleting file in order to let *Word* unlock it—a crutch. Invoking `.Collect()` and `.WaitForPendingFinalizers()` after `Word.Quit()` let us remove it. – Anton Shepelev Sep 10 '20 at 17:12
  • @PaulZahra Is *.NET* not suppsed to collect garbage whever neccessary, a deficiency of RAM being an instance of such necessity? – Anton Shepelev Sep 10 '20 at 17:16
  • @Ant_222 The real issue here is can you wait for garbage collection (depending on Windows to decide which as I explained can not be good enough in niche situations), or are you using large amounts of memory which need to be refreshed quickly; if the latter then you must force garbage collection or build a slow system I guess. – Paul Zahra Sep 11 '20 at 16:49

6 Answers6

35

The short answer is: take it out. That code will almost never improve performance, or long-term memory use.

All your points are true. (It can generate a deadlock; that does not mean it always will.) Calling GC.Collect() will collect the memory of all GC generations. This does two things.

  • It collects across all generations every time - instead of what the GC will do by default, which is to only collect a generation when it is full. Typical use will see Gen0 collecting (roughly) ten times as often than Gen1, which in turn collects (roughly) ten times as often as Gen2. This code will collect all generations every time. Gen0 collection is typically sub-100ms; Gen2 can be much longer.
  • It promotes non-collectable objects to the next generation. That is, every time you force a collection and you still have a reference to some object, that object will be promoted to the subsequent generation. Typically this will happen relatively rarely, but code such as the below will force this far more often:

    void SomeMethod()
    { 
     object o1 = new Object();
     object o2 = new Object();
    
     o1.ToString();
     GC.Collect(); // this forces o2 into Gen1, because it's still referenced
     o2.ToString();
    }
    

Without a GC.Collect(), both of these items will be collected at the next opportunity. With the collection as writte, o2 will end up in Gen1 - which means an automated Gen0 collection won't release that memory.

It's also worth noting an even bigger horror: in DEBUG mode, the GC functions differently and won't reclaim any variable that is still in scope (even if it's not used later in the current method). So in DEBUG mode, the code above wouldn't even collect o1 when calling GC.Collect, and so both o1 and o2 will be promoted. This could lead to some very erratic and unexpected memory usage when debugging code. (Articles such as this highlight this behaviour.)

EDIT: Having just tested this behaviour, some real irony: if you have a method something like this:

void CleanUp(Thing someObject)
{
    someObject.TidyUp();
    someObject = null;
    GC.Collect();
    GC.WaitForPendingFinalizers(); 
}

... then it will explicitly NOT release the memory of someObject, even in RELEASE mode: it'll promote it into the next GC generation.

Dan Puzey
  • 33,626
  • 4
  • 73
  • 96
  • 3
    Makes sense that the example at the end doesn't collect. The assignment to null does nothing, so it should just be taken out, and since there's no other activity making use of the stack, the fact that the compiler can use the space belonging to a local while it's in scope doesn't matter - it could, but it had no reason to do so. – Jon Hanna Sep 05 '12 at 09:10
  • 1
    @JonHanna absolutely, it makes sense. I just made explicit example of it because the original question suggests that there's this form of "cleanup" code present. I'm thinking that a developer who calls `GC.Collect` for every object they create would also write code like my sample, which does the exact opposite of what it's supposed to. – Dan Puzey Sep 05 '12 at 11:21
  • 1
    I ran into this problem (thank you Dan) but what was strange is that assigning "someObject" to null EVEN IN THE DECLARING SCOPE -- which you would think would "unroot" it -- would prevent it from being GC'd in DEBUG mode. – Keith Bluestone Oct 25 '12 at 13:57
  • 2
    In my current project we work with large arrays of doubles (usually of size 10k), which go directly to the large object heap. From my experience, the .Net garbage collector (v.4 at least) completely didn't cope to manage this stuff, so I had to add `GC.Collect()` to stop the process using 2GB of memory and swapping stuff to disk all the time. – Grzenio Jul 25 '14 at 08:04
  • @Grzenio sounds like your hardware was just over capacity and it needs more RAM – Chris Marisic Jul 01 '15 at 17:40
  • @ChrisMarisic, buying more RAM is always an option, but there is a limit to it, i.e. my current motherboard only supports 32GB if I remember correctly.... or you can acknowledge that .Net garbage collector is not perfect, save a few hundred pounds (dollars), and have the same experience :) – Grzenio Jul 02 '15 at 07:49
  • @Grzenio my point is that it was not a fault of the GC or .NET, it was specifically your usage. By your manual manipulation of the GC you were able to sidestep the underlying problem: insufficient resources. – Chris Marisic Jul 02 '15 at 15:23
  • @ChrisMarisic, I can't really agree that there are insufficient resources for what I am doing. Because, if I do call the GC manually, there resource pool *is* sufficient. Similarly, if I implemented the thing in C++ the resource pool would also be sufficient, so the resources *are* sufficient for this algorithm on the given hardware. It is just that the GC implementation/optimisation is not adequate for my usage (which I agree, is not standard). – Grzenio Jul 03 '15 at 10:13
  • 2
    re `then it will explicitly NOT release the memory of someObject, even in RELEASE mode: it'll promote it into the next GC generation.` - attempt to test a similar thing at https://github.com/dotnet/coreclr/issues/15207 plus very informative https://github.com/dotnet/coreclr/issues/5826#issuecomment-226574611 – quetzalcoatl May 29 '18 at 10:54
8

There is a point one can make that is very easy to understand: Having GC run automatically cleans up many objects per run (say, 10000). Calling it after every destruction cleans up about one object per run.

Because GC has high overhead (needs to stop and start threads, needs to scan all objects alive) batching calls is highly preferable.

Also, what good could come out of cleaning up after every object? How could this be more efficient than batching?

usr
  • 168,620
  • 35
  • 240
  • 369
  • 4
    Excellent point, you're taking the cost hit for *every* object versus spreading it out over many objects. – James Michael Hare Sep 04 '12 at 14:42
  • 1
    +1 Part of the high overhead -- walking the entire object graph -- generally requires more time as memory use increases. If a developer adds `GC.Collect()` calls to an application because of poor performance related to high memory use, the performance may in fact *decrease*. – phoog Sep 04 '12 at 15:06
  • 1
    Subjective remark: the OP's coworkers probably call Flush, Close, Dispose on all streams and put them into a using block in addition. Collecting after every object seems equally misguided. – usr Sep 04 '12 at 15:07
7

Your point number 3 is technically correct, but can only happen if someone locks during a finaliser.

Even without this sort of call, locking inside a finaliser is even worse than what you have here.

There are a handful of times when calling GC.Collect() really does help performance.

So far I've done so 2, maybe 3 times in my career. (Or maybe about 5 or 6 times if you include those where I did it, measured the results, and then took it out again - and this is something you should always measure after doing).

In cases where you're churning through hundreds or thousands of megs of memory in a short period of time, and then switching over to much less intensive use of memory for a long period of time, it can be a massive or even vital improvement to explicitly collect. Is that what's happening here?

Anywhere else, they're at best going to make it slower and use more memory.

Jon Hanna
  • 110,372
  • 10
  • 146
  • 251
6

See my other answer here:

To GC.Collect or not?

two things can happen when you call GC.Collect() yourself: you end up spending more time doing collections (because the normal background collections will still happen in addition to your manual GC.Collect()) and you'll hang on to the memory longer (because you forced some things into a higher order generation that didn't need to go there). In other words, using GC.Collect() yourself is almost always a bad idea.

About the only time you ever want to call GC.Collect() yourself is when you have specific information about your program that is hard for the Garbage Collector to know. The canonical example is a long-running program with distinct busy and light load cycles. You may want to force a collection near the end of a period of light load, ahead of a busy cycle, to make sure resources are as free as possible for the busy cycle. But even here, you might find you do better by re-thinking how your app is built (ie, would a scheduled task work better?).

Community
  • 1
  • 1
Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
  • And this is some sort of "app-wide" scenario. In the posted scenario a single "object destruction" is referred, which means all generations are scanned and all possible finalizers in the app are awaited to clean up a single object that likely has nothing to do with them. – lgoncalves Sep 04 '12 at 14:49
6

We have run into similar problems to @Grzenio however we are working with much larger 2-dimensional arrays, in the order of 1000x1000 to 3000x3000, this is in a webservice.

Adding more memory isn't always the right answer, you have to understand your code and the use case. Without GC collecting we require 16-32gb of memory (depending on customer size). Without it we would require 32-64gb of memory and even then there are no guarantees the system won't suffer. The .NET garbage collector is not perfect.

Our webservice has an in-memory cache in the order of 5-50 million string (~80-140 characters per key/value pair depending on configuration), in addition with each client request we would construct 2 matrices one of double, one of boolean which were then passed to another service to do the work. For a 1000x1000 "matrix" (2-dimensional array) this is ~25mb, per request. The boolean would say which elements we need (based on our cache). Each cache entry represents one "cell" in the "matrix".

The cache performance dramatically degrades when the server has > 80% memory utilization due to paging.

What we found is that unless we explicitly GC the .net garbage collector would never 'cleanup' the transitory variables until we were in the 90-95% range by which point the cache performance had drastically degraded.

Since the down-stream process often took a long duration (3-900 seconds) the performance hit of a GC collection was neglible (3-10 seconds per collect). We initiated this collect after we had already returned the response to the client.

Ultimately we made the GC parameters configurable, also with .net 4.6 there are further options. Here is the .net 4.5 code we used.

if (sinceLastGC.Minutes > Service.g_GCMinutes)
{
     Service.g_LastGCTime = DateTime.Now;
     var sw = Stopwatch.StartNew();
     long memBefore = System.GC.GetTotalMemory(false);
     context.Response.Flush();
     context.ApplicationInstance.CompleteRequest();
     System.GC.Collect( Service.g_GCGeneration, Service.g_GCForced ? System.GCCollectionMode.Forced : System.GCCollectionMode.Optimized);
     System.GC.WaitForPendingFinalizers();
     long memAfter = System.GC.GetTotalMemory(true);
     var elapsed = sw.ElapsedMilliseconds;
     Log.Info(string.Format("GC starts with {0} bytes, ends with {1} bytes, GC time {2} (ms)", memBefore, memAfter, elapsed));
}

After rewriting for use with .net 4.6 we split the garbage colleciton into 2 steps - a simple collect and a compacting collect.

    public static RunGC(GCParameters param = null)
    {
        lock (GCLock)
        {
            var theParams = param ?? GCParams;
            var sw = Stopwatch.StartNew();
            var timestamp = DateTime.Now;
            long memBefore = GC.GetTotalMemory(false);
            GC.Collect(theParams.Generation, theParams.Mode, theParams.Blocking, theParams.Compacting);
            GC.WaitForPendingFinalizers();
            //GC.Collect(); // may need to collect dead objects created by the finalizers
            var elapsed = sw.ElapsedMilliseconds;
            long memAfter = GC.GetTotalMemory(true);
            Log.Info($"GC starts with {memBefore} bytes, ends with {memAfter} bytes, GC time {elapsed} (ms)");

        }
    }

    // https://msdn.microsoft.com/en-us/library/system.runtime.gcsettings.largeobjectheapcompactionmode.aspx
    public static RunCompactingGC()
    {
        lock (CompactingGCLock)
        {
            var sw = Stopwatch.StartNew();
            var timestamp = DateTime.Now;
            long memBefore = GC.GetTotalMemory(false);

            GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnce;
            GC.Collect();
            var elapsed = sw.ElapsedMilliseconds;
            long memAfter = GC.GetTotalMemory(true);
            Log.Info($"Compacting GC starts with {memBefore} bytes, ends with {memAfter} bytes, GC time {elapsed} (ms)");
        }
    }

Hope this helps someone else as we spent a lot of time researching this.

[Edit] Following up on this, we have found some additional problems with the large matrices. we have started encountering heavy memory pressure and the application suddenly being unable to allocate the arrays, even if the process/server has plenty of memory (24gb free). Upon deeper investigation we discovered that the process had standby memory that was almost 100% of the "in use memory" (24gb in use, 24gb standby, 1gb free). When the "free" memory hit 0 the application would pause for 10+ seconds while standby was reallocated as free and then it could start responding to requests.

Based on our research this appears to be due to fragmentation of the large object heap.

To address this concern we are taking 2 approaches:

  1. We are going to change to jagged array vs multi-dimensional arrays. This will reduce the amount of continuous memory required, and ideally keep more of these arrays out of the Large Object Heap.
  2. We are going to implement the arrays using the ArrayPool class.

[Edit2] Implementing the ArrayPool class required some rework of the existing code because the Array that is "rented" is not the same size as the requested e.g. if you request an array of size 12, you could get a size 20 array. So things like the "Length" property can be unpredictable. To address this we actually wrapped the ArrayPool in another class which guaranteed that you could only access the cells in the array of the length requested and was consistent in its returning of count/length properties. Doing this avoided significant redesign of the application code.

[Edit3] We later determined that the largest hit in our RunGC method is the WaitForPendingFinalizers, if you look at the fine code for this it is actually doing a blocking collect so even if you don't "force" the collect in the first GC.Collect() call, this will cause a blocking collect. Furthermore the finalizer process is single threaded so we discovered that we needed to remove this statement entirely. After doing this we found our blocking and collection time dramatically reduced. This solution may not work for everyone, especially if using COM objects, but it worked for us. If using COM objects, there are alternatives but it requires very advanced coding practices and all COM objects have to be manually released and you have to be very careful when accessing properties of COM objects to avoid "hidden" instances of COM objects.

Justin
  • 1,303
  • 15
  • 30
3

I've used this just once: to clean up server-side cache of Crystal Report documents. See my response in Crystal Reports Exception: The maximum report processing jobs limit configured by your system administrator has been reached

The WaitForPendingFinalizers was particularly helpful for me, as sometimes the objects were not being cleaned up properly. Considering the relatively slow performance of the report in a web page - any minor GC delay was negligible, and the improvement in memory management gave an overall happier server for me.

Community
  • 1
  • 1
gojimmypi
  • 426
  • 4
  • 7