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
