Help with basic multithreaded program

[patrickob]

Hi all you C++ brilliant peoples

I'm writing my first multithreaded program to test processing time and for some reason my multithreaded program takes longer to process than when its structured using a single thread. I've provided the entire program below.

I have 2 functions each performing 500 inputs into an array of intergers 1 million times. I time this, do it 10 times, and take an average.
When i call the functions in order in a single thread, it takes 5.8 seconds on average and only 1 CPU Core is used (25% of my cores),
However, when i call both functions simultaniously, it takes 8 seconds on average and roughly 78% of my CPU Cores are used.....wtf.
The functions do not share any variables.
Am i doing something horribly wrong here? Cause it seems to me that if I'm using triple the processing power and not trying to access the same variables then the program should go at least twice as fast.
Any advice would be greatly appreciated.
Cheers
Pat

#include <cstdlib>
#include <iostream>
#include <time.h>
#include <process.h>


//This program is a test of multithreaded processing time

int x,y,z,a,b,c,math1[501],math2[501],times[10];
float avgtime=0;
bool on1,on2;

using namespace std;

void functionone(void *)
{
     for(x=0;x<1000000;x++)
        for(y=0;y<500;y++)
           math1[y]=x/25;
     on1=true;
     return;
     }
void functiontwo(void *)
{
     for(a=0;a<1000000;a++)
        for(b=0;b<500;b++)
           math2[b]=a/25;
        on2=true;
     return;
     }

int main(int argc, char *argv[])
{
    time_t time1,time2;
    for(z=0;z<10;z++)
    {
      on1=false;
      on2=false;
      time1=time(NULL);
       _beginthread(functionone,0,0);
       _beginthread(functiontwo,0,0);
       do {}while((!on1) || (!on2));
       time2=time(NULL);
       times[z]=time2-time1;
       }
           
    for(z=0;z<10;z++) avgtime+=times[z];
    cout<<"Functions Complete in "<<avgtime/10<<" seconds.";
    system("PAUSE");
    return EXIT_SUCCESS;
}

[shacktar]

This is your problem:

do {}while((!on1) || (!on2));

It's called spin-waiting and it eats up precious CPU resources. Ideally, the main thread should be sleeping until the threads are done, at which point it should "wake up". For your particular case, what I would do is assign an event to each thread. Set the events to unsignalled and run the threads. Each thread should set its event to signalled on completion. Meanwhile, you should use a built-in OS function to wait for the events. In Windows, you could use WaitForSingleObject and WaitForMultipleObjects. In your case, you should use the latter.

[patrickob]

Ok, I will look into that and try it, TY for your advice.

[patrickob]

Ok so i added event handles, set to unsignalled and ran it. It works a little bit faster (7.2 seconds avg), and it does only use 50% of the CPU power this time. So that definitly worked to pause the parent process. But its still running slower than a single thread.
Any additional ideas?
New Code:

#include <cstdlib>
#include <iostream>
#include <time.h>
#include <process.h>
#include <windows.h>

int x,y,z,a,b,c,math1[501],math2[501],times[10];
float avgtime=0;
HANDLE hands[3];

using namespace std;

void functionone(void *)
{
     for(x=0;x<1000000;x++)
        for(y=0;y<500;y++)
           math1[y]=x/25;
     SetEvent(hands[0]);
     return;
     }
void functiontwo(void *)
{
     for(a=0;a<1000000;a++)
        for(b=0;b<500;b++)
           math2[b]=a/25;
     SetEvent(hands[1]);
     return;
     }

int main(int argc, char *argv[])
{
    time_t time1,time2;
    hands[0]=CreateEvent(NULL,false,false,NULL);
    hands[1]=CreateEvent(NULL,false,false,NULL);    
    for(z=0;z<10;z++)
    {
       time1=time(NULL);
       _beginthread(functionone,0,0);
       _beginthread(functiontwo,0,0);
       WaitForMultipleObjects(2,hands,true,INFINITE); //Pause for Threads
       time2=time(NULL);
       times[z]=time2-time1;
       }
           
    for(z=0;z<10;z++) avgtime+=times[z];
    cout<<"Functions Complete in "<<avgtime/10<<" seconds.";
    system("PAUSE");
    return EXIT_SUCCESS;
}

[shacktar]

Perhaps try the events as manual reset events and see if that makes a difference? Other than that, I'm fresh out of ideas. Maybe for this example, the cost of context switching outweighs the benefit of multiple threads.