----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2399/#review2672 -----------------------------------------------------------
Thanks for the code review. /src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/2399/#comment6019> If we hit 2 Billion paths to watch then each entry in the watchTable will be about 250M and path2Id table will be ~20 GB (assuming 10 bytes per entry). Overall, this much data will be unmanageable from the memory and network utilization perspective. I am hesitant in changing the seq to long because it means pathIds are long and it nearly doubles the memory required to store this information. Also the BitSet implementation doesn't support long, i.e. it will not allocate over 2Billion bits. If you think that there is a usecase for over 2 Billion paths, let me know and I will try to change this to use long and implement a LongBitSet. /src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/2399/#comment6018> freePathIds is mainly to remember holes in the allocated pathIds using the pathIdSeq. For this reason, the freePathIds will start out empty and path deletion will mark the corresponding pathId in the freePathId set as available. /src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/2399/#comment6022> Done. /src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/2399/#comment6063> I moved the deletePathId() up and made it upgrade the lock. This will keep the loop to unset the watches on the node being deleted out of the write lock. /src/java/test/org/apache/zookeeper/server/WatchManagerTest.java <https://reviews.apache.org/r/2399/#comment6062> deleted. - Vikas 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 > >
