On Wed, Nov 27, 2013 at 10:15 AM, Galder Zamarreño <gal...@redhat.com>wrote:
> > On Nov 22, 2013, at 12:31 PM, Pedro Ruivo <pe...@infinispan.org> wrote: > > > > > > > On 11/21/2013 10:08 PM, Sanne Grinovero wrote: > >> Hi Pedro, > >> great analysis. > >> Conceptually I agree, and since I'm not too familiar with the core > >> code, I actually expected this to be the case as we discussed the > >> approach when we first discussed the single-lock-owner pattern. > >> > >> More specifically on the DataContainer: I don't think you should lock > >> the segment, you need to lock the key entry only as the CacheStore > >> could be quite slow so that is a "long" lock at least relatively to > >> normal usage. > > > > Agree with you. I based my solution on the current > > BoundConcurrentHashMap [1] implementation that uses locks per segment. > > Also, the eviction is kicked inside this locks so we already have a > > "long" lock to write the entry in the cache writer. > > > > [1] Segment.put(): > > > https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/util/concurrent/BoundedConcurrentHashMap.java#L1554 > > > >> > >> This should NOT be the normal key lock however, as you might remember > >> I'm since long time advocating that there needs to be a split in > >> logical "lock" as the user sees it (and as used by transactions) vs > >> the locks we use internally to keep our very own data structure > >> consistency. > >> > >> It's trivial to apply if you use > >> > >> EquivalentConcurrentHashMapV8.computeIfAbsent(K, Fun<? super K, ? > extends V>) > > > > This is a great idea :) > > +1 > > > > > The EquivalentConcurrentHashMapV8 will solve the problem for unbounded > > data container but we still have to do more work with the bounded data > > container I'm afraid… > > We need a BoundedEquivalentConcurrentHashMapV8. The reason we haven't > built one yet is because ConcurrentHashMapV8 has been in flux over past few > months, so keeping it up to date required some effort. As it gets more > stable, we can start looking into a BoundedEquivalentConcurrentHashMapV8. > This might be a good time to do it. > > +1 to write a BoundedEquivalentConcurrentHashMapV8, but changing the LRU and LIRS implementations is going to be tricky business, so I'd rather we do it separately from the DataContainer API change. > > > >> > >> Doesn't sound like a patch which requires a major release? > >> > >> Sanne > >> > >> > >> > >> > >> On 21 November 2013 18:48, Pedro Ruivo <pe...@infinispan.org> wrote: > >>> (re: https://issues.jboss.org/browse/ISPN-3048) > >>> (ps. sorry for the long mail. I hope that is clear) > >>> > >>> * Automatic eviction: > >>> > >>> I've been writing some scenario involving eviction and concurrent > >>> operations. This test shows a very reliable eviction but we have a > >>> problem when passivation is enabled. The passivation phase is ok but > >>> when we need to activate a key, we have a huge window in which the > >>> read/write operation does not find the key neither in-memory data > >>> container neither in the cache loader. This happens because the check > >>> in-memory data container is not synchronized with the check in cache > loader. > >>> > >>> Passivation phase works fine, as I said, because it happens inside the > >>> data container, i.e., the bounded concurrent hash map (BCHM) evict the > >>> key under the segment lock. > >>> > >>> * Manual eviction > >>> > >>> I haven't finish my test suite but so far it does not work as expected. > >>> If you are a backup owner, you are unable to evict any keys (because > the > >>> EvictCommand is handled as a RemoveCommand and the key is not wrapped > in > >>> EntryWrappingInterceptor). In addition, I believe that it has the same > >>> problems with activation as above. > >>> > >>> Also, I believe that passivation phase does not work well because it > >>> first tries to passivate than it removes from DataContainer. Nothing is > >>> preventing to passivate an old value, a put occurs and modifies the > data > >>> in DataContainer and finally the EvictCommand removes the new value > >>> (that is lost). > >>> > >>> * New implementation > >>> > >>> Since we are stating a new major release, I would like to "propose" the > >>> following improvements. One aspect of the implementation is the > dropping > >>> of the *Cache[Writer or Loader]Interceptor and the EvictCommand. Also, > >>> it would add a new method in DataContainer (void evict(K key)) and > >>> possible change the interface in PersistenceManager. > >>> > >>> My idea is based on the current implementation of the BCHM. The BCHM > >>> implementation is aware of the persistence and it performs internally > >>> the passivate() and activate() operations. I would like to extend this > >>> and make it full aware of the PersistenceManager. Also, it would be > >>> required to have ConcurrentHashMap (CHM) with the same features > >>> > >>> Enter in more detail, the (B)CHM.get() will be responsible to load the > >>> key from persistence (and activate it) if it is not found in-memory. > >>> Also, it stores the entry in memory. In other hand, the put() stores > the > >>> entry in-memory and in the persistence (if passivation==false) and we > >>> add an evict(k) to the CHM interface to remove from memory and > passivate > >>> it to persistence a single key. > >>> > >>> * Pros > >>> > >>> ** solves all the problems :) > >>> ** remove all the interceptors related to cache writer and cache loader > >>> => small interceptor chain > >>> ** remove evict command => lower process time(?) > >>> ** cache writer and cache loader does not need to have per key lock > >>> based (however, they should be thread safe) > >>> ** when passivation==true, we don't loose any key/value (yey) > >>> ** no need to acquire locks in lock manager (as the current manual > >>> eviction does) > >>> ** all the logic for cache loader/writer will be located in a single > mode > >>> > >>> * Cons > >>> > >>> ** difficult to maintain. We have to maintain the code for the (B)CHM > >>> ** new API and contract for DataContainer interface => it should handle > >>> all the load/write from cache loader/writer > >>> ** new API for PersistenceManager > >>> > >>> toughs? > >>> > >>> Cheers > >>> Pedro > >>> > >>> * Open question in case this is going forward > >>> > >>> ** should contains(key) put the entry loaded in-memory (if loaded from > >>> cache loader)? > >>> ** how iterator(), keys(), values() and entrySet() should behave? > should > >>> they put the entries in memory? And size()? > >>> > >>> * API changes > >>> > >>> DataContainer > >>> > >>> (new) void evict(K key) > >>> > >>> PersistenceManager > >>> > >>> (new) Entry onRead(K key) //shoud read the key. if passivation, remove > >>> from loader > >>> (new) void onWrite(K key, Entry entry) //if passivation, then do > >>> nothing, else store it > >>> (new) Entry onRemove(K key) //remove and return old > >>> (new) void onEvict(K key, Entry entry) //if passivation, then store it > >>> > >>> * Some (B)CHM pseudo-code > >>> > >>> V get(K key) > >>> Segment s = segment(key) > >>> V value = s.get(key) > >>> if (value == null) { > >>> s.lock() > >>> //recheck if s.get(key) is still null > >>> value = persistenceManager.onLoad(key); //this activates the key > if > >>> needed > >>> if (value != null) s.put(key, value) > >>> s.unlock() > >>> } > >>> return value > >>> > >>> V put(K key, V value) > >>> Segment s = segment(key) > >>> s.lock() > >>> V oldValue = s.get(key) > >>> persistenceManager.onWrite(key, value) //first try the persistence > >>> s.put(key, value) //can trigger eviction > >>> s.unlock() > >>> > >>> void evict(K key, V value) > >>> Segment s = segment(key) > >>> s.lock() > >>> persistenceManager.onEvict(key, value) //first try the persistence > >>> s.remove(key) //remove from memory if peristence is successful (we > >>> don't wanna loose the value, right?) > >>> s.unlock() > >>> _______________________________________________ > >>> infinispan-dev mailing list > >>> infinispan-dev@lists.jboss.org > >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev > >> _______________________________________________ > >> infinispan-dev mailing list > >> infinispan-dev@lists.jboss.org > >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > >> > > _______________________________________________ > > infinispan-dev mailing list > > infinispan-dev@lists.jboss.org > > https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > -- > Galder Zamarreño > gal...@redhat.com > twitter.com/galderz > > Project Lead, Escalante > http://escalante.io > > Engineer, Infinispan > http://infinispan.org > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev >
_______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev