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 :) 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... > > Doesn't sound like a patch which requires a major release? > > Sanne > > > > > On 21 November 2013 18:48, Pedro Ruivo <[email protected]> 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 >> [email protected] >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ > infinispan-dev mailing list > [email protected] > https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ infinispan-dev mailing list [email protected] https://lists.jboss.org/mailman/listinfo/infinispan-dev
