[RESOLVED] Memory Leak Question

[steve65]

Hi all, I have the following code in a class that I am using to sort large text files in C#. The code works great except that I have a memory leak. I am assuming that this is the outputArray in the SortTextFile method causing the problem. From what I can tell there is no erase like function for C#. Is there a way to make sure that outputArray is disposed of?

Thanks Steve

public string[] FileToStringArray(string filePath)
{
	ArrayList fileArrayList = new ArrayList();
	string inputLine;
	string[] outputArray;
	StreamReader sr = new StreamReader(filePath);

	inputLine = sr.ReadLine();

	while(inputLine != null)
	{
		fileArrayList.Add(inputLine);
		inputLine = sr.ReadLine();
	}
	sr.Close();
	outputArray = new string[fileArrayList.Count];
	fileArrayList.CopyTo(outputArray);

	return outputArray;
}

public void StringArrayToFile(string[] inputArray, string filePath)
{
	StreamWriter sw = new StreamWriter(filePath);

		foreach (string inputLine in inputArray)
		{
			sw.WriteLine(inputLine);
		}
		sw.Close();
}

public void SortTextFile(string inputFilePath, string outputFilePath)
{
	string[] outputArray = this.FileToStringArray(inputFilePath);
	Array.Sort(outputArray);
	this.StringArrayToFile(outputArray, outputFilePath);
}

[benmartin101]

try gc.collect(); its in the System.Runtime.Interop namespace.

[Kasracer]

Hi all, I have the following code in a class that I am using to sort large text files in C#. The code works great except that I have a memory leak. I am assuming that this is the outputArray in the SortTextFile method causing the problem. From what I can tell there is no erase like function for C#. Is there a way to make sure that outputArray is disposed of?

Your code is quite inefficient, which is why you're having problems.

You can call GC.Collect() as suggested, however; it is not guarenteed to even run when you call it.

Here are my suggestions:

public string[] FileToStringArray(string filePath)
{
	ArrayList fileArrayList = new ArrayList();
	string inputLine;
	string[] outputArray;
	StreamReader sr = new StreamReader(filePath);

	inputLine = sr.ReadLine();

	while(inputLine != null)
	{
		fileArrayList.Add(inputLine);
		inputLine = sr.ReadLine();
	}
	sr.Close();
	outputArray = new string[fileArrayList.Count];
	fileArrayList.CopyTo(outputArray);

	return outputArray;
}

First, put your StreamReader in a using statement. This will ensure the stream will be closed and disposed of properly when it's out of scope. As it is now, you're only closing it and not disposing of it. With the using statement, you don't even need to call the close()or dispose() methods as it'll automatically do it.

using (StreamReader sin = new StreamReader(filepath)){
//sin only exists in this scope. No need to call close() or dispose()
}

Also, why so many arrays? First, your original ArrayList is fine. No need to create yet another array to copy its elements to it, just return fileArrayList.ToArray().

Also, instead of returning an Array, it may be more efficient to put use a reference to an ArrayList in the function's parameters. Then populate that. Saves a lot of copying.

public void StringArrayToFile(string[] inputArray, string filePath)
{
	StreamWriter sw = new StreamWriter(filePath);

		foreach (string inputLine in inputArray)
		{
			sw.WriteLine(inputLine);
		}
		sw.Close();
}

Again, use a using statement for the StreamWriter. Like I mentioned before, I think Close() only closes the stream and does not dispose of it.

Also, you're copying the original array and passing the entire thing into this function. If the array huge, this will be a major bottle neck. Again, might want to pass it in by reference especially since you're only writing the data

[conipto]

I think your memory leak comes from this:

fileArrayList.CopyTo(outputArray);

Because arralists can also contain reference types, it's possible the compiler doesn't know to destory that arraylist. Aside from that, your first function could be GREATLY simplified like this:

public string[] FileToStringArray(string filePath)
{
    string[] outputArray;
    StreamReader sr = new StreamReader(filePath);
    outputArray = sr.ReadToEnd().Split(Environment.NewLine);
    sr.Close();
    return outputArray;
}

Bill

[steve65]

Thanks for the replies, as you can tell I am new to .NET

When should a using statement be used, for all objects in a class? From what I read so far I thought it was bad programming to call GC.Collect. I should let the GC collect on its own.

I will take a look at the other comments.

Steve

[Kasracer]

public string[] FileToStringArray(string filePath)
{
    string[] outputArray;
    StreamReader sr = new StreamReader(filePath);
    outputArray = sr.ReadToEnd().Split(Environment.NewLine);
    sr.Close();
    return outputArray;
}

Bill

Did you miss my rants about the using statement? ALL C# programmers should be utilizing it. You also do not dispose of your StreamReader.

When should a using statement be used, for all objects in a class? From what I read so far I thought it was bad programming to call GC.Collect. I should let the GC collect on its own.

