Whats up with the ThreadPoolExecutor?

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Whats up with the ThreadPoolExecutor?

Ravinder Singh-2
I don't know if this is a bug or me just using it wrongly. But I add
work to the pool, and I add the same Runnable each time. Since I try to
avoid GC. But it seems that it doesn't process the full queue. If I
create a new Runnable each time its ok.

It seems that the remaining work (unprocessed) is the amount of maximum
threads. So it seems taht a new thread is not able to start its first
work somehow...

Tried to extract the code in this small application:
-----------------------------------------------------------------------------------------------------------------------------

import org.apache.log4j.BasicConfigurator;
import edu.emory.mathcs.backport.java.util.concurrent.*;

public class WPTest
{
   static ThreadPoolExecutor wp = null;
   static Runnable workInProgress = new Woerker();
   static private int cSt, cWrk;
   static int WORKCOUNT = 1000;


   public static void main(String[] args)
   {
       BasicConfigurator.configure();
       wp = new ThreadPoolExecutor(1, 10, 60, TimeUnit.SECONDS, new
ArrayBlockingQueue(30));
       wp.setCorePoolSize(0);
       wp.setMaximumPoolSize(30);
       wp.setKeepAliveTime(60000, TimeUnit.MILLISECONDS);

       for (int i = 0; i < WORKCOUNT; i++)
       {
           cWrk++;
           addWork(workInProgress);
       }

       while (cSt != WORKCOUNT)
       {
           System.out.println("Counters: " + ":" + cSt + ":" + cWrk);
           try
           {
               Thread.sleep(1000);
           }
           catch (InterruptedException e)
           {
               e.printStackTrace();
           }
       }
       System.out.println("Done...");
   }


   public static class Woerker implements Runnable
   {
       public void run()
       {
           cSt++;
       }
   }


   public static void addWork(Runnable r)
   {
       boolean addOk = false;
       while (!addOk)
       {
           try
           {
               wp.execute(r);
               addOk = true;
           }
           catch (RejectedExecutionException rx)
           {
               // Ignore, keep trying.
           }
       }
   }
}


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Whats up with the ThreadPoolExecutor?

tpeierls
Ravinder Singh wrote:
> I don't know if this is a bug or me just using it wrongly. But I add
> work to the pool, and I add the same Runnable each time. Since I try to
> avoid GC. But it seems that it doesn't process the full queue. If I
> create a new Runnable each time its ok.

There may be a bug in java.util.concurrent.ThreadPoolExecutor (and thus in the
backport TPE).

     public void execute(Runnable command) {
         for (;;) {
             ...
             if (workQueue.offer(command))
                 return;
             Runnable r = addIfUnderMaximumPoolSize(command);
             if (r == command)  // !!!
                 return;
             if (r == null) { /* reject */ }
             // else retry
         }
     }

On the line marked "!!!", there is an identity comparison. When you use the
same Runnable object, this test can succeed even though the command hasn't run.

However, it's not clear to me that TPE should have to support this kind of
thing. The workaround is simple: use separate Runnable instances.


>   static private int cSt, cWrk;
>
>   public static class Woerker implements Runnable
>   {
>       public void run()
>       {
>           cSt++;
>       }
>   }

This is not the main problem, but you are incrementing the cSt counter without
any kind of synchronization, so the cSt value you are seeing is suspect.

--tim

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

RE: Whats up with the ThreadPoolExecutor?

David Holmes
In reply to this post by Ravinder Singh-2
Ravinder,

> I don't know if this is a bug or me just using it wrongly. But I add
> work to the pool, and I add the same Runnable each time. Since I try to
> avoid GC. But it seems that it doesn't process the full queue. If I
> create a new Runnable each time its ok.

As Tim indicated this is indeed a bug, but one only exercised under
particular conditions:
 - there are >= core-size threads already in the pool
 - the queue is bounded and full
 - you've submitted the same Runnable multiple times

So change any of the above and you can work-around the problem.

There is an easy fix that Dawid can hopefully get into the backport in the
very near future:

- addIfUnderCorePoolSize needs to be changed to return an int <0, 0 or >0 to
represent the three cases it is checking for.

Cheers,
David Holmes

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Whats up with the ThreadPoolExecutor?

Doug Lea
David Holmes wrote:
>
> There is an easy fix that Dawid can hopefully get into the backport in the
> very near future:

And the fix for the java.util.concurrent version will with some luck be
in Mustang.

While we never expected anyone to reuse Runnable tasks
in this way, (and in general, it is not a good idea)
the specs did not say you cannot, so it is indeed a bug.

-Doug


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Whats up with the ThreadPoolExecutor?

Dawid Kurzyniec
In reply to this post by David Holmes
David Holmes wrote:

>Ravinder,
>
>  
>
>>I don't know if this is a bug or me just using it wrongly. But I add
>>work to the pool, and I add the same Runnable each time. Since I try to
>>avoid GC. But it seems that it doesn't process the full queue. If I
>>create a new Runnable each time its ok.
>>    
>>
>
>As Tim indicated this is indeed a bug, but one only exercised under
>particular conditions:
> - there are >= core-size threads already in the pool
> - the queue is bounded and full
> - you've submitted the same Runnable multiple times
>
>So change any of the above and you can work-around the problem.
>
>There is an easy fix that Dawid can hopefully get into the backport in the
>very near future:
>
>  
>
I will. Thanks.

