Beefy Boxes and Bandwidth Generously Provided by pair Networks
P is for Practical
 
PerlMonks  

Re^2: RFC: Using 'threads' common aspects

by DeadPoet (Scribe)
on Jan 08, 2011 at 15:19 UTC ( #881232=note: print w/ replies, xml ) Need Help??


in reply to Re: RFC: Using 'threads' common aspects
in thread RFC: Using 'threads' common aspects

I am presuming that you are referring to the independent threads that manage the print_manager and the thread_timer aspect of the program. Yes, those threads are outside of the pool and are started near the top of MAIN.

my $thr_pm = threads->create( \&print_manager )->detach(); my $thr_tm = threads->create( \&thread_timer )->detach();
However, the worker aspect of the program operates from a pool of threads, which are reused and defined by:
for ( 1..$DEFAULTS->{'max_threads'} ) { my $q_work = Thread::Queue->new(); my $thr = threads->create( \&thread_worker, $q_work ); $work_queues{$thr->tid()} = $q_work; } ## end for max_threads.
I am expecting the ':locked' attribute to ensure that no other thread, intentional or otherwise, will access the subroutine while the active thread has control. Do you read otherwise? If so, how?


Comment on Re^2: RFC: Using 'threads' common aspects
Select or Download Code
Re^3: RFC: Using 'threads' common aspects
by BrowserUk (Pope) on Jan 08, 2011 at 18:00 UTC
    I am expecting the ':locked' attribute to ensure that no other thread, intentional or otherwise, will access the subroutine while the active thread has control.

    If you read the threads documentation, the :locked attribute isn't mentioned because it does nothing. Quite literally nothing. It is a noop.

    It is non-functioning leftover from perl5005 threads that has never had any affect on iThreads.

    I am presuming that you are referring to the independent threads that manage the print_manager and the thread_timer aspect of the program.

    No. Having a couple of other threads outside of the worker pool isn't a problem. That's quite normal.

    The problem is that you manage each of the threads in your 'pool' as individual entities. Ie. Each has a separate queue. Which means that your dispatcher has to allocate and coordinate work to each worker individually. The result of which is the excessive complexity of your implementation. With a traditional thread pool, all the threads are identical and often anonymous. And they get their work from a single work queue.

    Think of this as analogous to the baggage carousels in airports. The bags are loaded onto the belt back-of-house, and the 'workers', passengers in this scenario, collect the bags from the carousel entirely independently of each other, and of the loading.

    What you've done is to replace the carousel with a fan of one-to-one conveyors, and placed a dispatcher between the inbound conveyor and that fan. He has to take the bags off the inbound conveyor, read the labels, and decide which of the fan of conveyors he needs to place each bag on for it to get to the appropriate customer.

    You've added huge complexity--the fan of one-customer-at-a-time, one-bag-at-a-time conveyors; and created a bottle-neck--the dispatcher--upon whom all the customers have to wait. Not to mention all the tagging, control and coordination problems created by the need to route each customer to the right conveyor and each bag to the right customer.

    In the process, you've made some very strange choices.

    For example, your 'jobs' queue. You create a shared array, onto which you push all your jobs en-mass. You then pull them off that queue and feed them onto the worker's individual queues. But it is the same thread that puts them on the jobs queue as takes them off. So there was no need for a shared data-structure--with all the size and performance penalties it entails. A simple array would have sufficed.

    And the rest of your code is full of similar anomalies.

    I'm trying very hard not to be negative of someone trying to do what I have declined to do, but the truth is that as a tutorial, your code is actually worse than no tutorial, because it teaches a lot of bad practices. And once out there, they become very hard to unlearn, and the time-bombs they will create become very hard to diffuse.

    Given the (unwritten, despite your original claim to "solid documentation") 'spec' of your 400+ line program, its function (such as it is) can be easily and far, far more clearly achieved with 30 or 40 lines, of cleaner, clearer and more easily maintained code. It is typical of code written to "demonstrate some functionality" rather than solve a problem. It is also typical of a program written by someone who has yet to try and use iThreads in a real application.

    Sorry.


    Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
    "Science is about questioning the status quo. Questioning authority".
    In the absence of evidence, opinion is indistinguishable from prejudice.
      There is no need at all to be sorry. This is exactly why I placed this as an RFC. While your points simply enforces why I feel a solid illustration of threads is needed. Moreover, I agree that this example may not be it, but at least the topic is raised.

      Next, many will say just use module X or module Y to solve this or that thread problem. However, did one really learn how to do it, or did they just learn a shortcut that they may not have in the future? Me, I prefer to learn and understand.

      Finally, I have great respect for your comments and thoughts, as I see you have actually authored several write-ups. While others tend never to pass knowledge on and consistently stand in judgement of those that would. So, no apologies are needed. --Poet
      Okay, let me back this off a bit and see if I can't get this correct. For now, I have dropped the timers and I "think" I have made the corrections that you have stated. However, I am not 100% and would appreciate your eyes.

      If there are glaring errors, then please provide insight as to what I have done wrong.
        • Jobs queue/array:

          Okay. You took my point about your jobs queue only being accessed by a single thread and reverted that to being just an array rather than a queue. You also took my point about the fan of 1-per-worker queues, and did away with them. But now, as you've done away with the latter, it is necessary for the workers to access the former, so you've made the jobs array shared. Of course, that means that you need to lock the shared array before removing jobs from it, so you've added the get_next_job() subroutine that takes care of the locking. And what you've ended up doing is incompletely re-implementing Thread::Queue, which is just a shared array that does locking.

          What you could (should?) have done is just do away with the per-worker queues and retained the jobs queue. Then your workers get their jobs from that queue using dequeue in the normal way and get_next_job() is unnecessary.

        • Idle queue:

          In your thread subroutine you push the tid and zero each time around the work loop. And when the work loop ends (no more jobs), you push the tid and -1. And in the do_work() sub you push the tid and 1.

          Why? What code is reading and acting upon these messages?

          The only place where you read this queue is in the main() sub. After you've placed all the jobs in the array and started all the workers, you enter a loop that dequeues values from the idle queue. But, it dequeues one value at a time and labels it $tid, even though each time you enqueued a tid you also enqueued a magic number (-1,0 or 1). It then decides that if the number (referred to as $tid, but half the time, just one of the magic numbers), is less than 0, then it exits that loop.

          Lets say you queue up 4 jobs, and start 2 workers, and each worker processes 2 jobs. Then the contents of the idle queue--ignoring anything being removed for a moment--will be something like (the ordering is non-deterministic):

          tid. magic 1 0 Worker 1 first job in while loop 1 1 Worker 1 first job in do_work 2 0 Worker 2 first job in while loop 2 1 Worker 2 first job in do_work 1 0 Worker 1 second job in while loop 1 1 Worker 1 second job in do_work 2 0 Worker 2 second job in while loop 2 1 Worker 2 second job in do_work 1 -1 Worker 1 after exiting while loop 2 -1 Worker 2 after exiting while loop

          But, as mentioned above, that ordering is non-determanistic, so could also be:

          tid. magic 1 0 Worker 1 first job in while loop 1 1 Worker 1 first job in do_work 1 0 Worker 1 second job in while loop 1 1 Worker 1 second job in do_work 1 -1 Worker 1 after exiting while loop 2 0 Worker 2 first job in while loop 2 1 Worker 2 first job in do_work 2 0 Worker 2 second job in while loop 2 1 Worker 2 second job in do_work 2 -1 Worker 2 after exiting while loop
          tid. magic 2 0 Worker 2 first job in while loop 2 1 Worker 2 first job in do_work 2 0 Worker 2 second job in while loop 2 1 Worker 2 second job in do_work 2 -1 Worker 2 after exiting while loop 1 0 Worker 1 first job in while loop 1 1 Worker 1 first job in do_work 1 0 Worker 1 second job in while loop 1 1 Worker 1 second job in do_work 1 -1 Worker 1 after exiting while loop

          Or any of a dozen other variations. So what is it telling the main loop?

          Remember, that the loop in main will terminate the first time it sees a value less than zero. And if you look at the above possibilities, that will never actually indicate that all the threads have completed. Which I what I think was your intent. Even with only two threads and the most optimistic ordering, there will always be at least two values that will never be removed from that queue. In the worst case scenario, which will happen far more often that statistics alone would suggest, the while loop in main() will terminate when all the workers except one are still processing many jobs.

          What affect will the worse case scenario have upon your program?

          Actually, nothing. Because as soon as you exit the while loop in main(), you call shutdown_engine(), and the first thing it does is loop over all the thread objects and waits for them to join. Ie end.

          The net results is that the entire idle queue mechanism is not only totally broken, it is entirely redundant.

          Or at least it would be if you weren't detaching your threads which makes the joins redundant. But that also means that those return values you set up and return from your workers are also redundant.

        • Print manager:

          Why are you queueing all your messages to a separate thread in order to have them printed?

          Your answer will be something to the effect that you need to serialise the output from multiple threads to the single resource--the screen or file attached to STDOUT. And that is true. But do you need to use a separate thread and queue to do this?

          And the answer is no. It is a waste of resource and an additional level of redundant complexity. A simple print wrapper combined with a single shared variable to act as a mutex is all that is required. Eg;

          my $semSTDOUT : shared; sub tprint{ lock $semSTDOUT; print @_; } tprint "$tid: Some text";

          Simpler to write and maintain and far more flexible and convenient to use.

        • run_external_command()
          sub run_external_command { my ( $job, $tid ) = @_; $q_PRINT->enqueue( "Thread $tid is running exteral job $job" ); local( *w_IN, *w_OUT, *w_ERR ); my $pid = open3( \*w_IN, \*w_OUT, \*w_ERR, '/bin/sleep', int( rand +($DEFAULTS->{'random'} ) ) ); waitpid( $pid, 0 ); return $DEFAULTS->{'ret_success'}; }

          Why are you using open3? Instead of say, just system which does everything your code currently does.

          I appreciate that you were originally limiting the time the command ran for, which means you need a pid in order to use kill, but you can also get a pid from piped open, and that doesn't suffer the problems associated with open3. (Namely, that juggling two output streams either of which can block the process if they fill, is notoriously difficult to get right.)

        • timestamp()

          is never used.

        • local $SIG{'KILL'} = sub { threads->exit(); };

          Why? If all you are going to do is terminate, why not just let that happen?

        • Finally %DEFAULTS.

          You set up this hash named %DEFAULTS containing a bunch of named values.

          But, there is no mechanism for modifying them, so they aren't defaults, but rather constants. So why not use constant?

          In addition, none of those return values is ever assigned anywhere, much less checked or acted upon.

        So, here is how I would write a program to do what I believe you intend your program to do.

        Stylistically, it is written in my (infinitely preferable and superior :) style, but essentially does everything yours attempts to do. But it does correctly and in a simple, clear, easily understood and maintainable way.

        #! perl -slw use strict; use threads; use threads::shared; use Thread::Queue; use constant { RANDOM => 4, THREADS => 4, JOBS => 20, }; my $semSTD :shared; sub tprint { my $tid = threads->tid; lock $semSTD; print "[$tid] ", @_; } my $die_early :shared = 0; $SIG{ INT } = sub { tprint "Early termination requested"; $die_early = 1; }; sub worker { tprint "worker started"; my( $Q ) = @_; while( !$die_early and defined( my $job = $Q->dequeue ) ) { tprint "processing job:$job"; my $pid = open my $PIPE, '-|', "sleep " . int( rand RANDOM ) o +r die $!; tprint "waiting for pid: $pid"; waitpid $pid, 0; tprint "pid: $pid done"; } tprint "Worker ending"; return 1; } my $Q = new Thread::Queue; $Q->enqueue( map "JOB-$_", 1 .. JOBS ); $Q->enqueue( (undef) x THREADS ); tprint "Queue populated"; my @threads = map threads->new( \&worker, $Q ), 1 .. THREADS; tprint "Workers started; waiting..."; $_->join for @threads; print "Program complete"; __END__ C:\test>881217 [0] Queue populated [1] worker started [1] processing job:JOB-1 [1] waiting for pid: 1740 [2] worker started [2] processing job:JOB-2 [1] waiting for pid: 1056 [2] pid: 2684 done [2] processing job:JOB-4 [1] waiting for pid: 2116 [3] worker started [3] processing job:JOB-6 [3] waiting for pid: 3244 [0] Workers started; waiting... [4] worker started [4] processing job:JOB-7 ... [3] pid: 1112 done [3] processing job:JOB-20 [3] waiting for pid: 4048 [2] pid: 1728 done [1] pid: 3432 done [2] Worker ending [1] Worker ending [4] pid: 3924 done [4] Worker ending [3] pid: 4048 done [3] Worker ending Program complete

        Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
        "Science is about questioning the status quo. Questioning authority".
        In the absence of evidence, opinion is indistinguishable from prejudice.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://881232]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others exploiting the Monastery: (12)
As of 2014-10-02 15:14 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    What is your favourite meta-syntactic variable name?














    Results (64 votes), past polls