I agree with your patch, and you're right that since the creator thread
is the _only thing anywhere_ that calls create and destroy my changes
were unneeded.
You might want to make the wait in the tail of the loop 1000 (didn't see
that in your patch, sorry if I missed it, I'm not very good at reading
context diff, only udiff)
Go ahead and commit your version (with possibly 1000 ms wait).
--Brandon
On Sat, 09/27/03 at 14:47:21 -0400, Edward J. Huff wrote:
> On Sat, 2003-09-27 at 12:01, Brandon Low wrote:
> > Here is a proposed patch, tell me what you think.
> >
> > (note this also goes back to wait(1000) once every loop of the creation
> > thread, I did this to ensure that one call to awaken() will _always_ put
> > the thread in creation mode.
> >
> > --Brandon
>
> Here is my proposed clarification. I deleted the createThreads
> and deleteThreads functions (which are called only from one place)
> and put the code into the creator thread's loop. I also moved the
> awaken() code into getThread(). Look at this to see if you still
> think changes are needed.
>
> [from the earlier message]
> > What happens if ... the QThreadFactory's creator thread is
> > able to keep calling createThread repeatedly but NONE of
> > those threads that are created ever manage to get the
> > countLock? Then all of a sudden we have tons of extra
> > threads around.
>
> With createThread moved into the text of the loop, it
> becomes clear that there is no way for available to
> be off by more than one. It doesn't matter if
> available says the stack is empty when it has one
> free thread, but the reverse is a mess -- an NPE
> or messy code to avoid it.
>
> I agree that the code you removed is unnecessary
> and removal makes the remaining code clearer:
> this patch removes it too.
>
> -- Ed Huff
>
> Index: QThreadFactory.java
> ===================================================================
> RCS file: /cvsroot/freenet/freenet/src/freenet/thread/QThreadFactory.java,v
> retrieving revision 1.35
> diff -c -r1.35 QThreadFactory.java
> *** QThreadFactory.java 27 Sep 2003 07:37:16 -0000 1.35
> --- QThreadFactory.java 27 Sep 2003 17:03:16 -0000
> ***************
> *** 73,85 ****
> }
> while ( available < required ) {
> doLog = true;
> ! createThread();
> ! synchronized(countLock) {
> // required must not increase.
> required =
> Math.min(required,
>
> Math.max((int) (active * IDEAL_AVAILABLE_RATIO),
>
> 2 * MINIMUM_AVAILABLE_ABS));
> if ( available > required &&
> available > active *
> MINIMUM_AVAILABLE_RATIO ) {
> try {
> --- 73,99 ----
> }
> while ( available < required ) {
> doLog = true;
> ! // createThread();
> ! QThread newThread;
> ! long newThreadNumber;
> ! synchronized(numLock) {
> ! newThreadNumber = ++threadNumber;
> ! }
> ! newThread = new QThread(newThreadNumber);
> ! synchronized(headLock) {
> ! newThread.next = headerThread;
> ! headerThread = newThread;
> ! }
> ! synchronized(countLock) {
> ! available++; // never an overestimate.
> // required must not increase.
> required =
> Math.min(required,
>
> Math.max((int) (active * IDEAL_AVAILABLE_RATIO),
>
> 2 * MINIMUM_AVAILABLE_ABS));
> + // Wait after each thread creation to
> see if we still need it.
> + // This limits the rate of thread
> creation. Some active
> + // threads might finish.
> if ( available > required &&
> available > active *
> MINIMUM_AVAILABLE_RATIO ) {
> try {
> ***************
> *** 120,132 ****
> }
> while ( available > allowed ) {
> doLog = true;
> ! destroyThread();
> ! synchronized(countLock) {
> // allowed must not decrease.
> allowed =
> Math.max((int) (active *
> IDEAL_AVAILABLE_RATIO),
>
> Math.max(allowed,
>
> 2 * MINIMUM_AVAILABLE_ABS));
> if ( available < allowed &&
> available < active *
> MAXIMUM_AVAILABLE_RATIO ) {
> try {
> --- 134,160 ----
> }
> while ( available > allowed ) {
> doLog = true;
> ! synchronized(countLock) {
> ! // Remove a thread from the stack and
> signal it to die.
> ! // But if all of the threads have
> meanwhile started jobs,
> ! // then do nothing.
> ! if (available >= 1) {
> ! available--; // never an
> overestimate.
> ! QThread dyingThread;
> ! synchronized(headLock) {
> ! dyingThread =
> headerThread;
> ! headerThread =
> headerThread.next;
> ! }
> ! dyingThread.die();
> ! }
> // allowed must not decrease.
> allowed =
> Math.max((int) (active *
> IDEAL_AVAILABLE_RATIO),
>
> Math.max(allowed,
>
> 2 * MINIMUM_AVAILABLE_ABS));
> + // Wait after each thread destruction
> to see if we
> + // still want to destroy more. This
> limits the rate
> + // of thread destruction. More work
> might come in.
> if ( available < allowed &&
> available < active *
> MAXIMUM_AVAILABLE_RATIO ) {
> try {
> ***************
> *** 236,280 ****
> thread.job = job;
> thread.start();
>
> - awaken();
> - return thread;
> - }
> -
> - /**
> - * Creates a thread and adds it to the stack.
> - */
> - private final void createThread() {
> - QThread newThread;
> - long newThreadNumber;
> - synchronized(numLock) {
> - newThreadNumber = ++threadNumber;
> - }
> - newThread = new QThread(newThreadNumber);
> - synchronized(headLock) {
> - newThread.next = headerThread;
> - headerThread = newThread;
> - }
> synchronized(countLock) {
> ! available++; // never an overestimate.
> ! }
> ! }
> !
> ! /**
> ! * Removes a thread from the stack and signals it to die.
> ! * But if all of the threads have meanwhile started jobs,
> ! * then does nothing.
> ! */
> ! private final void destroyThread() {
> ! QThread dyingThread;
> ! synchronized(countLock) {
> ! if (available < 1) return;
> ! available--; // never an overestimate.
> ! }
> ! synchronized(headLock) {
> ! dyingThread = headerThread;
> ! headerThread = headerThread.next;
> }
> ! dyingThread.die();
> }
>
> /**
> --- 264,278 ----
> thread.job = job;
> thread.start();
>
> synchronized(countLock) {
> ! if ( ( available < MINIMUM_AVAILABLE_ABS) ||
> ! ( available < active * MINIMUM_AVAILABLE_RATIO) ||
> ! ( ( available > (3 * MINIMUM_AVAILABLE_ABS)) &&
> ! ( available > active * MAXIMUM_AVAILABLE_RATIO))) {
> ! countLock.notifyAll();
> ! }
> }
> ! return thread;
> }
>
> /**
> ***************
> *** 288,304 ****
> synchronized(countLock) {
> active--;
> available++; // never an overestimate
> - }
> - }
> -
> - private final void awaken() {
> - synchronized(countLock) {
> - if ( ( available < MINIMUM_AVAILABLE_ABS) ||
> - ( available < active * MINIMUM_AVAILABLE_RATIO) ||
> - ( ( available > (3 * MINIMUM_AVAILABLE_ABS)) &&
> - ( available > active * MAXIMUM_AVAILABLE_RATIO))) {
> - countLock.notifyAll();
> - }
> }
> }
>
> --- 286,291 ----
> _______________________________________________
> Devl mailing list
> [EMAIL PROTECTED]
> http://dodo.freenetproject.org/cgi-bin/mailman/listinfo/devl
_______________________________________________
Devl mailing list
[EMAIL PROTECTED]
http://dodo.freenetproject.org/cgi-bin/mailman/listinfo/devl