BTW. the updated backport distribution, with fix of Condition variables
which used to not properly release multiple lock holds on await() can be
downloaded from the backport page - under "daily builds".

Regards,
Dawid


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Whats up with the ThreadPoolExecutor?

Gregg Wonderly-3
In reply to this post by Doug Lea
Doug Lea wrote:

> David Holmes wrote:
>
>>
>> There is an easy fix that Dawid can hopefully get into the backport in
>> the
>> very near future:
>
>
> And the fix for the java.util.concurrent version will with some luck be
> in Mustang.
>
> While we never expected anyone to reuse Runnable tasks
> in this way, (and in general, it is not a good idea)
> the specs did not say you cannot, so it is indeed a bug.

I've recently made changes to Timer to allow TimerTasks to be reused so that a TimerTask can reschedule the same object
into the future without a lot of garbage being generated for fast cycling tasks with odd repeating intervals.  I think
it makes a lot of sense to try and work towards reducing garbage generation by not requiring new objects.  There are, of
course some interesting issues, such as this one...

Gregg Wonderly
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Whats up with the ThreadPoolExecutor?

Joshua Bloch
It was not an accident that you can't reuse a finished timer task.  It
was a conscious design decision, based the principal that one should
minimize mutability (keep the state space of a object as simple as
possible).  In fact, I use it as an example of good API design in my
API design talk.  I would be very, very surprised if you could come up
with a realistic example of a program whose performance is noticeably
improved by reusing timer tasks.

            Josh

On 8/22/05, Gregg Wonderly <[hidden email]> wrote:

> Doug Lea wrote:
> > David Holmes wrote:
> >
> >>
> >> There is an easy fix that Dawid can hopefully get into the backport in
> >> the
> >> very near future:
> >
> >
> > And the fix for the java.util.concurrent version will with some luck be
> > in Mustang.
> >
> > While we never expected anyone to reuse Runnable tasks
> > in this way, (and in general, it is not a good idea)
> > the specs did not say you cannot, so it is indeed a bug.
>
> I've recently made changes to Timer to allow TimerTasks to be reused so that a TimerTask can reschedule the same object
> into the future without a lot of garbage being generated for fast cycling tasks with odd repeating intervals.  I think
> it makes a lot of sense to try and work towards reducing garbage generation by not requiring new objects.  There are, of
> course some interesting issues, such as this one...
>
> Gregg Wonderly
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
>

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Whats up with the ThreadPoolExecutor?

Ravinder Singh-2
In reply to this post by tpeierls
I don't think its suspect, since the integer is only incremented and
never deccremented, the value should allways be correct.

I have now corrected to use a separate Runnable for each task.

>>   static private int cSt, cWrk;
>>
>>   public static class Woerker implements Runnable
>>   {
>>       public void run()
>>       {
>>           cSt++;
>>       }
>>   }
>
>
> This is not the main problem, but you are incrementing the cSt counter
> without any kind of synchronization, so the cSt value you are seeing
> is suspect.
>
> --tim
>


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Whats up with the ThreadPoolExecutor?

Ravinder Singh-2
In reply to this post by Doug Lea
The reason I am doing it this way, is that our system processess
messages from a jms queue. And I am using a threadpool to faster process
messages simoultaneously. But if I somehow loose connection to the
message source, I must drop everything in the work queue.
By using the same Runnable which just picks messages from a linkedlist I
could just empty that list, when connection breaks.

I have allready reported the bug to Sun, and hopefully they will do
something about it.


Doug Lea wrote:

> David Holmes wrote:
>
>>
>> There is an easy fix that Dawid can hopefully get into the backport
>> in the
>> very near future:
>
>
> And the fix for the java.util.concurrent version will with some luck
> be in Mustang.
>
> While we never expected anyone to reuse Runnable tasks
> in this way, (and in general, it is not a good idea)
> the specs did not say you cannot, so it is indeed a bug.
>
> -Doug
>
>
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
>
>
>

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Whats up with the ThreadPoolExecutor?

Ravinder Singh-2
In reply to this post by Ravinder Singh-2
I take it back... sorry.. It does offcourse affect the value, if you
don't sync it.


Ravinder Singh wrote:

> I don't think its suspect, since the integer is only incremented and
> never deccremented, the value should allways be correct.
>
> I have now corrected to use a separate Runnable for each task.
>
>>>   static private int cSt, cWrk;
>>>
>>>   public static class Woerker implements Runnable
>>>   {
>>>       public void run()
>>>       {
>>>           cSt++;
>>>       }
>>>   }
>>
>>
>>
>> This is not the main problem, but you are incrementing the cSt
>> counter without any kind of synchronization, so the cSt value you are
>> seeing is suspect.
>>
>> --tim
>>
>
>
>
>

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Whats up with the ThreadPoolExecutor?

Doug Lea
In reply to this post by Ravinder Singh-2
Ravinder Singh wrote:
>
>
> I have allready reported the bug to Sun, and hopefully they will do
> something about it.
>

You must be new here :-)

While it is always a good idea to officially report bugs, we (the
ex-JSR166 folks) always work with Sun to get fixes in for anything in
java.util.concurrent, so reporting them on concurrency-interet list is
the most efficient path to resolution.

-Doug

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest