You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Trustin Lee <tr...@gmail.com> on 2007/11/08 20:30:58 UTC
A new ThreadPoolExecutor implementation - performs 20% better
Hi community,
I've just implemented a subclass of ThreadPoolExecutor that accepts
only IoEvents and IoFilterEvents and maintains the order of events by
itself (i.e. without any help of ExecutorFilter).
The result is impressive; UnorderedExecutorFilter +
OrderedThreadPoolFilter outperforms ExecutorFilter +
Executors.newFixedThreadPool() by apx 20% in my machine.
Please review the code:
http://svn.apache.org/viewvc/mina/trunk/core/src/main/java/org/apache/mina/filter/executor/OrderedThreadPoolExecutor.java?view=markup
One limitation is that you cannot specify the Queue implementation.
Except that, I think I implemented all features of the
ThreadPoolExecutor.
So, what do you think about:
* Removing ExecutorFilter,
* merging AbstractExecutorFilter and UnorderedExecutorFilter into the
new ExecutorFilter
* and letting it use OrderedThreadPoolExecutor by default?
I think most users won't need to specify other executor than
OrderedThreadPoolExecutor and providing many convenience constructors
in ExecutorFilter will compensate the possible breakage of backward
compatibility.
If there's no objection, let me go ahead.
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6
Re: A new ThreadPoolExecutor implementation - performs 20% better
Posted by Mark <el...@gmail.com>.
Absolutely!
By my calculations, 20% of 'really fast' is 'super fast' :)
On Nov 8, 2007 2:30 PM, Trustin Lee <tr...@gmail.com> wrote:
> Hi community,
>
> I've just implemented a subclass of ThreadPoolExecutor that accepts
> only IoEvents and IoFilterEvents and maintains the order of events by
> itself (i.e. without any help of ExecutorFilter).
>
> The result is impressive; UnorderedExecutorFilter +
> OrderedThreadPoolFilter outperforms ExecutorFilter +
> Executors.newFixedThreadPool() by apx 20% in my machine.
>
> Please review the code:
>
> http://svn.apache.org/viewvc/mina/trunk/core/src/main/java/org/apache/mina/filter/executor/OrderedThreadPoolExecutor.java?view=markup
>
> One limitation is that you cannot specify the Queue implementation.
> Except that, I think I implemented all features of the
> ThreadPoolExecutor.
>
> So, what do you think about:
>
> * Removing ExecutorFilter,
> * merging AbstractExecutorFilter and UnorderedExecutorFilter into the
> new ExecutorFilter
> * and letting it use OrderedThreadPoolExecutor by default?
>
> I think most users won't need to specify other executor than
> OrderedThreadPoolExecutor and providing many convenience constructors
> in ExecutorFilter will compensate the possible breakage of backward
> compatibility.
>
> If there's no objection, let me go ahead.
>
> Trustin
> --
> what we call human nature is actually human habit
> --
> http://gleamynode.net/
> --
> PGP Key ID: 0x0255ECA6
>
--
--------------------------------
The adjuration to be "normal" seems shockingly repellent to me; I see
neither hope nor comfort in sinking to that low level. I think it is
ignorance that makes people think of abnormality only with horror and
allows them to remain undismayed at the proximity of "normal" to
average and mediocre. For surely anyone who achieves anything is,
essentially, abnormal.
Dr. Karl Menninger
Re: A new ThreadPoolExecutor implementation - performs 20% better
Posted by Trustin Lee <tr...@gmail.com>.
Now I see what you mean. :) What you say makes a lot of sense. I
applied the same change to UnorderedThreadPoolExecutor
Cheers,
Trustin
On Nov 10, 2007 12:35 PM, Mike Heath <mh...@apache.org> wrote:
> Trustin Lee wrote:
> > corePoolSize is a configuration property that represent the minimum
> > size of the pool. So, I think updating corePoolSize property doesn't
> > make sense. Am I missing something?
>
> With my suggestion, OrderedThreadPoolExecutor would only change
> corePoolSize if corePoolSize > maximumCorePoolSize. I just updated and
> noticed that you added a check that throws an IllegalArgumentException
> if maximumPoolSize < corePoolSize which is exactly what
> ThreadPoolExecutor does. I don't have a problem with this approach.
> (Besides, who am I to argue with Doug Lea?)
>
> On the flip side, it makes sense in my mind to not allow the
> corePoolSize to exceed the maximumPoolSize. TheadPoolExecutor doesn't
> check for this but I don't see why it shouldn't. I added this check to
> OrderedThreadPoolExecutor.setCorePoolSize
>
> This is just me being picky but setMaximumPoolSize should also only call
> removeWorker if workers.size() > maximumPoolSize. So, if my
> maximumPoolSize is 100 and I change it to 50 and I only have 5 worker
> threads, it doesn't call removeWorker() 50 times (which wouldn't really
> matter since removeWorker() won't actually do anything in this case). I
> changed that too.
>
> This is the part where you say, "Look, setMaximumPoolSize and
> setCorePoolSize will probably never even get used. Mike, stop being
> such a pedant and go find something productive to do."
>
> -Mike
>
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6
Re: A new ThreadPoolExecutor implementation - performs 20% better
Posted by Mike Heath <mh...@apache.org>.
Trustin Lee wrote:
> corePoolSize is a configuration property that represent the minimum
> size of the pool. So, I think updating corePoolSize property doesn't
> make sense. Am I missing something?
With my suggestion, OrderedThreadPoolExecutor would only change
corePoolSize if corePoolSize > maximumCorePoolSize. I just updated and
noticed that you added a check that throws an IllegalArgumentException
if maximumPoolSize < corePoolSize which is exactly what
ThreadPoolExecutor does. I don't have a problem with this approach.
(Besides, who am I to argue with Doug Lea?)
On the flip side, it makes sense in my mind to not allow the
corePoolSize to exceed the maximumPoolSize. TheadPoolExecutor doesn't
check for this but I don't see why it shouldn't. I added this check to
OrderedThreadPoolExecutor.setCorePoolSize
This is just me being picky but setMaximumPoolSize should also only call
removeWorker if workers.size() > maximumPoolSize. So, if my
maximumPoolSize is 100 and I change it to 50 and I only have 5 worker
threads, it doesn't call removeWorker() 50 times (which wouldn't really
matter since removeWorker() won't actually do anything in this case). I
changed that too.
This is the part where you say, "Look, setMaximumPoolSize and
setCorePoolSize will probably never even get used. Mike, stop being
such a pedant and go find something productive to do."
-Mike
Re: A new ThreadPoolExecutor implementation - performs 20% better
Posted by Trustin Lee <tr...@gmail.com>.
On Nov 9, 2007 6:08 AM, Mike Heath <mh...@apache.org> wrote:
> Impressive. +1 A 20% performance increase is amazing. Way to go!
I've just finished refactoring. :)
> I'm curious. What was causing the bottleneck with ExecutorFilter +
> Executors.newFixedThreadPool() that UnorderedExecutorFilter +
> OrderedThreadPoolExecutor overcomes? Did you find this bottleneck using
> a profiler?
Actually I didn't run any profiler because the introduction of
OrderedThreadPoolExecutor was to provide event throttling for those
who don't need such a complicated throttling that
org.apache.mina.filter.traffic provides. Performance improvement was
just an accident(?). :D
> After looking over OrderedThreadPoolExecutor, the only suggestion I have
> is to modify setMaximumPoolSize(int maximumPoolSize). Currently if the
> new maximumPoolSize is smaller then the actual maximumPoolSize, it
> invokes removeWorker() multiple times based on the difference.
> Shouldn't it really look something like:
>
> @Override
> public void setMaximumPoolSize(int maximumPoolSize) {
> synchronized (workers) {
> if (maximumPoolSize > corePoolSize) {
> setCorePoolSize(maximumPoolSize);
> }
> this.maximumPoolSize = maximumPoolSize;
> }
> }
>
> This way the corePoolSize field will get updated if need be and
> setCorePoolSize will update the the workers size if need be. I don't
> have much experience with the implementations of ThreadPoolExecutors so
> let me know if I'm off base.
corePoolSize is a configuration property that represent the minimum
size of the pool. So, I think updating corePoolSize property doesn't
make sense. Am I missing something?
Cheers,
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6
Re: A new ThreadPoolExecutor implementation - performs 20% better
Posted by Mike Heath <mh...@apache.org>.
Impressive. +1 A 20% performance increase is amazing. Way to go!
I'm curious. What was causing the bottleneck with ExecutorFilter +
Executors.newFixedThreadPool() that UnorderedExecutorFilter +
OrderedThreadPoolExecutor overcomes? Did you find this bottleneck using
a profiler?
After looking over OrderedThreadPoolExecutor, the only suggestion I have
is to modify setMaximumPoolSize(int maximumPoolSize). Currently if the
new maximumPoolSize is smaller then the actual maximumPoolSize, it
invokes removeWorker() multiple times based on the difference.
Shouldn't it really look something like:
@Override
public void setMaximumPoolSize(int maximumPoolSize) {
synchronized (workers) {
if (maximumPoolSize > corePoolSize) {
setCorePoolSize(maximumPoolSize);
}
this.maximumPoolSize = maximumPoolSize;
}
}
This way the corePoolSize field will get updated if need be and
setCorePoolSize will update the the workers size if need be. I don't
have much experience with the implementations of ThreadPoolExecutors so
let me know if I'm off base.
-Mike
Trustin Lee wrote:
> Hi community,
>
> I've just implemented a subclass of ThreadPoolExecutor that accepts
> only IoEvents and IoFilterEvents and maintains the order of events by
> itself (i.e. without any help of ExecutorFilter).
>
> The result is impressive; UnorderedExecutorFilter +
> OrderedThreadPoolFilter outperforms ExecutorFilter +
> Executors.newFixedThreadPool() by apx 20% in my machine.
>
> Please review the code:
>
> http://svn.apache.org/viewvc/mina/trunk/core/src/main/java/org/apache/mina/filter/executor/OrderedThreadPoolExecutor.java?view=markup
>
> One limitation is that you cannot specify the Queue implementation.
> Except that, I think I implemented all features of the
> ThreadPoolExecutor.
>
> So, what do you think about:
>
> * Removing ExecutorFilter,
> * merging AbstractExecutorFilter and UnorderedExecutorFilter into the
> new ExecutorFilter
> * and letting it use OrderedThreadPoolExecutor by default?
>
> I think most users won't need to specify other executor than
> OrderedThreadPoolExecutor and providing many convenience constructors
> in ExecutorFilter will compensate the possible breakage of backward
> compatibility.
>
> If there's no objection, let me go ahead.
>
> Trustin