Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (asf.osuosl.org: domain of slowhog@gmail.com designates
 64.233.184.228 as permitted sender)
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
        s=beta; d=gmail.com;
        h=received:message-id:date:from:user-agent:mime-version:to:cc:subject:references:in-reply-to:content-type:content-transfer-encoding:sender;
        b=BBkpKNF5DEePQzwUFYromClj7pBmtr1wvyU0jPcZ9Qk3IoTCP4PN+yIORfuTW4uBHwETFOrV3+CJtHBITN15T8AjsU6YQqR1vlqa6Ywl/1Lpgv0IwVTqWYvFQOcvnb1i5Do5E6122yORHOCdrdPPrQSA8rRz2HyLzHoyshMAXpA=
Message-ID: <44566C21.70009@ztune.net>
Date: Mon, 01 May 2006 13:14:25 -0700
From: Henry Jen <henryjen@ztune.net>
User-Agent: Mail/News 1.5 (X11/20060315)
MIME-Version: 1.0
To: "William A. Rowe, Jr." <wrowe@rowe-clan.net>
CC: APR Development List <dev@apr.apache.org>
Subject: Re: Thread pool prototype
References: <4455B86E.6000701@ztune.net> <4455EFC4.2000607@rowe-clan.net>
In-Reply-To: <4455EFC4.2000607@rowe-clan.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Sender: Henry Jen <slowhog@gmail.com>

William A. Rowe, Jr. wrote:
> Henry Jen wrote:
>>
>> Please let me know if you have any comments. :-)
>> +/**
>> + * Setup all of the internal structures required to use thread pools
>> + */
>> +APR_DECLARE(apr_status_t) apr_thread_pool_init(void);
>> +
>> +/**
>> + * Tear down all of the internal structures required to use pools
>> + */
>> +APR_DECLARE(void) apr_thread_pool_terminate(void);
> 
> Double initialization, double teardown issues?  Consider that my libfoo.so
> may want to create a threadpool, I'm not privy to whether or not the 
> application
> that embedded libfoo.so actualy intialized the threadpool API itself.
> 
> Is this needed?  If not can we please chuck it?  I understand the desire to
> anticipate the unanticipated, but this certainly will be a bigger 
> problem if
> it were used, than if we discover it's needed somehow.
>

Based on the implementation, it does not seem to be needed. I will take 
it out. So is the apr_thread_pool_task_t type definition. We can simply 
reuse the apr_thread_start_t.

As the multiple init/term, that would be carefully handled as 
apr_init/term if those function is actually in use. :-)

>> +APR_DECLARE(apr_thread_pool_t*) apr_thread_pool_create(apr_pool_t *pool,
>> +                                                       apr_size_t 
>> init_threads, +                                                       
>> apr_size_t max_threads, 
>> +                                                       apr_status_t 
>> *err);
> 
> EWWWW!
> 
> Do you mean
> 
> APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t 
> **threadpool,
>                                                  apr_pool_t *pool,
>                                                  apr_size_t init_threads,
>                                                  apr_size_t max_threads);
> 
> Please follow style guidelines, and precident, in your proposals :)
> 

Is there a guideline other than 
http://httpd.apache.org/dev/styleguide.html?

I understand the precedent, but I thought there is no requirement. :-D
apr_hash_t * 	apr_hash_make (apr_pool_t *pool)

I prefer this way because it is clear that an app can check against the 
returned pointer to be NULL without worrying the value to be undefined 
when an error occurs.

That said, I will fix it.

>> +/**
>> + * Get current number of tasks waiting in the queue
>> + * @param me The thread pool
>> + * @return Number of tasks in the queue
>> + */
>> +APR_DECLARE(int) apr_thread_pool_tasks_cnt_get(apr_thread_pool_t *me);
> 
> Worst of all worlds.  It's not a property (e.g. get/set), it's a verb 
> (please
> *count* for me your entities), so apr_thread_pool_tasks_count( ) seems 
> right.
> 
>> +/**
>> + * Get current number of idling thread
>> + * @param me The thread pool
>> + * @return Number of idling threads
>> + */
>> +APR_DECLARE(int) apr_thread_pool_unused_cnt_get(apr_thread_pool_t *me);
> 
> ditto... _count() not cnt_get()
> 

Thank you! Pardon me as not a native English speaker.

>> +/**
>> + * Stop all unused threads. Ignore the maximum number of idling threads.
>> + * @param me The thread pool
>> + * @return The total number of threads stopped.
>> + */
>> +APR_DECLARE(int) 
>> apr_thread_pool_stop_unused_threads(apr_thread_pool_t *me);
> 
> What's the difference between setting apr_thread_pool_unused_max_set() 
> to 0,
> and this function?  Can we drop it?
> 
> It seems this function would stop idling threads at this point in time, 
> while
> it makes much more design sense to set unused_max to 0, ensuring the unused
> threads continue to be pruned as they finish their unit of work.
> 
> Thoughts?
>

I tend to agree with you. Just use the set_unused_max makes sense.

In a way they work the same. But there are a few interesting differences:

1. Set the unused max does not force the thread to stop, an unused 
thread kills itself when it becomes idle and realize there are enough 
idle threads. Thus no joining or any locking issue for using this function.

2. stop_unused_threads kills *all* unused threads regardless the unused_max.

Is that mechanism make sense or just adding complexity? Not sure. It is 
for occasions you want to kill all unused threads and allow them to grow 
later. But why does an app do that? Under a resource constraint 
environment, this can serve as a mechanism for error recovery. Of 
course, you can do that with two consequent call to set_unused_max.

Think of that, I forgot to signal the idle threads when new tasks are 
added! :-P

Cheers,
Henry