> On 2011-10-19 23:45:06, Vikas Mehta wrote: > > /src/java/main/org/apache/zookeeper/server/WatchManager.java, line 124 > > <https://reviews.apache.org/r/2399/diff/1/?file=50634#file50634line124> > > > > 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.
If I'm reading the code correctly, true in the bitset means that the id is free, but bitset starts with all positions having a false value, which means that we are starting with all positions used. Don't we need to initialize in this case? It might be easier to invert the logic and keep track of used ids instead of free. > On 2011-10-19 23:45:06, Vikas Mehta wrote: > > /src/java/main/org/apache/zookeeper/server/WatchManager.java, line 54 > > <https://reviews.apache.org/r/2399/diff/1/?file=50634#file50634line54> > > > > 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. My comment was triggered by a comment in the code about wrapping around, so I was wondering about making it a long to make it less likely that we wrap around. I agree with you observation about the memory requirements, though, and I don't feel strongly about making it a long. - fpj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2399/#review2672 ----------------------------------------------------------- On 2011-10-19 23:45:13, Vikas Mehta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2399/ > ----------------------------------------------------------- > > (Updated 2011-10-19 23:45:13) > > > 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 > >
