OpenImageIO/oiio — pull request #388 by lgritz (2012-06-26 21:03:13+00:00)
Title: Review: threading improvements (especially for IC/TS)

A number of threading improvements:

* Improved spinlock, atomic, and rw-lock unit tests (incorporates and subsumes https://github.com/OpenImageIO/oiio/pull/382).

* Speed up our (non-TBB) spin locks by relaxing the barrier conditions (potentially dangerous? but seems to work) and some other extremely minor tweaks.

* Switch ImageCache tile and file caches to use reader/writer locks, which is a significant speedup for many threads with high contention, as well as some minor fixes to the spin_rw_mutex itself that speed it up (including aligning and separating its members into different cache lines -- this makes the mutex 128 bytes, but definitely is a speedup) and another barrier change when acquiring the write lock (again, possibly dangerous, but it passes unit tests).

Two of these changes, as noted, are potentially dangerous, so I'd like others (especially with access to different processors and OS's) to pull these changes and test, especially a stress test like this:

build/ARCH/libOpenImageIO/spinlock_test --wedge --threads 64 --trials 3 --iters 1000000000
build/ARCH/libOpenImageIO/spin_rw_test --wedge --threads 64 --trials 3 --iters 1000000000

OK, here's the payoff, renders on a 2 CPU x 6 core each Linux box (i.e. 12 hyperthreaded cores, spread over 2 CPU sockets):

num aligned align rw
threads tbb=1 tbb=0 rwlocks rwlocks new wrlock
1 6:19 6:28 6:27 6:31 6:23
2 3:17 3:23 3:23 3:21 3:19
4 1:47 1:51 1:49 1:48 1:47
8 1:03 1:08 1:04 1:03 1:02
12 0:56 1:04 0:55 0:51 0:48
16 1:05 1:06 0:56 0:49 0:47
24 1:28 1:10 0:57 0:51 0:47
32 1:28 1:11 1:00 0:52 0:47

(hint: compare the first and last columns to see the full range of "before and after" changes due to all the changes in this pull request)

tbb=1 means using the old TBB and none of the new changes.

tbb=0 means compiling with TBB=0, i.e. our own spin_mutex in thread.h. Note that it slows down low thread counts slightly, but speeds up overthreading a lot more. (Philosophical aside: should we ever fret about moderate slowdowns for the 1-thread case, or is anybody who cares about performance always running multithreaded?)

rwlocks is what happened when I changed the ImageCache to use our spin_rw_mutex instead of spin_mutex. Not much change for low thread counts, but a significant change for higher thread counts.

aligned rwlocks is when I added the alignment and padding to the spin_rw_mutex class, makes it bigger (128 bytes instead of 8 bytes) by taking an entire cache line for each of its two elements.

"new rwlock" is aligned rwlocks, plus the change about how write_lock deals with barriers -- you can see it's an across-the-board help.

In light of these numbers, I'm fairly confident (pending testing by you nice people) in changing the default to USE_TBB=0. I'll leave the TBB code in place for now so that we can continue to occasionally experiment with USE_TBB=1 (or use it in case of emergency if a critical bug is found in our locks). Though with more time and confidence, we may elect to remove it entirely.