Hi Trustin,
Thanks for getting back to me. I know how busy one can get with e-mail on a
popular project. I appreciate your efforts to get back to everyone, even if it
takes some time. It is one of the things that will continue to grow the Mina
community.
Sorry the reply isn't quoted. Yahoo mail isn't doing that properly for some
reason lately.
> 1) Pass only a processorCount parameter and let SocketAcceptor use the most
> appropriate Executor internally. Even better, maybe not use an executor at
> all and just create a thread dedicated to each processorCount. Why bother
> with the overhead of a pool if the thread is basically dedicated to the IO
> processor?
I/O processor threads exit when there's no more channels to manage,
and start to run a new connection is added to it. Creating a new
thread is an expensive task, so providing an executor will improve the
performance. We started to utilize Executor interface because one of
the users wanted to control how the threads are created in a container
environment.
Ok, makes sense. I figured that was the reason. But perhaps you could add one
more constructor for people who want multiple IO threads but don't need control
over the Executors, like the one I proposed below.
> or
>
> 2) Pass only an Executor and if you want a fixed number of IO processors use
> Executor.newFixedThreadPool(nThreads).
>
> My thought is someone could do something like this:
>
> new SocketAcceptor(10, Executor.newFixedThreadPool(3));
>
> And that's probably really a bad idea.
True. it should be Executors.newFixedThreadPool(11). (10 I/O
processors and one acceptor thread).
You should probably add a note in the SocketAcceptor's constructor javadoc
about including a thread for the acceptor in addition to the IO processors. In
the application I am developing I used the same number for IO processors and
executor threads. When I set the IO processors to 1 the server didn't work and
I didn't understand why. Now it makes perfect sense.
new SocketAcceptor(1, Executor.newFixedThreadPool(1)); can never work, and
that's effecitvely what I had.
> If instead the constructors were changed to:
>
> public SocketAcceptor(){
> this(1);
> }
>
> public SocketAcceptor(int processorCount){
> this(processorCount, Executor.newFixedThreadPool(processorCount));
> }
Looks good so far.
Yep, except it should be:
public SocketAcceptor(int processorCount){
this(processorCount, Executor.newFixedThreadPool(processorCount + 1));
}
because the Acceptor thread is needed. Can you add this constructor to
SocketAcceptor? I bet most people would use this constructor instead of either
of the others.
> private SocketAcceptor(int processorCount, Executor executor){
> // existing code of constructor here.
> // or refactor code into SocketAcceptor(int processorCount) constructor
> }
>
> Things would be more bullet proof and prevent someone from making a mistake
> like I've shown above. Since the Executor (actually ExecutorService) is
> internal to the SocketAcceptor it would be responsible for calling
> ExecutorServiceImpl.shutdown(). This could be done in the doUnbind() method
> of SocketAcceptor.
>
> Failing such modifications, the default constructor for SocketAcceptor uses
> NewThreadExecutor(). Wouldn't it be more efficient to use
> Executor.newFixedThreadPool(1)?
It will be more efficient, but user will not have control over the
life cycle of the executor (thread pool). When should shutdown() of
the thread pool called? I think most users will need to control it.
An internal executor should be shut down when when unbind is called. That
could be handled easily within the doUnbind method of SocketAcceptor. All that
is needed is a flag to determine if doUnbind should shutdown the executor. I
would do something like this:
public SocketAcceptor(){
this(1);
}
// I would wager most people would use this constructor.
public SocketAcceptor(int processorCount){
this(1, Executor.newFixedThreadPool(processorCount + 1), true);
}
// for backwards compatibility - also add note that executor should have
processorCount +1 threads if fixed number of threads.
public SocketAcceptor(int processorCount, Executor executor){
this(1, Executor.newFixedThreadPool(processorCount + 1), false);
}
// for full control - add same note as above about fixed number of executor
threads.
public SocketAcceptor(int processorCount, Executor executor, boolean
shutdownOnUnbind){
this.shutdownOnUnbind = shutdownOnUnbind;
if(this.shutdownOnUnbind && ! (executor instanceof ExecutorService)){
throw new IllegalArgumentException("If shutdownOnUnbind is true the
executor must implement ExecutorService.");
}
// all other existing constructor code here.
}
//.....other code...
protected void doUnbind() throws IOException{
// all other existing doUnbind() code here.
if(this.shutdownOnUnbind && executor instanceof ExecutorService){
(ExecutorService (executor)).shutdown();
}
}
What do you think?
Rob
____________________________________________________________________________________Boardwalk
for $500? In 2007? Ha! Play Monopoly Here and Now (it's updated for today's
economy) at Yahoo! Games.
http://get.games.yahoo.com/proddesc?gamekey=monopolyherenow
