CODEWITHSUNDEEP
c#threadpool
Nitin
Nitin

Reputation: 388

ThreadPool uses excessive amounts of memory in just a few seconds

I have made a simple console application for printing prime numbers. I am using ThreadPool for the function which checks whether a number is prime or not.

In the task manager , this program starts to take too much memory ( 1 GB in a few seconds ) How do I improve that if I have to still use ThreadPool?

Here is the code which I wrote 

class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine(2);
        Console.WriteLine(3);
        Console.WriteLine(5);
        Console.WriteLine(7);
        Console.WriteLine(11);
        Console.WriteLine(13);
        Console.WriteLine(17);
        for (long i = 19; i < Int64.MaxValue; i = i+2)
        {
            if(i % 3 == 0 || i % 5 == 0 || i % 7 == 0 || i % 11 == 0 || i % 13 == 0 || i % 17 == 0 )
                continue;

            ThreadPool.QueueUserWorkItem(CheckForPrime, i);
        }
        Console.Read();
    }

    private static void CheckForPrime(object i)
    {
        var i1 = i as long?;
        var val =  Math.Sqrt(i1.Value);
        for (long j = 19; j <= val; j = j + 2)
        {
            if (i1 % j == 0) return;
        }
        Console.WriteLine(i1);

    }
}

Upvotes: 1

Views: 3480

Answers (3)

Cody Gray
Cody Gray

Reputation: 244692

To put it rather bluntly, you're doing multi-threading wrong. Threads are a powerful tool when used correctly, but like all tools, they're not the correct solution in every case. A glass bottle works well for holding beer, but not so great for hammering nails.

In the general case, creating more threads is not going to make things run any faster, and that is particularly true here as you've discovered. The code you've written queues up a new thread each iteration through your loop, and each of those threads will allocate a stack. Since the default size for a stack in the .NET world is 1 MB, it doesn't take very long for your memory commitment to skyrocket. It therefore comes as no particular surprise that you're exceeding 1 GB. Eventually, you'll run into a hard memory limit and get an OutOfMemoryException thrown at you. And memory is just the most obvious of the resources that your design is quickly starving your system for. Unless your system resources can grow exponentially with your thread pool, you're not going to experience any performance advantages.

Adil suggests inserting a call to Thread.Sleep to give the new thread you create time to run before continuing through the loop (and creating additional threads). As I mentioned in a comment, although this "works", it seems like an awfully ugly hack to me. But it's hard for me to suggest a better solution because the real problem is the design. You say that you have to use a thread pool, but you don't say why that is the case.

If you absolutely have to use a thread pool, the best workaround is probably to set an arbitrary limit on the size of the thread pool (i.e., how many new threads it can spawn), which is accomplished by calling the SetMaxThreads method. This feels at least a little less hacky to me than Thread.Sleep.

Note: If you decide to pursue the SetMaxThreads approach, you should be aware that you cannot set the maximum to less than the minimum. The default value for the minimum is the number of CPU cores, so if you have a dual-core processor, you can't set the maximum to 1 without first lowering the minimum.

Finally, although it doesn't really change the answer in this case, it is worth noting that Task Manager is not a memory profiler. Relying on it as if it were one will frequently get you bad (or at least very misleading) data.

Edit: After further thought, it occurs to me that the problem really lies not with exponential execution but with exponential querying. The maximum number of allowed threads is probably irrelevant because the code will still queue 'em up faster than they can ever hope to be processed. So never mind about limiting the size. You probably want to go with Joachim's solution involving the creation of a semaphore, or the implicit suggestion that everyone has made of not using a thread pool.

Upvotes: 4

Joachim Isaksson
Joachim Isaksson

Reputation: 180867

Simplest way to fix your code, just limit the work queue using a semaphore;

class Program
{
    // Max 100 items in queue
    private static readonly Semaphore WorkLimiter = new Semaphore(100, 100);

    static void Main(string[] args)
    {
        Console.WriteLine(2);
        Console.WriteLine(3);
        Console.WriteLine(5);
        Console.WriteLine(7);
        Console.WriteLine(11);
        Console.WriteLine(13);
        Console.WriteLine(17);

        for (long i = 19; i < Int64.MaxValue; i = i + 2)
        {
            if (i % 3 == 0 || i % 5 == 0 || i % 7 == 0 || i % 11 == 0 || i % 13 == 0 || i % 17 == 0)
                continue;

            // Get one of the 100 "allowances" to add to the queue.
            WorkLimiter.WaitOne();
            ThreadPool.QueueUserWorkItem(CheckForPrime, i);
        }
        Console.Read();
    }

    private static void CheckForPrime(object i)
    {
        var i1 = i as long?;
        try
        {
            var val = Math.Sqrt(i1.Value);
            for (long j = 19; j <= val; j = j + 2)
            {
                if (i1%j == 0) return;
            }
            Console.WriteLine(i1);
        }
        finally
        {
            // Allow another add to the queue
            WorkLimiter.Release();
        }
    }
}

This will allow you to keep the queue full (100 items in queue) at all times, without over-filling it or adding a Sleep.

Upvotes: 4

Adil
Adil

Reputation: 148110

You are creating threads in loop without any break. You should give some break to the process of creating thread so that some of thread finish their execution before you create more thread in the ThreadPool. You can use System.Threading.Thread.Sleep for that.

for (long i = 19; i < Int64.MaxValue; i = i+2)
{
      if(i % 3 == 0 || i % 5 == 0 || i % 7 == 0 || i % 11 == 0 || i % 13 == 0 || i % 17 == 0 )
            continue;
      ThreadPool.QueueUserWorkItem(CheckForPrime, i);
      System.Threading.Thread.Sleep(100);
}

You should know where to use threads and they will be beneficial and how many threads you require and what would be the impact of thread on application performance. It depends upon the application that for how much time it would suspend the current thread. I just gave 100 miliseconds, you adjust according to your application.

Upvotes: 1

Related Questions