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