----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2399/#review2670 -----------------------------------------------------------
It looks nice in general. Here are a few comments and questions on the patch. /src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/2399/#comment6010> I wonder if we should use longs for pathIds. /src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/2399/#comment6012> Don't we have to initialize all bits to true? My understanding is that set means that it is free, and upon creation, the BitSet has all bits set to false. I couldn't find where you're initializing the bits. /src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/2399/#comment6009> Should perhaps wrap it with LOG.isDebugEnabled. /src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/2399/#comment6013> It seems to me that the write lock is only necessary for deleting the path id. Can't we move acquiring the lock to deletePathId()? Moving it would reduce the block size running under the lock. /src/java/test/org/apache/zookeeper/server/WatchManagerTest.java <https://reviews.apache.org/r/2399/#comment6008> Shouldn't contain an author tag. - fpj On 2011-10-17 01:49:34, Vikas Mehta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2399/ > ----------------------------------------------------------- > > (Updated 2011-10-17 01:49:34) > > > Review request for zookeeper. > > > Summary > ------- > > This change is to optimize the memory allocated for the watches managed by > the WatchManager. With this change, WatchManager consumes less than 100M for > 200M watches. > > I have also attempted to improve the synchronization in WatchManager to use > fine-grained locks. This optimization should reduce contention amongst > addWatch() operations and also between addWatch() and triggerWatch(). > > > Diffs > ----- > > /src/java/main/org/apache/zookeeper/server/WatchManager.java 1183779 > /src/java/test/org/apache/zookeeper/server/WatchManagerTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/2399/diff > > > Testing > ------- > > Added a unit test to verify the memory optimization. > > Ran zookeeper under yourkit profiler with 5000 clients setting watches on a > 10k node and a writer updating the node every second. This run did not show > any contention in the WatchManager. > > > Thanks, > > Vikas > >
