Re: [infinispan-dev] [ISPN-116] Async cache store: aggregation of multiple changes on a single key

Hi, As part of this JIRA, I'm thinking whether we wanna leave the current queue based async store. For the work I've done, I've created a new class called CoalescedAsyncStore that works in the way agreed below, iow, instead of queying mods, it keeps a ConcurrentMap and which is swapped by a new instance when the async thread is going to apply the changes. I've run some perf tests between CoalescedAsyncStore and AsyncStore and when doing 1 million store where each key is different, the performance is fairly similar. The difference comes when trying to run a similar test where the same key is always updated, this results in lesser underlying store calls and hence CoalescedAsyncStore solution is faster here. So, rather than keeping both, I'd suggest replacing AsyncStore with the impl in CoalescedAsyncStore. We also need to look at the configuration for the new AsyncStore: Configuration wise in AsyncStoreConfig, I'd leave threadPoolSize, enabled and add the read/write lock acquisition timeout. The rest I'd get rid as no longer apply: int batchSize = 100; long pollWait = 100; int queueSize = 10000; By default, I'd give 5000ms to the read/write lock acquisition timeout. Thoughts? On 07/09/2009 01:08 PM, Manik Surtani wrote: > > On 9 Jul 2009, at 11:37, Galder Zamarreno wrote: > >> >> >> On 07/09/2009 11:55 AM, Manik Surtani wrote: >>> >>> On 8 Jul 2009, at 19:53, Jason T. Greene wrote: >>> >>>> Manik Surtani wrote: >>>>> * Make the concurrent map volatile >>>>> * When iterating, first create a new ConcurrentMap and replace the >>>>> old one with the new one so all concurrent threads write to the new >>>>> Map >>>>> * Iterate over the old map >>>> >>>> That would lead to race conditions since a concurrent writing thread >>>> could write to the "old" map, either by getting a recent yet incorrect >>>> read off the volatile, or reading it right before it changes. >>> >>> True, since referencing the map and writing to it isn't atomic. >>> >>> We could guard access to the map with a read/write lock. Safe, if a >>> little heavy-handed... map writers would acquire a RL (since we want >>> concurrent access here) but the async flushing thread would need to >>> wait >>> for a WL to swap the map reference, releasing the lock after the map >>> reference has been swapped. >> >> Yeah, I don't think it could be volatile because it only applies to >> the variable itself, not the contents pointed at. The R/W lock >> approach looks like a valid one and better than using synchronized >> blocks. Another thing I had in my mind is whether we need the copy to >> be a ConcurrentMap, we could probably just use a >> Immutables.immutableMapCopy(). > > Sync blocks would suck since it is effectively an exclusive lock. We > want non-exclusive (read) locks for threads writing to this map - we > want multiple application threads to do this concurrently otherwise this > becomes a bottleneck. And hence the need for a ConcurrentMap. > > And we don't make a copy at all - all the async flushing thread does is > that it creates a new, empty ConcurrentMap and swaps the 'async queue' > Map so that application threads can continue logging their changes > somewhere while old changes can be iterated through and flushed. And for > this, the field still needs to be volatile (not strictly so, since we > can rely on the fencing that will happen in the lock as a side-effect) > >> I looked at the two implementations (ConcurrentHashMap(Map) >> constructor vs Immutables.immutableMapCopy() which would >> Object.clone() on the map) but I'm not sure which one would be faster. >> Since clone() would rely on a native clone() impl, I'd imagine that to >> be faster. > > Like I said, we don't need a copy. We need a new, empty map for threads > to log changes to while the old one's being flushed. > >> >>> -- >>> Manik Surtani >>> manik(a)jboss.org >>> Lead, Infinispan >>> Lead, JBoss Cache >>> http://www.infinispan.org >>> http://www.jbosscache.org >>> >>> >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> -- >> Galder Zamarreño >> Sr. Software Engineer >> Infinispan, JBoss Cache > > -- > Manik Surtani > manik(a)jboss.org > Lead, Infinispan > Lead, JBoss Cache > http://www.infinispan.org > http://www.jbosscache.org > > > >