The using statement will only work on classes that are derived from the IDisposable class. Having said that, if it's possible, you should always use the using statement on classes with an IDisposable base. That way you can let it do the cleaning up for you and it makes your application more memory efficient.

Also, calling GC.Collect() is a bit of a bad practice. Since the GC may or may not collect when you call the collect() method, there really is no point in calling it anyway.

[benmartin101]

to completely get rid of the reference of the streamreader, you could:

strmrdr.Close();
strmrdr = null;

now the garbage collector should do its thing because strmrdr is no longer referenced.

[Kasracer]

to completely get rid of the reference of the streamreader, you could:

strmrdr.Close();
strmrdr = null;

now the garbage collector should do its thing because strmrdr is no longer referenced.

Why wouldyou do that though? Since it's base is IDisposable, you don't have to set it to null, call Close() or Dispose(). You use it and then it's gone. Setting it to null also doesn't mean it'll automatically get cleaned up as quickly as the using statement or calling Dispose() would.

[jmcilhinney]

Please specify your version in future. Are you using C# 2.0? If so it's as easy as this:

string filePath = @"file path here";
string[] lines = System.IO.File.ReadAllLines(filePath);

Array.Sort(lines);

System.IO.File.WriteAllLines(filePath, lines);

If not it's still easy:

string filePath = @"file path here";
string[] lines;

using (System.IO.StreamReader sr = new System.IO.StreamReader(filePath))
{
    lines = System.Text.RegularExpressions.Regex.Split(sr.ReadToEnd(), Environment.NewLine);
    sr.Close();
}

Array.Sort(lines);

string sortedLines = string.Join(Environment.NewLine, lines);

using (System.IO.StreamWriter sw = new System.IO.StreamWriter(filePath))
{
    sw.Write(sortedLines);
    sw.Close();
}

BTW kas, IDisposable is an interface that is implemented, not a class that's inherited. Also, the 'using' block will implicitly call the Dispose method. It is still good practice, although not strictly necessary, to call the Close method.

[steve65]

The using statement will only work on classes that are derived from the IDisposable class. Having said that, if it's possible, you should always use the using statement on classes with an IDisposable base. That way you can let it do the cleaning up for you and it makes your application more memory efficient.

Should I implemented IDisposable is an interface to all of the classes I create?

[steve65]

Please specify your version in future.

C# 2003. Will the code I am writing now port directly to 2.0 or will I have to convert it again?

string[/color] sortedLines = string.Join(Environment.NewLine, lines);

using (System.IO.StreamWriter sw = new System.IO.StreamWriter(filePath))
{
    sw.Write(sortedLines);
    sw.Close();
}

I was not sure why you went with using the above statement as compared to keeping it an array and cycle through members using sw.WriteLine. I am assuming your way is better for performance bad for memory since you are copying the entire array to sortedLines.

[jmcilhinney]

You should the IDisposable interface if and only if your class holds unmanaged (non-.NET) resources or it has member variables that implement IDisposable and thus need to be Disposed when your object is. Note that if your class inherits a class that implements IDisposable then your class implicitly implements it too, thus you can simply override the Dispose method of the base class.

The C# 1.1 code I provided was written in C# 2.0. It will work fine when upgraded. It's just not as simple as it could be.

[steve65]

You should the IDisposable interface if and only if your class holds unmanaged (non-.NET) resources or it has member variables that implement IDisposable and thus need to be Disposed when your object is.

How do I know I am using unmanaged resources or variables that implement IDisposable? Implicitly noting what I have when I code, experience or both? I know it sounds like a stupid question but as a new .net user I am still not sure what is what when I use it.

[wossname]

On a side note, there is no such thing as a memory leak in .NET Its just a bug you've got.

[steve65]

On a side note, there is no such thing as a memory leak in .NET Its just a bug you've got.

Then bugs I have. I am either taking a hammer to the bugs or me.

[steve65]

I really appreciate all of the time you have given me. I thought I had this down but I am still doing something wrong. I could have written the code without the classes but I wanted to makes sure that I understood how to pass references and not values in C#.

The application is still holding onto the memory even through I have left the class that was using all of the memory. I have tried setting fileSorter = null, fileArray = null but that did not work either.

using System;

