Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.c++ > #47488 > unrolled thread

thread concurancy

Back to article view | Back to comp.lang.c++

Contents

  thread concurancy Popping mad <rainbow@colition.gov> - 2016-12-21 20:48 +0000
    Re: thread concurancy Paavo Helde <myfirstname@osa.pri.ee> - 2016-12-21 23:39 +0200
      Re: thread concurancy ruben safir <ruben@mrbrklyn.com> - 2016-12-21 23:48 -0500
        Re: thread concurancy Paavo Helde <myfirstname@osa.pri.ee> - 2016-12-22 12:32 +0200
          Re: thread concurancy ruben safir <ruben@mrbrklyn.com> - 2016-12-22 12:13 -0500
            Re: thread concurancy Paavo Helde <myfirstname@osa.pri.ee> - 2016-12-22 20:29 +0200
          Re: thread concurancy scott@slp53.sl.home (Scott Lurndal) - 2016-12-23 14:31 +0000
            Re: thread concurancy Paavo Helde <myfirstname@osa.pri.ee> - 2016-12-23 20:19 +0200
              Re: thread concurancy scott@slp53.sl.home (Scott Lurndal) - 2016-12-23 22:39 +0000
                Re: thread concurancy Paavo Helde <myfirstname@osa.pri.ee> - 2016-12-24 01:31 +0200
      Re: thread concurancy ruben safir <ruben@mrbrklyn.com> - 2016-12-21 23:49 -0500
        Re: thread concurancy "Chris M. Thomasson" <invalid@invalid.invalid> - 2016-12-23 15:19 -0800
    Re: thread concurancy Sal LO <gegefffffff@gmail.com> - 2016-12-22 07:04 -0800
      Re: thread concurancy ruben safir <ruben@mrbrklyn.com> - 2016-12-22 11:00 -0500

#47488 — thread concurancy

