You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Michael <fr...@adsl-209-204-165-151.sonic.net> on 2002/12/24 08:25:52 UTC
[PATCH] Re: implement ThreadPool with GenericObjectPool?
Costin Manolache wrote:
>The main reason I'm cautious - this is a very critical piece of code.
>
I also value stability (and more than performance). I'm tired of backing
out and waiting to upgrade TC releases. (4.1.16 works for me, BTW.)
>>the debugging feature (JMX support) you're adding.
>>
>>
>JMX is not a debugging feature - it's monitoring/control. It would allow
>people to see how many threads are used, to eventually stop threads or
>change the parameters dynamically.
>
I know what JMX itself is--I'm going by the check-in comment:
I want to add JMX support and make calls to store the
"state" of the thread, i.e. what is the thread doing.
This would allow people to debug hunged threads and
allow more control.
Perhaps I misunderstood this to mean debugging of the TC code, rather
than the servlet's.
In any case, ThreadPool will at least need this patch, since
removeThread() is never called. I also can't imagine why addThread() and
removeThread() would need to be more than "private", given their role.
Other liberal access modifiers abound.
Also: MonitorRunnable, now that it has a start() method (for JMX, I
presume) has no restriction on running more than one thread. This
undercuts the meaning of "interval", and makes stop() poorly defined.
Re: [PATCH] Re: ThreadPool
Posted by Michael <fr...@adsl-209-204-165-151.sonic.net>.
Remy Maucherat wrote:
> Although refactoring the monitor timer code is relatively safe, the
> thread pool as a whole is a critical piece of code in Tomcat 4.1.x (=
> you should test things extensively after making a change).
>
> Since your patch doesn't build, I'll have to assume you didn't try to
> *run* with it either ;-)
I know, I know. I intended to build *and* test it. I don't know why I
pushed "send". I shouldn't be up this late.
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Remy Maucherat <re...@apache.org>.
Michael wrote:
> Umm...that last patch won't compile, I forgot to remove a line. Knew I
> was forgetting to do something. :-[
Although refactoring the monitor timer code is relatively safe, the
thread pool as a whole is a critical piece of code in Tomcat 4.1.x (=
you should test things extensively after making a change).
Since your patch doesn't build, I'll have to assume you didn't try to
*run* with it either ;-)
Remy
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Michael <fr...@adsl-209-204-165-151.sonic.net>.
Umm...that last patch won't compile, I forgot to remove a line. Knew I
was forgetting to do something. :-[
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Costin Manolache <cm...@yahoo.com>.
Remy Maucherat wrote:
> BTW, can't Linux 2.5/2.6 handle thousands of threads without any problem
> ? I remember reading an iterview of Ingo who said JVM performance and
> thread handling should be way better in 2.6.
> The idea is that there's nothing wrong with designing with threads in
> mind (I think that's good design), but the problem is that it doesn't
> quite work too well with most current OSes.
I'm pretty sure most OSes and apps will have problems with thousands of
threads ( especially if a many are in RUN state ). They did a lot
of changes in the linux scheduler - so hopefully thread handling improved
too.
I'm not very sure "normal" one request/thread will scale well for
1000s of concurent requests. That's an area where NIO would help
( select ).
Squid ( and few other servers ) uses a single thread plus select plus
some tricks to deal with extremely high load.
>> I agree. We could pretty easily provide JMX-based operations for
>> everything that manager does (so that they're accessible from a JMX-based
>> client), and have the manager webapp itself just be wrappers around those
>> same MBean operations (for easy integration into non-JMX clients that can
>> perform HTTP requests).
>
> I added (theorical) support for MC4J in 5.0 some time ago, BTW. The only
> problem is that there's a version clash with MX4J (Tomcat has to use an
> API which changed between MX4J 1.1 and 1.1.1 to start the JRMP
> connector, and MC4J only works with MX4J 1.1 at the moment).
> After hacking to get it to work, in order to test the feature, I noticed
> the start/stop/reload operations weren't declared on the model MBeans.
I added a reload for Context in the model mbean some time ago, and it
seemed to work.
For "remote" jmx - yes, MX4J RMI should work ( I was able to use it ),
but it's very tricky.
I think adding some simple operations in the ManagerServlet would be
a good idea - just a get/set for a single attribute and a simple
no-param method invocation would cover a lot of use cases.
And we can easily implement an JMX-over-http on top of this.
Costin
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by "Jerome Lacoste (Frisurf)" <la...@frisurf.no>.
> BTW, can't Linux 2.5/2.6 handle thousands of threads without any problem
> ? I remember reading an iterview of Ingo who said JVM performance and
> thread handling should be way better in 2.6.
You're right, and Java should be one to benefit well from it.
But if the kernel period freeze lasts as long as the last one, we won't
have 2.6.0 before 10 months, and give it another 4-5 months before it
gets used in mainline distrib (last Debian uses 2.2 by default if I am
correct :))
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Remy Maucherat <re...@apache.org>.
Craig R. McClanahan wrote:
>
> On Mon, 30 Dec 2002, Costin Manolache wrote:
>
>>I agree.
>>
>>Is 'reloadabl' disabled by default ?
>
>
> Yes ... at least in HEAD of 4.1 and 5.0. I'm pretty sure it's always been
> that way.
Yes, it's disabled by default.
BTW, can't Linux 2.5/2.6 handle thousands of threads without any problem
? I remember reading an iterview of Ingo who said JVM performance and
thread handling should be way better in 2.6.
The idea is that there's nothing wrong with designing with threads in
mind (I think that's good design), but the problem is that it doesn't
quite work too well with most current OSes.
>>BTW - another way to do reload-on-demand is using JMX ( the html interface
>>provided by either jmx-ri or mx4j - or any other jmx adaptor ). It may be
>>a good idea to make the manager webapp more powerfull - i.e. support generic
>>( simple ) get/set operations via JMX.
>>
>
>
> I agree. We could pretty easily provide JMX-based operations for
> everything that manager does (so that they're accessible from a JMX-based
> client), and have the manager webapp itself just be wrappers around those
> same MBean operations (for easy integration into non-JMX clients that can
> perform HTTP requests).
I added (theorical) support for MC4J in 5.0 some time ago, BTW. The only
problem is that there's a version clash with MX4J (Tomcat has to use an
API which changed between MX4J 1.1 and 1.1.1 to start the JRMP
connector, and MC4J only works with MX4J 1.1 at the moment).
After hacking to get it to work, in order to test the feature, I noticed
the start/stop/reload operations weren't declared on the model MBeans.
Remy
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by "Craig R. McClanahan" <cr...@apache.org>.
On Mon, 30 Dec 2002, Costin Manolache wrote:
>
> >> As for someone stoping the check for class modification at run time using
> >> an admin interface - what's wrong with that ?
> >
> > I don't see a problem with that, but I've stopped using "reloadable" at
> > all for my development -- reload-on-demand (via the manager webapp) is a
> > much more effective strategy IMHO. And "reloadable" shouldn't be used on
> > a production server anyway.
>
> I agree.
>
> Is 'reloadabl' disabled by default ?
Yes ... at least in HEAD of 4.1 and 5.0. I'm pretty sure it's always been
that way.
> Should we deprecate it ?
>
Or perhaps (in 5.0) convert it to an installable module rather than a core
feature?
> BTW - another way to do reload-on-demand is using JMX ( the html interface
> provided by either jmx-ri or mx4j - or any other jmx adaptor ). It may be
> a good idea to make the manager webapp more powerfull - i.e. support generic
> ( simple ) get/set operations via JMX.
>
I agree. We could pretty easily provide JMX-based operations for
everything that manager does (so that they're accessible from a JMX-based
client), and have the manager webapp itself just be wrappers around those
same MBean operations (for easy integration into non-JMX clients that can
perform HTTP requests).
>
> Costin
>
Craig
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Costin Manolache <cm...@yahoo.com>.
Craig R. McClanahan wrote:
>> It would actually be a pretty good solution IMO. A single thread checking
>> the files for modification in all contexts is better ( again IMO ) than
>> one thread per context. Same for checking sessions.
>>
>> The coding is a bit trickier ( and there are some issues related with DOS
>> and security ), but it may be worth it.
>>
>
> A single thread checking for modified files is probably ok (although it
> will slow down the effective reload rate for individual apps), but I'd be
> particularly careful about trying to combine the session timeout
> processing of multiple webapps into one thread. The problem is that
> application-level event handlers are called from this thread, and you
> really don't want an accidentally or maliciously coded webapp to start
> sleeping in one of these event handlers and thereby disable session
> timeout service for the entire Tomcat JVM.
>
> It's possible that some way to time out the timeout event hander :-) could
> deal with this, but it will require some care.
I know - executing the even handlers can't be done on the expiration
thread.
The problem I'm trying to solve is scaling tomcat with the number of
webapps. ( well, I'm not actually coding anything right now - there are
more important issues :-)
A hosting site with 100s of apps ( some very infrequently used !) would
benefit a lot from having a single thread doing the checks for expiration.
A thread pool can be used to process the event handlers - with some
safeguards to prevent DOS ( event handlers hunging the expiration thread).
It's not very difficult - let the thread pool grow to the max number of
apps, and make sure a each context gets at most one thread for events. If
all apps behave, you'll have few threads doing the expiration and one doing
the check.
Regarding the delay - yes, it may be faster to have 100 threads doing
nothing but expiration ( or modification ) check, one per context. However
it may be better to have 100 threads doing request processing and keep
a single thread doing session maintainance. Java GC is done in a single
thread too.
Anyway - I don't think this is a very urgent problem to solve.
>> As for someone stoping the check for class modification at run time using
>> an admin interface - what's wrong with that ?
>
> I don't see a problem with that, but I've stopped using "reloadable" at
> all for my development -- reload-on-demand (via the manager webapp) is a
> much more effective strategy IMHO. And "reloadable" shouldn't be used on
> a production server anyway.
I agree.
Is 'reloadabl' disabled by default ? Should we deprecate it ?
BTW - another way to do reload-on-demand is using JMX ( the html interface
provided by either jmx-ri or mx4j - or any other jmx adaptor ). It may be
a good idea to make the manager webapp more powerfull - i.e. support generic
( simple ) get/set operations via JMX.
Costin
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Remy Maucherat <re...@apache.org>.
Costin Manolache wrote:
> Craig R. McClanahan wrote:
>
>
>
>>I like using the custom Ant tasks included with Tomcat 4.1 for
>>manipulation via manager, so that I can create a "reload" target. If you
>>don't use Ant, it's just as easy to leave a browser window open to:
>
>
> I don't know if you looked at the JMX ant tasks in modeler.
>
> If we add some "simple remote JMX" feature in Manager - and implement
> the client side in the modeler ant tasks - it should be possible to
> control every aspect of tomcat from ant.
I think the client part should just use JRMP to talk to MX4J (or one of
the others it provides).
Ok, I'm being lazy here; I just don't feel like inventing our own custom
protocol when one already exists ;-)
Remy
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by "Craig R. McClanahan" <cr...@apache.org>.
On Mon, 30 Dec 2002, Costin Manolache wrote:
> Date: Mon, 30 Dec 2002 13:02:09 -0800
> From: Costin Manolache <cm...@yahoo.com>
> Reply-To: Tomcat Developers List <to...@jakarta.apache.org>
> To: tomcat-dev@jakarta.apache.org
> Subject: Re: [PATCH] Re: ThreadPool
>
> Craig R. McClanahan wrote:
>
>
> > I like using the custom Ant tasks included with Tomcat 4.1 for
> > manipulation via manager, so that I can create a "reload" target. If you
> > don't use Ant, it's just as easy to leave a browser window open to:
>
> I don't know if you looked at the JMX ant tasks in modeler.
>
I saw the commits, but not the code yet. It's on my list of stuff to look
at.
> If we add some "simple remote JMX" feature in Manager - and implement
> the client side in the modeler ant tasks - it should be possible to
> control every aspect of tomcat from ant.
>
>
Sounds like a reasonable way to do things.
> Costin
Craig
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Costin Manolache <cm...@yahoo.com>.
Craig R. McClanahan wrote:
> I like using the custom Ant tasks included with Tomcat 4.1 for
> manipulation via manager, so that I can create a "reload" target. If you
> don't use Ant, it's just as easy to leave a browser window open to:
I don't know if you looked at the JMX ant tasks in modeler.
If we add some "simple remote JMX" feature in Manager - and implement
the client side in the modeler ant tasks - it should be possible to
control every aspect of tomcat from ant.
Costin
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by "Craig R. McClanahan" <cr...@apache.org>.
On Mon, 30 Dec 2002, Joseph Shraibman wrote:
> Date: Mon, 30 Dec 2002 14:00:57 -0500
> From: Joseph Shraibman <jk...@selectacast.net>
> Reply-To: Tomcat Developers List <to...@jakarta.apache.org>
> To: Tomcat Developers List <to...@jakarta.apache.org>
> Subject: Re: [PATCH] Re: ThreadPool
>
> Craig R. McClanahan wrote:
> >
>
> >
> > I don't see a problem with that, but I've stopped using "reloadable" at
> > all for my development -- reload-on-demand (via the manager webapp) is a
> > much more effective strategy IMHO. And "reloadable" shouldn't be used on
> > a production server anyway.
> >
> Is there a way to do that from the admin app as well? If there is I
> haven't found it.
>
No, there's not, although in principle it would be possible to do this.
I like using the custom Ant tasks included with Tomcat 4.1 for
manipulation via manager, so that I can create a "reload" target. If you
don't use Ant, it's just as easy to leave a browser window open to:
http://localhost:8080/manager/reload?path=/myapp
and just click "Reload" whenever you want. The first time you do this
it'll ask you to log on (via BASIC authentication) -- after that, it's
just a single click away.
Craig
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Joseph Shraibman <jk...@selectacast.net>.
Craig R. McClanahan wrote:
>
>
> I don't see a problem with that, but I've stopped using "reloadable" at
> all for my development -- reload-on-demand (via the manager webapp) is a
> much more effective strategy IMHO. And "reloadable" shouldn't be used on
> a production server anyway.
>
Is there a way to do that from the admin app as well? If there is I haven't found it.
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by "Craig R. McClanahan" <cr...@apache.org>.
On Sun, 29 Dec 2002, Costin Manolache wrote:
>
> Michael wrote:
>
> > Costin Manolache wrote:
[snips]
> >> IMO MonitorRunnable, the session expiration thread ( one per ctx )
> >> and the reloading thread ( one per ctx again )
> >
> > What classes are you referring to specifically? StandardManager &
> > WebappLoader?
>
> Yes, they add 2 threads per webapp. Which may limit the scalability with
> the number of deployed webapps ( 100 contexts -> 200 threads ).
>
> >> should all go away and
> >> be replaced by a single mechanism. Maybe the JMX timer ?
> >
> > I'd hate to think what would happen if someone stopped the timer
> > service. Also, using the event-driven approach to trigger maintenance
> > activities in foreground threads would be quirky coding, and may not
> > even be appropriate in classes that don't always have executing code.
>
> It would actually be a pretty good solution IMO. A single thread checking
> the files for modification in all contexts is better ( again IMO ) than
> one thread per context. Same for checking sessions.
>
> The coding is a bit trickier ( and there are some issues related with DOS
> and security ), but it may be worth it.
>
A single thread checking for modified files is probably ok (although it
will slow down the effective reload rate for individual apps), but I'd be
particularly careful about trying to combine the session timeout
processing of multiple webapps into one thread. The problem is that
application-level event handlers are called from this thread, and you
really don't want an accidentally or maliciously coded webapp to start
sleeping in one of these event handlers and thereby disable session
timeout service for the entire Tomcat JVM.
It's possible that some way to time out the timeout event hander :-) could
deal with this, but it will require some care.
> As for someone stoping the check for class modification at run time using
> an admin interface - what's wrong with that ?
I don't see a problem with that, but I've stopped using "reloadable" at
all for my development -- reload-on-demand (via the manager webapp) is a
much more effective strategy IMHO. And "reloadable" shouldn't be used on
a production server anyway.
Craig
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Costin Manolache <cm...@yahoo.com>.
Michael wrote:
> Costin Manolache wrote:
>
>> You're probably right that addThread() doesn't need to be public - but
>> I don't think it hurts too much either.
>
> It does when you're trying to refactor a class. It forces you to ask
> whether the interface really is used, or can you rearrange like you want
> to. Usually I grep the sources to determine that, but when you're using
> reflection/introspection (e.g., JMX), then there's no telling. You have
> to take an educated guess and hope that you don't break things.
>
> It also prevents the byte code compiler from doing complete, inlined
> optimization when a member is non-private.
That's one of the worst reasons to make a method private. You make a method
public or protected if it may provide something usefull. You make it private
if it is strictly implementation and shouldn't be called from outside.
There are a lot of things you need to design in order to get good
performance, but making methods private just for that shouldn't be one of
them.
>> IMO MonitorRunnable, the session expiration thread ( one per ctx )
>> and the reloading thread ( one per ctx again )
>
> What classes are you referring to specifically? StandardManager &
> WebappLoader?
Yes, they add 2 threads per webapp. Which may limit the scalability with
the number of deployed webapps ( 100 contexts -> 200 threads ).
>> should all go away and
>> be replaced by a single mechanism. Maybe the JMX timer ?
>
> I'd hate to think what would happen if someone stopped the timer
> service. Also, using the event-driven approach to trigger maintenance
> activities in foreground threads would be quirky coding, and may not
> even be appropriate in classes that don't always have executing code.
It would actually be a pretty good solution IMO. A single thread checking
the files for modification in all contexts is better ( again IMO ) than
one thread per context. Same for checking sessions.
The coding is a bit trickier ( and there are some issues related with DOS
and security ), but it may be worth it.
As for someone stoping the check for class modification at run time using
an admin interface - what's wrong with that ?
> So I'd opt for java.util.Timer. It requires J2SE 1.3+...is that a
> problem? Here's what it looks like:
What's the benefit ? More fun ?
> * Replace MonitorRunnable with java.util.Timer.
> * The Timer thread is always daemonized; I couldn't understand why
> MonitorRunnable would conditionally do so based on ThreadPool.
> * Unlike the start/stop() MonitorRunnable i/f--which I assumed was
> for activating the new wait() interval--the Timer can never be
> stopped w/o shutting down the ThreadPool.
Sorry, I don't think I'll go with this change.
I'm sure MonitorRunnable can be improved a lot - but for now I don't
see any bugs you'll fix or any real performance improvement.
I love refactoring code - but usually with a goal :-)
Costin
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Michael <fr...@adsl-209-204-165-151.sonic.net>.
Remy Maucherat wrote:
>>>> Also: MonitorRunnable, now that it has a start() method (for JMX, I
>>>> presume) has no restriction on running more than one thread. This
>>>> undercuts the meaning of "interval", and makes stop() poorly defined.
>>>
> I would like it if this was fixing any issue
the above (which hasn't made it into a release yet)
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Bill Barker <wb...@wilshire.com>.
----- Original Message -----
From: "Remy Maucherat" <re...@apache.org>
To: "Tomcat Developers List" <to...@jakarta.apache.org>
Sent: Saturday, December 28, 2002 12:40 AM
Subject: Re: [PATCH] Re: ThreadPool
> Michael wrote:
> > Costin Manolache wrote:
> >
> >> You're probably right that addThread() doesn't need to be public - but
> >> I don't think it hurts too much either.
> >
> >
> > It does when you're trying to refactor a class. It forces you to ask
> > whether the interface really is used, or can you rearrange like you want
> > to. Usually I grep the sources to determine that, but when you're using
> > reflection/introspection (e.g., JMX), then there's no telling. You have
> > to take an educated guess and hope that you don't break things.
> >
> > It also prevents the byte code compiler from doing complete, inlined
> > optimization when a member is non-private.
> >
> >>> Also: MonitorRunnable, now that it has a start() method (for JMX, I
> >>> presume) has no restriction on running more than one thread. This
> >>> undercuts the meaning of "interval", and makes stop() poorly defined.
> >>
> >>
> >>
> >> IMO MonitorRunnable, the session expiration thread ( one per ctx )
> >> and the reloading thread ( one per ctx again )
> >
> >
> > What classes are you referring to specifically? StandardManager &
> > WebappLoader?
> >
> >> should all go away and
> >> be replaced by a single mechanism. Maybe the JMX timer ?
> >
> >
> > I'd hate to think what would happen if someone stopped the timer
> > service. Also, using the event-driven approach to trigger maintenance
> > activities in foreground threads would be quirky coding, and may not
> > even be appropriate in classes that don't always have executing code.
> >
> > So I'd opt for java.util.Timer. It requires J2SE 1.3+...is that a
> > problem? Here's what it looks like:
> >
> > * Replace MonitorRunnable with java.util.Timer.
> > * The Timer thread is always daemonized; I couldn't understand why
> > MonitorRunnable would conditionally do so based on ThreadPool.
> > * Unlike the start/stop() MonitorRunnable i/f--which I assumed was
> > for activating the new wait() interval--the Timer can never be
> > stopped w/o shutting down the ThreadPool.
>
> If I were you, I would pick a less critical section to start playing
> with, so that we get to know your code, if you know what I mean.
>
> Also, I would like it if this was fixing any issue or actually improving
> performance, rather than just beautifying the code, which is not
desirable.
Not to mention that this section of code is used by TC 3.3. I'll '-1' the
patch for that reason, since it won't work for a 1.1.x JVM (as TC 3.3
currently requires).
>
> Remy
>
>
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] Re: ThreadPool
Posted by Remy Maucherat <re...@apache.org>.
Michael wrote:
> Costin Manolache wrote:
>
>> You're probably right that addThread() doesn't need to be public - but
>> I don't think it hurts too much either.
>
>
> It does when you're trying to refactor a class. It forces you to ask
> whether the interface really is used, or can you rearrange like you want
> to. Usually I grep the sources to determine that, but when you're using
> reflection/introspection (e.g., JMX), then there's no telling. You have
> to take an educated guess and hope that you don't break things.
>
> It also prevents the byte code compiler from doing complete, inlined
> optimization when a member is non-private.
>
>>> Also: MonitorRunnable, now that it has a start() method (for JMX, I
>>> presume) has no restriction on running more than one thread. This
>>> undercuts the meaning of "interval", and makes stop() poorly defined.
>>
>>
>>
>> IMO MonitorRunnable, the session expiration thread ( one per ctx )
>> and the reloading thread ( one per ctx again )
>
>
> What classes are you referring to specifically? StandardManager &
> WebappLoader?
>
>> should all go away and
>> be replaced by a single mechanism. Maybe the JMX timer ?
>
>
> I'd hate to think what would happen if someone stopped the timer
> service. Also, using the event-driven approach to trigger maintenance
> activities in foreground threads would be quirky coding, and may not
> even be appropriate in classes that don't always have executing code.
>
> So I'd opt for java.util.Timer. It requires J2SE 1.3+...is that a
> problem? Here's what it looks like:
>
> * Replace MonitorRunnable with java.util.Timer.
> * The Timer thread is always daemonized; I couldn't understand why
> MonitorRunnable would conditionally do so based on ThreadPool.
> * Unlike the start/stop() MonitorRunnable i/f--which I assumed was
> for activating the new wait() interval--the Timer can never be
> stopped w/o shutting down the ThreadPool.
If I were you, I would pick a less critical section to start playing
with, so that we get to know your code, if you know what I mean.
Also, I would like it if this was fixing any issue or actually improving
performance, rather than just beautifying the code, which is not desirable.
Remy
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
[PATCH] Re: ThreadPool
Posted by Michael <fr...@adsl-209-204-165-151.sonic.net>.
Costin Manolache wrote:
> You're probably right that addThread() doesn't need to be public - but
> I don't think it hurts too much either.
It does when you're trying to refactor a class. It forces you to ask
whether the interface really is used, or can you rearrange like you want
to. Usually I grep the sources to determine that, but when you're using
reflection/introspection (e.g., JMX), then there's no telling. You have
to take an educated guess and hope that you don't break things.
It also prevents the byte code compiler from doing complete, inlined
optimization when a member is non-private.
>> Also: MonitorRunnable, now that it has a start() method (for JMX, I
>> presume) has no restriction on running more than one thread. This
>> undercuts the meaning of "interval", and makes stop() poorly defined.
>
>
> IMO MonitorRunnable, the session expiration thread ( one per ctx )
> and the reloading thread ( one per ctx again )
What classes are you referring to specifically? StandardManager &
WebappLoader?
> should all go away and
> be replaced by a single mechanism. Maybe the JMX timer ?
I'd hate to think what would happen if someone stopped the timer
service. Also, using the event-driven approach to trigger maintenance
activities in foreground threads would be quirky coding, and may not
even be appropriate in classes that don't always have executing code.
So I'd opt for java.util.Timer. It requires J2SE 1.3+...is that a
problem? Here's what it looks like:
* Replace MonitorRunnable with java.util.Timer.
* The Timer thread is always daemonized; I couldn't understand why
MonitorRunnable would conditionally do so based on ThreadPool.
* Unlike the start/stop() MonitorRunnable i/f--which I assumed was
for activating the new wait() interval--the Timer can never be
stopped w/o shutting down the ThreadPool.
Re: [PATCH] Re: implement ThreadPool with GenericObjectPool?
Posted by Costin Manolache <cm...@yahoo.com>.
Michael wrote:
>>> the debugging feature (JMX support) you're adding.
>>>
>>
>> JMX is not a debugging feature - it's monitoring/control. It would
>> allow people to see how many threads are used, to eventually stop
>> threads or change the parameters dynamically.
>
> I know what JMX itself is--I'm going by the check-in comment:
>
> I want to add JMX support and make calls to store the
> "state" of the thread, i.e. what is the thread doing.
> This would allow people to debug hunged threads and
> allow more control.
>
>
> Perhaps I misunderstood this to mean debugging of the TC code, rather
> than the servlet's.
I was thinking more of user code debugging. I hope tomcat is not hunging
too much :-) It looks like a nice feature - like the status page in
apache ( that shows you all requests that are active ).
> In any case, ThreadPool will at least need this patch, since
> removeThread() is never called. I also can't imagine why addThread() and
> removeThread() would need to be more than "private", given their role.
> Other liberal access modifiers abound.
I'll apply your patch, thanks.
You're probably right that addThread() doesn't need to be public - but
I don't think it hurts too much either.
> Also: MonitorRunnable, now that it has a start() method (for JMX, I
> presume) has no restriction on running more than one thread. This
> undercuts the meaning of "interval", and makes stop() poorly defined.
IMO MonitorRunnable, the session expiration thread ( one per ctx )
and the reloading thread ( one per ctx again ) should all go away and
be replaced by a single mechanism. Maybe the JMX timer ?
I took a look at GenericObjectPool and all associated classes - and
it feels far too complex for my taste. If you send a patch to make the
whole thread pool pluggable - I'm ok and that would allow you to use
whatever pool you want. My requirements: the TP must be an mbean and
it must use an interface that allows passing the handback object.
Costin
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>