namespace ConsoleApplication1
{
	class Class1
	{
		[STAThread]
		static void Main(string[] args)
		{
			// Mem in use by ConsoleApplication1: 6,320K			
			FileSorter fileSorter = new FileSorter();
			fileSorter.SortFile();

			// Mem in use by ConsoleApplication1: 564,176K
		}
	}
	public class FileSorter
	{
		public void SortFile()
		{
			string[] fileArray = null;
			FileHandler fileHandler = new FileHandler();
			StringHandler stringHandler = new StringHandler();
			
			// Mem in use by ConsoleApplication1: 6,408K			
			fileHandler.FileToArray(ref fileArray, @"c:\tmp\Really Big File.txt");

			// Mem in use by ConsoleApplication1: 561,084K
			// File being sorted is only 114,437K
			// I expected a memory usage of about 120,845K
			stringHandler.SortArray(ref fileArray);

			// Mem in use by ConsoleApplication1: 561,136K
			fileHandler.ArrayToFile(ref fileArray, @"c:\tmp\Really Big File Sorted.txt");

			// The file has now been sorted.  How do I dispose of fileArray properly before I leave this method?
		}
	}
	
	public class FileHandler
	{
		public void FileToArray(ref string[] outputArray, string filePath)
		{
			using (System.IO.StreamReader sr = new System.IO.StreamReader(filePath))
			{
				outputArray = System.Text.RegularExpressions.Regex.Split(sr.ReadToEnd(), Environment.NewLine);
				sr.Close();
			}
		}

		public void ArrayToFile(ref string[] inputArray, string filePath)
		{
			using (System.IO.StreamWriter sw = new System.IO.StreamWriter(filePath))
			{
				foreach (string inputLine in inputArray)
				{
					sw.WriteLine(inputLine);
				}
				sw.Close();
			}
		}
	}

	public class StringHandler
	{
		public void SortArray(ref string[] inputArray)
		{
			Array.Sort(inputArray);
		}
	}
}

[conipto]

I'm not sure what the point of having them in classes is, since nothing there really needs to be instantiated. I can tell you that ReadToEnd() creates a StringBuilder as it reads the file, and I have little experience with them and how much memory they take. I believe they are supposed to be even more efficient than concatenating, so I'm not sure where your problem is coming from.

Bill

[conipto]

Did you miss my rants about the using statement? ALL C# programmers should be utilizing it.

First off, look at the post times, I was typing mine while you replied

Secondly, I think "using" is a matter of personal preference. It causes more execution and the extra declaration of a boolean variable internally when it is used. For example, here are two small functions and their equivalent IL code.

//Function with Using
        private void SWWithUsing()
        {
            using (System.IO.StreamReader SR = new System.IO.StreamReader("File.Txt") )
            {
                SR.ReadToEnd();
            }
        }

//THe IL for that code.
.method private hidebysig instance void SWWithUsing() cil managed
{
      .maxstack 2
      .locals init (
            [0] [mscorlib]System.IO.StreamReader reader1,
            [1] bool flag1)
      L_0000: nop 
      L_0001: ldstr "File.Txt"
      L_0006: newobj instance void [mscorlib]System.IO.StreamReader::.ctor(string)
      L_000b: stloc.0 
      L_000c: nop 
      L_000d: ldloc.0 
      L_000e: callvirt instance string [mscorlib]System.IO.TextReader::ReadToEnd()
      L_0013: pop 
      L_0014: nop 
      L_0015: leave.s L_0027
      L_0017: ldloc.0 
      L_0018: ldnull 
      L_0019: ceq 
      L_001b: stloc.1 
      L_001c: ldloc.1 
      L_001d: brtrue.s L_0026
      L_001f: ldloc.0 
      L_0020: callvirt instance void [mscorlib]System.IDisposable::Dispose()
      L_0025: nop 
      L_0026: endfinally 
      L_0027: nop 
      L_0028: ret 
      .try L_000c to L_0017 finally handler L_0017 to L_0027
}

And here is the equivalent without a using block:

//Without Using
        private void SWWithoutUsing()
        {
            System.IO.StreamReader SR = new System.IO.StreamReader("File.Txt");
            SR.ReadToEnd();
            SR.Close();
            SR.Dispose();
        }


//And the IL

.method private hidebysig instance void SWWithoutUsing() cil managed
{
      .maxstack 2
      .locals init (
            [0] [mscorlib]System.IO.StreamReader reader1)
      L_0000: nop 
      L_0001: ldstr "File.Txt"
      L_0006: newobj instance void [mscorlib]System.IO.StreamReader::.ctor(string)
      L_000b: stloc.0 
      L_000c: ldloc.0 
      L_000d: callvirt instance string [mscorlib]System.IO.TextReader::ReadToEnd()
      L_0012: pop 
      L_0013: ldloc.0 
      L_0014: callvirt instance void [mscorlib]System.IO.TextReader::Close()
      L_0019: nop 
      L_001a: ldloc.0 
      L_001b: callvirt instance void [mscorlib]System.IO.TextReader::Dispose()
      L_0020: nop 
      L_0021: ret 
}

Without "Using" it's a good bit shorter, does not use a .try and does not declare the extra boolean in the beginning. Now, whether this means any speed increase or not, I can't tell you, and if it does, I'm sure it's infinitesmal. Still, I think it's up to the individual coder and their style as to whether or not to use "Using". Besides that, Why should it be so necessary in C# to make entensive use of it, and I never hear it even mentioned in the VB.NET forum?

