ThreadPool giving amazing results, did I do this right? (no, I didn't)

Question

Not that I'm not appreciative of the powers of multithreading or ThreadPool, but I'm scared I broke something since I'm getting a roughly 20x speed increase (2-3s down from over a minute) with a relatively naive usage of ThreadPool. So I submit my code here to be torn apart by people far wiser than I.

Am I doing something wrong here, or is this just a far better candidate for multithreading than I ever hoped? (Yes, this function is an entire thread: like I said, this used to take over a minute to run)

EDIT: To answer my own question, no, this is broken: It seems to be running multiple times, but over the same trigger. Is this because of the way lambda are handled?

private static void CompileEverything()
{
    try
    {
        // maintain the state of our systray icon
        object iconLock = new object();
        bool iconIsOut = true;

        // keep a count of how many threads are still running
        object runCountLock = new object();
        int threadRunning = 0;

        foreach (World w in Worlds)
        {
            foreach (Trigger t in w.Triggers)
            {
                lock (runCountLock)
                {
                    threadRunning++;
                }

                ThreadPool.QueueUserWorkItem(o =>
                {
                    // [snip]: Do some work involving compiling code already in memory with CSharpCodeProvider

                    // provide some pretty feedback
                    lock (iconLock)
                    {
                        if (iconIsOut)
                            notifyIcon.Icon = Properties.Resources.Icon16in;
                        else
                            notifyIcon.Icon = Properties.Resources.Icon16out;

                        iconIsOut = !iconIsOut;
                    }

                    lock (runCountLock)
                    {
                        threadRunning--;
                    }
                });
            }
        }

        // wait for all the threads to finish up
        while (true)
        {
            lock (runCountLock)
            {
                if (threadRunning == 0)
                    break;
            }
        }

        // set the notification icon to our default icon.
        notifyIcon.Icon = Properties.Resources.Icon16;
    }
    // we're going down before we finished starting...
    // oh well, be nice about it.
    catch (ThreadAbortException) { }
}

Answer 1

Well the ThreadPool automatically limits the number of running threads to the number of processors which is maximally efficient. Each context switch is up to 1Mb (default) in 4Kb page increments of memory swapping, so if you're using a lot more threads than cores, you could be getting a ton of speed just from no context switches.

Answer 2

I think you can do better. There is no need to lock around the changes to threadRunning. You can just use Interlocked.Increment() and Interlocked.Decrement():

        Interlocked.Increment(ref threadRunning);
        ThreadPool.QueueUserWorkItem(o =>
        {
            // [snip]: Do some work involving compiling code already in memory with CSharpCodeProvider

            // provide some pretty feedback
            lock (iconLock)
            {
                notifyIcon.Icon = (iconIsOut ? Properties.Resources.Icon16in : Properties.Resources.Icon16out);
                iconIsOut = !iconIsOut;
            }

            Interlocked.Decrement(ref threadRunning);
        });

Answer 3

Interlocked.Increment is better than locking, but the polling loop at the end scares me. First, if you're going to loop, then do a Thread.Sleep(0) to release the processor each time. Second, if you're going to poll for a variable, then you need to make sure it's either marked volatile or you use MemoryBarrier, else the compiler may assume no outside thread will change it and therefore optimize away the check, leading to an infinite loop.

Even better would be for each thread to check for it hitting zero and set an event if it does. You can then wait on the event instead of polling. The only trick is that you want to increment once in the main thread before the dispatch loop, then decrement and check for zero before waiting on the event.

edit

If it's broken because it's reusing the trigger, then the closure is wrong. Trying copying the value of world into a variable local to the inside of the loop and using that variable for the lambda expression.