[asterisk-dev] [Code Review] 2882: Threadpool simplifications

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2882/
-----------------------------------------------------------

(Updated Nov. 14, 2013, 5:18 p.m.)


Status
------

This change has been discarded.


Review request for Asterisk Developers, David Lee and Joshua Colp.


Repository: Asterisk


Description
-------

When initial performance testing was done, threadpool usage in stasis appeared to be a big offender, so I set out to simplify threadpools as much as possible.

Here is a listing of the simplifications:

* threadpool listeners have been removed.
* ast_threadpool_set_size() has been removed (and thus there are no longer "zombie" threads)
* There are no longer separate ao2_containers for threads. They are instead kept in a single list, and counters are used to determine how many of any given type there are.
* Pools make use of a semaphore for signaling idle threads to awaken, rather than a boolean, lock, and ast_cond_t per-worker.
* worker thread structures have been simplified. In addition to what has been mentioned, they also no longer have an id, or any state information stored on them.

Because of the removal of threadpool listeners, the unit tests had to be pared down some. They no longer could test the actual threadpool mechanics so much as the API provided by the threadpool. Thus, many tests have been removed, and other tests have been altered to not require a threadpool listener. There are no tests for certain threadpool options such as idle timeout right now since it is not really possible to test that a thread has actually been removed due to idleness.

In addition to the simplifications that have been made, there has also been a modification to threadpool's initial_size option. In addition to being the initial size of the threadpool on creation, it now also serves as a minimum size. Workers that are allocated when the threapdool is created will have no idle timeout, despite how the threadpool may be configured.

These changes seem to have mixed effects when it comes to performance. With a reasonable number of threads (say, number of CPU cores * 2) in the threadpool, these changes introduce a positive change to performance. With a ridiculous number of threads in the threadpool (say 200), this either has no effect or has a slight degradation to performance. Judicious maximum threadpool sizes will result in a net positive effect from these changes. Plus, this removes a *lot* of code that really was just not necessary.


Diffs
-----

  /team/group/performance/include/asterisk/threadpool.h 399869 
  /team/group/performance/main/sorcery.c 399869 
  /team/group/performance/main/threadpool.c 399869 
  /team/group/performance/res/res_pjsip.c 399869 
  /team/group/performance/tests/test_threadpool.c 399869 

Diff: https://reviewboard.asterisk.org/r/2882/diff/


Testing
-------

The unit tests all pass.

I ran stock Asterisk 12 with these changes and ensured that the threadpool used by stasis there did not encounter any difficulties.
I also ran a call test with chan_pjsip in the performance branch to ensure that its usage of the threadpool did not cause issues.


Thanks,

Mark Michelson

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131114/f6fcf41a/attachment-0001.html>