Using sr As IO.StreamReader = New IO.StreamReader("Bill.TExt")
        sr.ReadToEnd()
End Using

Bill

[jmcilhinney]

The advantage of the 'using' statement, besides the shorter code, is specifically that it DOES involve an implicit exception handler. This guarantees that even if an exception is thrown the object will still be disposed. Without a 'using' statement you should have an explicit exception handler for something like file I/O, where an exception could well occur and leave the file open.

Also, Using is new to VB 2005 so many VB.NET developers can't use it and many who can don't know it exists. I've used Using in a number of my VB.NET posts and I higly recommend it. C# has had the 'using' statement from the beginning, so C# developers are more aware of its existence.

[Kasracer]

I think "using" is a matter of personal preference. It causes more execution and the extra declaration of a boolean variable internally when it is used.

Personal preference? So you prefer to create your Streams, then explicitly close and dispose them, even if an error happens? Why?

A simple using statement will implicity do what you should explicitly do if you're not using it. It creates less code and is more readable.

The MSIL you posted doesn't matter as you did not impliment the features the using statement implicity adds (and you should add those anyway).

Besides that, Why should it be so necessary in C# to make entensive use of it, and I never hear it even mentioned in the VB.NET forum?

It cleans up the programmer's mess. I highly doubt any programmer handles every IDisposable object by making sure it's Disposed of when if it executes normally and if it throws an exception. It also will close your streams and it's much more readable.

Also, like jmcil says, it's brand new in VB.Net and many in the VB.Net forum tell users about it if it applies to their question.

[steve65]

Well, I have to say I am thoroughly lost, but as long as you all are having fun

I still would love to know what I am doing wrong. I admit this is a very simplistic program made more complex than needed. I did it this way so I could get a better understanding of how the objects are passed from one class to another using references and once a class is vacated how memory would be freed up, or so I thought

Minus the additional classes, that are redundant I know, programmatically did I still miss the boat on something? Thanks for any input you can provide.

[jmcilhinney]

On what are you basing the assumption that the app is using too much memory? Are you just looking at the Task Manager? That is not an accurate way to determine exactly what the resource footprint of your app is. The Framework looks after memory management in .NET apps. If memory is required then the Framework will reclaim it. There are really only a few simple rules that you need to follow to make sure that your app is doing its part and the rest you can leave to the Framework. If you want to gain an understanding of memory and resource management in .NET apps then I suggest that you search MSDN for the relevant terms. Here are a couple of articles to start with.

http://msdn.microsoft.com/msdnmag/is...I/default.aspx
http://msdn.microsoft.com/msdnmag/is...2/default.aspx

[steve65]

Thanks I will look at the articles. This discussion has helped me a lot, albeit a pain in the backside. Yes, all of my memory readings are from the task manager. I did find out that the framework keeps a working set of memory, which may be noted in your articles. In addition, even though my app is not using memory .net still holds onto it in memory. This article was helpful.

This much simplified code demonstrates the fact, that prior to my program removing any working sets it is using 564MB of memory. After I reduce the working sets to zero the memory used is 1.3MB, again all reported from Task Manager.

class Class1
{
	[STAThread]

	[DllImport("kernel32")]
	static extern bool SetProcessWorkingSetSize(IntPtr handle, int minSize, int maxSize);

	static void Main(string[] args)
	{
		Class1 tempclass = new Class1();
		tempclass.SortFile(@"c:\tmp\Really Big File.txt", @"c:\tmp\Really Big File Sorted.txt");
		tempclass = null;

		SetProcessWorkingSetSize(Process.GetCurrentProcess().Handle, -1, -1);
	}

	private void SortFile(string inputFilePath, string outputFilePath)
	{
		string[] fileArray = null;

		using (System.IO.StreamReader sr = new System.IO.StreamReader(inputFilePath))
		{
			fileArray = System.Text.RegularExpressions.Regex.Split(sr.ReadToEnd(), Environment.NewLine);
			sr.Close();
		}

		Array.Sort(fileArray);

		using (System.IO.StreamWriter sw = new System.IO.StreamWriter(outputFilePath))
		{
			foreach (string inputLine in fileArray)
			{
				sw.WriteLine(inputLine);
			}
			sw.Close();
		}

		fileArray = null;
	}
}

[alexandros]

try gc.collect(); its in the System.Runtime.Interop namespace.

Is there a similar thing for the 1.1 Framework ?

[jmcilhinney]

The garbage collector is an inherent part of the Framework and has been there from the beginning. The GC class in in the System namespace (not System.Runtime.Interop, which I don't think actually exists) so you can call GC.Collect anywhere, any time. Not that you should mind you.