(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(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

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/o... > > 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 :) 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...

On 11/22/2013 11:40 AM, Sanne Grinovero wrote: > > Do we still need to support the non-V8 implementations? > Clearly there are cases in which we need it to be available: > org.infinispan.util.concurrent.locks.containers.AbstractPerEntryLockContainer<L> > refers explicitly to EquivalentConcurrentHashMapV8 I don't think we need to support the non-v8 implementation but I'm not aware of any BoundedConcurrentHashMapV8. So, if I have to choose, for me it is simpler to change the current BounderConcurrentHashMap than implement all the eviction policies on top of ECHM-V8.

On Nov 22, 2013, at 12:31 PM, Pedro Ruivo <pedro(a)infinispan.org&gt; 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/o... > >> >> 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.

> >> >> Doesn't sound like a patch which requires a major release? >> >> Sanne >> >> >> >> >> On 21 November 2013 18:48, Pedro Ruivo <pedro(a)infinispan.org&gt; 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(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño galder(a)redhat.com twitter.com/galderz Project Lead, Escalante http://escalante.io Engineer, Infinispan http://infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

(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(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev