HBASE-3587: Eliminate use of ReadWriteLock in RegionObserver coprocessor invocation
Review Request #1681 - Created April 6, 2011 and updated
This patch eliminates the ReentrantReadWriteLock around the coprocessors collection in CoprocessorHost. Since this lock was shared across the entire HRegion and obtained for each RegionObserver pre/post hook call, they were a significant point of contention between handler threads. Since the coprocessor list is expected to be small (a few entries in most cases) and should very rarely change, I added a new utility class org.apache.hadoop.hbase.util.SortedCopyOnWriteSet, which uses an internal TreeSet instance for storage and creates a new copy on mutations. The existing java.util.concurrent implementations -- CopyOnWriteArrayList and CopyOnWriteArraySet -- do not support sorting, which we need to order the coprocessors by priority. The SortedCopyOnWriteSet implementation could be further optimized by replacing the internal TreeSet with an internal Object, but this would add a bit more complexity to the implementation. This would improve the iterator performance, but I'm not sure it would have a meaningful impact on most region operations which are far more likely to be performance bound elsewhere.
Profiled with put-based MR workload in EC2. Don't have exact numbers on throughput (I was looking mostly at CPU utilization and contention), but request counts were considerably higher (up to several times) after applying the patch.
+1 Nice one Gary.
I had a chat with JD about this (on a completely different jira). The point he raised was a TreeSet is a badly designed interface in the sense that it can have a comparator/comparable check at compile time i.e., in the constructor that takes E, one can have a check whether E is instanceof Comparable or not otherwise, throw an exception. It sounds right to me. Though you have documented this dependency but still want to know your opinion.