:(

I don't know.  I'm very confused about the behavior of this test program I've been right.
I'm trying to move date by worker threads from one blob of memory to another and the debugger 
is saying that the pointers beg and canvas_index is jumping 10 bytes between this two lines

for(int i = 0; i < 10;i++)
			{
			t[i] = std::thread([this]{ readin(beg, canvas_index); });

and I'm not sure why.  I lost the concurrency somewhere, but can't seem to figure out what I did wrong


#include <iostream>
#include <thread>
#include <mutex>
std::mutex medco;
std::mutex master;

namespace testing{
	std::thread t[10];
	class PIC
	{
		public:
		PIC():beg{&source[0]} 
		{
			canvas_index = canvas;
			std::cout << "Testing Source" << std::endl;
			for(int i = 0; i<100; i++)
			{
				std::cout << i << " " << source[i] << std::endl ;
			}
			for(int i = 0; i < 10;i++)
			{
			t[i] = std::thread([this]{ readin(beg, canvas_index); });		
			std::cerr << i << ": Making a thread"  << std::endl; 
			sync_canvas_and_input();
			}
		};

		void sync_canvas_and_input()
		{
			std::cout << "**LOCKING**" << std::endl;
			std::lock_guard<std::mutex> turn(medco);
			beg += 10;
			canvas_index += 10;
		}
		
		~PIC()
		{
			std::cerr << "In the destructor"  << std::endl;
			for(int i=0; i<10; i++)
			{
				t[i].join();
			std::cerr << i << ": Joining a thread"  << std::endl; 
			}

		};
		void readin(char * start, char * loc_canvas_index)
		{
			for( int i = 9; i>=0; i-- )
			{
				*loc_canvas_index = start[i];
				std::cerr << i << ": Copy " << start[i] << std::endl;
				std::cerr << i << ": Copied to loc_canvas_index " << reinterpret_cast<char>(*loc_canvas_index) << std::endl;
				loc_canvas_index++;
			}

[toc] | [next] | [standalone]

#47493

On 21.12.2016 22:48, Popping mad wrote:
> :(
>
> I don't know.  I'm very confused about the behavior of this test program I've been right.
> I'm trying to move date by worker threads from one blob of memory to another and the debugger
> is saying that the pointers beg and canvas_index is jumping 10 bytes between this two lines
>
> for(int i = 0; i < 10;i++)
> 			{
> 			t[i] = std::thread([this]{ readin(beg, canvas_index); });
>
> and I'm not sure why.  I lost the concurrency somewhere, but can't seem to figure out what I did wrong

Who knows. Debuggers sometimes also get confused and display wrong data.

Your example is incomplete (truncated?), cannot be compiled and even the 
types of beg and canvas_index are not known. So not much can be said 
about the code.

 From the visible code I can only infer that you have not understood why 
and how to protect data with mutexes (mutex lock is in one thread only, 
and the data apparently  protected by the lock (beg, canvas_index) is 
not even accessed in the other threads.

Also, access to the shared data buffer (canvas) is creating a lot of 
false sharing as the slicing size 10 is most probably not divisible by 
the cache line size, thus causing potentially significant performance 
penalties (probably not important for your toy example, but worth to 
mention).

>
> #include <iostream>
> #include <thread>
> #include <mutex>
> std::mutex medco;
> std::mutex master;
>
> namespace testing{
> 	std::thread t[10];
> 	class PIC
> 	{
> 		public:
> 		PIC():beg{&source[0]}
> 		{
> 			canvas_index = canvas;
> 			std::cout << "Testing Source" << std::endl;
> 			for(int i = 0; i<100; i++)
> 			{
> 				std::cout << i << " " << source[i] << std::endl ;
> 			}
> 			for(int i = 0; i < 10;i++)
> 			{
> 			t[i] = std::thread([this]{ readin(beg, canvas_index); });		
> 			std::cerr << i << ": Making a thread"  << std::endl;
> 			sync_canvas_and_input();
> 			}
> 		};
>
> 		void sync_canvas_and_input()
> 		{
> 			std::cout << "**LOCKING**" << std::endl;
> 			std::lock_guard<std::mutex> turn(medco);
> 			beg += 10;
> 			canvas_index += 10;
> 		}
> 		
> 		~PIC()
> 		{
> 			std::cerr << "In the destructor"  << std::endl;
> 			for(int i=0; i<10; i++)
> 			{
> 				t[i].join();
> 			std::cerr << i << ": Joining a thread"  << std::endl;
> 			}
>
> 		};
> 		void readin(char * start, char * loc_canvas_index)
> 		{
> 			for( int i = 9; i>=0; i-- )
> 			{
> 				*loc_canvas_index = start[i];
> 				std::cerr << i << ": Copy " << start[i] << std::endl;
> 				std::cerr << i << ": Copied to loc_canvas_index " << reinterpret_cast<char>(*loc_canvas_index) << std::endl;
> 				loc_canvas_index++;
> 			}
>

[toc] | [prev] | [next] | [standalone]

#47505

How is this fix ;)


/*
 * =====================================================================================
 *
 *       Filename:  test.cpp
 *
 *    Description:  Threading Experiment
 *
 *        Version:  1.0
 *        Created:  12/18/2016 12:46:51 PM
 *       Revision:  none
 *       Compiler:  gcc
 *
 *         Author:  Ruben Safir (mn), ruben@mrbrklyn.com
 *        Company:  NYLXS Inc
 *
 * =====================================================================================
 */

#include <iostream>
#include <thread>
#include <mutex>
std::mutex medco;
std::mutex master;

namespace testing{
	std::thread t[10];
	class PIC
	{
		public:
		PIC():source_index{&source[0]} 
		{
			canvas_index = canvas;
			std::cout << "Testing Source" << std::endl;
			for(int i = 0; i<100; i++)
			{
				std::cout << i << " " << source[i] << std::endl ;
			}

			for(int i = 0; i < 10;i++)
			{
			t[i] = std::thread([this]{ readin(); });		
			std::cerr << i << ": Making a thread"  << std::endl; 
			}
		};

		
		~PIC()
		{
			std::cerr << "In the destructor"  << std::endl;
			for(int i=0; i<10; i++)
			{
				t[i].join();
			std::cerr << i << ": Joining a thread"  << std::endl; 
			}

		};
		
		void readin()
		{
			char * loc_canvas_index;
			char * loc_source_index;
			sync_canvas_and_input(loc_canvas_index, loc_source_index);
			
			for( int i = 9; i>=0; i-- )
			{
				*loc_canvas_index = loc_source_index[i];
				std::cerr << i << ": Copy " << loc_source_index[i] << std::endl;
				std::cerr << i << ": Copied to loc_canvas_index " << reinterpret_cast<char>(*loc_canvas_index) << std::endl;
				loc_canvas_index++;
			}

		};
		void sync_canvas_and_input(char * &loc_canvas_index, char * &loc_source_index )
		{
			std::cout << "**LOCKING**" << std::endl;
			std::lock_guard<std::mutex> turn(medco);
			loc_canvas_index = canvas_index;
			loc_source_index = source_index;
			source_index += 10;
			canvas_index += 10;
		};

		char * get_canvas()
		{
			return canvas;
		}
		char * get_canvas_index()
		{
			return canvas_index;
		}
		char * get_source_index()
		{
			return source_index;
		}

		private:
		char * canvas = new char[100]{
			'z', 'z','z','z','z','z','z','z','z','z',
			'z', 'z','z','z','z','z','z','z','z','z',
			'z', 'z','z','z','z','z','z','z','z','z',
			'z', 'z','z','z','z','z','z','z','z','z',
			'z', 'z','z','z','z','z','z','z','z','z',
			'z', 'z','z','z','z','z','z','z','z','z',
			'z', 'z','z','z','z','z','z','z','z','z',
			'z', 'z','z','z','z','z','z','z','z','z',
			'z', 'z','z','z','z','z','z','z','z','z',
			'z', 'z','z','z','z','z','z','z','z','z'
		};
		char * canvas_index;
		char source[100] = {
			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'
		};
		char * source_index;
	};
}//end namespace

int main(int argc, char** argv)
{
	testing::PIC fido;
	for(int i = 0; i<100;i++)
	{
		std::cout << i << " Canvas Position " << fido.get_canvas()[i] << std::endl;
	}

	return 0;
}

[toc] | [prev] | [next] | [standalone]

#47509

On 22.12.2016 6:48, ruben safir wrote:
>
> How is this fix ;)

Yes, this makes a bit more sense, at least the mutex is really used for 
mutual exclusion. A couple of remarks:

The functions get_canvas_index() and get_source_index() are lacking the 
mutex lock. Any access to data which is concurrently modified needs a 
mutex lock.

The function get_canvas() returns a pointer to the shared array without 
any synchronization. This means race conditions. Instead, it should wait 
on a std::condition_variable until all threads have fulfilled the task; 
upon completion, each thread should increment a counter of completed 
tasks under another mutex lock and notify the condition variable. (In 
this toy example this waiting could be replaced by just joining all the 
threads, but in real life you typically don't want to terminate your 
service threads after each task.)

The mutex is defined in another place (a global!) than the protected 
data. This is insane. The mutex should be together with the protected 
data, preferably in its own section of private variables, clearly 
documented:

   private: // data members protected by medco
     std::mutex medco;
     char * canvas_index;
     char * source_index;

   private: // other data
     // ...

The thread array t is also global, with no need, no protection, etc. As 
soon as you create another instance of PIC it gets garbled.

HTH
Paavo

>
> /*
>   * =====================================================================================
>   *
>   *       Filename:  test.cpp
>   *
>   *    Description:  Threading Experiment
>   *
>   *        Version:  1.0
>   *        Created:  12/18/2016 12:46:51 PM
>   *       Revision:  none
>   *       Compiler:  gcc
>   *
>   *         Author:  Ruben Safir (mn), ruben@mrbrklyn.com
>   *        Company:  NYLXS Inc
>   *
>   * =====================================================================================
>   */
>
> #include <iostream>
> #include <thread>
> #include <mutex>
> std::mutex medco;
> std::mutex master;
>
> namespace testing{
> 	std::thread t[10];
> 	class PIC
> 	{
> 		public:
> 		PIC():source_index{&source[0]}
> 		{
> 			canvas_index = canvas;
> 			std::cout << "Testing Source" << std::endl;
> 			for(int i = 0; i<100; i++)
> 			{
> 				std::cout << i << " " << source[i] << std::endl ;
> 			}
>
> 			for(int i = 0; i < 10;i++)
> 			{
> 			t[i] = std::thread([this]{ readin(); });		
> 			std::cerr << i << ": Making a thread"  << std::endl;
> 			}
> 		};
>
> 		
> 		~PIC()
> 		{
> 			std::cerr << "In the destructor"  << std::endl;
> 			for(int i=0; i<10; i++)
> 			{
> 				t[i].join();
> 			std::cerr << i << ": Joining a thread"  << std::endl;
> 			}
>
> 		};
> 		
> 		void readin()
> 		{
> 			char * loc_canvas_index;
> 			char * loc_source_index;
> 			sync_canvas_and_input(loc_canvas_index, loc_source_index);
> 			
> 			for( int i = 9; i>=0; i-- )
> 			{
> 				*loc_canvas_index = loc_source_index[i];
> 				std::cerr << i << ": Copy " << loc_source_index[i] << std::endl;
> 				std::cerr << i << ": Copied to loc_canvas_index " << reinterpret_cast<char>(*loc_canvas_index) << std::endl;
> 				loc_canvas_index++;
> 			}
>
> 		};
> 		void sync_canvas_and_input(char * &loc_canvas_index, char * &loc_source_index )
> 		{
> 			std::cout << "**LOCKING**" << std::endl;
> 			std::lock_guard<std::mutex> turn(medco);
> 			loc_canvas_index = canvas_index;
> 			loc_source_index = source_index;
> 			source_index += 10;
> 			canvas_index += 10;
> 		};
>
> 		char * get_canvas()
> 		{
> 			return canvas;
> 		}
> 		char * get_canvas_index()
> 		{
> 			return canvas_index;
> 		}
> 		char * get_source_index()
> 		{
> 			return source_index;
> 		}
>
> 		private:
> 		char * canvas = new char[100]{
> 			'z', 'z','z','z','z','z','z','z','z','z',
> 			'z', 'z','z','z','z','z','z','z','z','z',
> 			'z', 'z','z','z','z','z','z','z','z','z',
> 			'z', 'z','z','z','z','z','z','z','z','z',
> 			'z', 'z','z','z','z','z','z','z','z','z',
> 			'z', 'z','z','z','z','z','z','z','z','z',
> 			'z', 'z','z','z','z','z','z','z','z','z',
> 			'z', 'z','z','z','z','z','z','z','z','z',
> 			'z', 'z','z','z','z','z','z','z','z','z',
> 			'z', 'z','z','z','z','z','z','z','z','z'
> 		};
> 		char * canvas_index;
> 		char source[100] = {
> 			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
> 			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
> 			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
> 			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
> 			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
> 			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
> 			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
> 			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
> 			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
> 			'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'
> 		};
> 		char * source_index;
> 	};
> }//end namespace
>
> int main(int argc, char** argv)
> {
> 	testing::PIC fido;
> 	for(int i = 0; i<100;i++)
> 	{
> 		std::cout << i << " Canvas Position " << fido.get_canvas()[i] << std::endl;
> 	}
>
> 	return 0;
> }
>

[toc] | [prev] | [next] | [standalone]

#47518

On 12/22/2016 05:32 AM, Paavo Helde wrote:
> The functions get_canvas_index() and get_source_index() are lacking the
> mutex lock. Any access to data which is concurrently modified needs a
> mutex lock.


Wouldn't that cause a deadlock condition?

[toc] | [prev] | [next] | [standalone]

#47520

On 22.12.2016 19:13, ruben safir wrote:
> On 12/22/2016 05:32 AM, Paavo Helde wrote:
>> The functions get_canvas_index() and get_source_index() are lacking the
>> mutex lock. Any access to data which is concurrently modified needs a
>> mutex lock.
>
>
> Wouldn't that cause a deadlock condition?

Deadlock means that at least 2 threads wait for each other. In your 
example there is only one function, called in a single thread only, 
which ever waits for anything (the destructor, in the thread join 
operations), so there is no possibility of deadlocks.

Or alternatively, if you look at the code of sync_canvas_and_input() 
then you see that when it has locked the mutex it will release it again 
a microsecond or so later, unconditionally. Mutex locks in 
get_canvas_index() and get_source_index() are needed so that these 
functions would not return incompatible or garbage information during 
this microsecond. This is the whole point of "mutual exclusion".

[toc] | [prev] | [next] | [standalone]

#47544

Paavo Helde <myfirstname@osa.pri.ee> writes:
>On 22.12.2016 6:48, ruben safir wrote:
>>
>> How is this fix ;)
>
>Yes, this makes a bit more sense, at least the mutex is really used for 
>mutual exclusion. A couple of remarks:
>
>The functions get_canvas_index() and get_source_index() are lacking the 
>mutex lock. Any access to data which is concurrently modified needs a 
>mutex lock.

Not necessarily - atomics are a viable alternative.

[toc] | [prev] | [next] | [standalone]

#47553

On 23.12.2016 16:31, Scott Lurndal wrote:
> Paavo Helde <myfirstname@osa.pri.ee> writes:
>> On 22.12.2016 6:48, ruben safir wrote:
>>>
>>> How is this fix ;)
>>
>> Yes, this makes a bit more sense, at least the mutex is really used for
>> mutual exclusion. A couple of remarks:
>>
>> The functions get_canvas_index() and get_source_index() are lacking the
>> mutex lock. Any access to data which is concurrently modified needs a
>> mutex lock.
>
> Not necessarily - atomics are a viable alternative.

Yes, atomics are fine in many situations, but not here, in the OP 
example. They are updated and read concurrently by the 
sync_canvas_and_input() function. If it was using 2 atomics instead, 
then the values returned are not guaranteed to be in sync, which would 
cause interesting bugs (swapped slices in the result).

[toc] | [prev] | [next] | [standalone]

#47573

Paavo Helde <myfirstname@osa.pri.ee> writes:
>On 23.12.2016 16:31, Scott Lurndal wrote:
>> Paavo Helde <myfirstname@osa.pri.ee> writes:

>>> Any access to data which is concurrently modified needs a
>>> mutex lock.
>>
>> Not necessarily - atomics are a viable alternative.
>
>Yes, atomics are fine in many situations, but not here, in the OP 

You stated, as above, "Any access to data which is concurrently
modified needs a mutex lock".   To which I replied as shown.  I
made no claim about the OP's code.

[toc] | [prev] | [next] | [standalone]

#47577

On 24.12.2016 0:39, Scott Lurndal wrote:
> Paavo Helde <myfirstname@osa.pri.ee> writes:
>> On 23.12.2016 16:31, Scott Lurndal wrote:
>>> Paavo Helde <myfirstname@osa.pri.ee> writes:
>
>>>> Any access to data which is concurrently modified needs a
>>>> mutex lock.
>>>
>>> Not necessarily - atomics are a viable alternative.
>>
>> Yes, atomics are fine in many situations, but not here, in the OP
>
> You stated, as above, "Any access to data which is concurrently
> modified needs a mutex lock".   To which I replied as shown.  I
> made no claim about the OP's code.

Right, I should have worded my claims more carefully!

Merry Christmas!
p

[toc] | [prev] | [next] | [standalone]

#47506

On 12/21/2016 04:39 PM, Paavo Helde wrote:
> alse sharing as the slicing size 10 is most probably not divisible by
> the cache line size, thus causing potentially significant performance
> penalties (probably not important for your toy example, but worth to
> mention).


That is a VERY cool observation!!!

Thank You

[toc] | [prev] | [next] | [standalone]

#47576

On 12/21/2016 8:49 PM, ruben safir wrote:
> On 12/21/2016 04:39 PM, Paavo Helde wrote:
>> alse sharing as the slicing size 10 is most probably not divisible by
>> the cache line size, thus causing potentially significant performance
>> penalties (probably not important for your toy example, but worth to
>> mention).
>
>
> That is a VERY cool observation!!!
>
> Thank You
>

Agreed. False sharing is the root of all evil wrt 
multi-thread/processing. What good is a slick use of atomics if the 
data-structures are not properly padded up to, and allocated in memory 
on properly aligned cache lines?

False sharing will totally ruin any chance of a possible speed up wrt 
atomic's.

[toc] | [prev] | [next] | [standalone]

#47514

On Wednesday, December 21, 2016 at 10:49:05 PM UTC+2, Popping mad wrote:
> :(
> 
> I don't know.  I'm very confused about the behavior of this test program I've been right.
> I'm trying to move date by worker threads from one blob of memory to another and the debugger 
> is saying that the pointers beg and canvas_index is jumping 10 bytes between this two lines
> 
> for(int i = 0; i < 10;i++)
> 			{
> 			t[i] = std::thread([this]{ readin(beg, canvas_index); });
> 
> and I'm not sure why.  I lost the concurrency somewhere, but can't seem to figure out what I did wrong
> 
> 
> #include <iostream>
> #include <thread>
> #include <mutex>
> std::mutex medco;
> std::mutex master;
> 
> namespace testing{
> 	std::thread t[10];
> 	class PIC
> 	{
> 		public:
> 		PIC():beg{&source[0]} 
> 		{
> 			canvas_index = canvas;
> 			std::cout << "Testing Source" << std::endl;
> 			for(int i = 0; i<100; i++)
> 			{
> 				std::cout << i << " " << source[i] << std::endl ;
> 			}
> 			for(int i = 0; i < 10;i++)
> 			{
> 			t[i] = std::thread([this]{ readin(beg, canvas_index); });		
> 			std::cerr << i << ": Making a thread"  << std::endl; 
> 			sync_canvas_and_input();
> 			}
> 		};
> 
> 		void sync_canvas_and_input()
> 		{
> 			std::cout << "**LOCKING**" << std::endl;
> 			std::lock_guard<std::mutex> turn(medco);
> 			beg += 10;
> 			canvas_index += 10;
> 		}
> 		
> 		~PIC()
> 		{
> 			std::cerr << "In the destructor"  << std::endl;
> 			for(int i=0; i<10; i++)
> 			{
> 				t[i].join();
> 			std::cerr << i << ": Joining a thread"  << std::endl; 
> 			}
> 
> 		};
> 		void readin(char * start, char * loc_canvas_index)
> 		{
> 			for( int i = 9; i>=0; i-- )
> 			{
> 				*loc_canvas_index = start[i];
> 				std::cerr << i << ": Copy " << start[i] << std::endl;
> 				std::cerr << i << ": Copied to loc_canvas_index " << reinterpret_cast<char>(*loc_canvas_index) << std::endl;
> 				loc_canvas_index++;
> 			}

https://youtu.be/Al9bmstjUjc?t=2s

[toc] | [prev] | [next] | [standalone]

#47517

> 
> https://youtu.be/Al9bmstjUjc?t=2s
> 

I'm deaf and can't hear videos, but thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.c++

csiph-web