On 7 Jun 2011, at 13:13, Sanne Grinovero wrote: Hello all, in this scenario we have the Infinispan Lucene Directory using batching (DummyTransaction), eviction and passivation to keep the amount of memory being used for the index under control; I'm using LIRS but experienced the same issue with all other strategies. As you can see from the following stacktrace, the batching ends by sending a commit request, so the status of the transaction is 8 (STATUS_COMMITTING) in this context. The new data is stored in the DataContainer, then the BoundedConcurrentHashMap notifies the EvictionManagerImpl as it has to evict some values, and this one attempts to acquire a lock on the to-be-evicted keys (which are obviously not the same I'm trying to store). Acquiring this lock is an invalid operation as the transaction is in commit state, and so this operation fails with an exception. Thread [Hibernate Search: Directory writer-1] (Suspended (breakpoint at line 92 in LockManagerImpl)) LockManagerImpl.lockAndRecord(Object, InvocationContext) line: 92 EvictionManagerImpl.acquireLock(InvocationContext, Object) line: 210 EvictionManagerImpl.onEntryEviction(Object, InternalCacheEntry) line: 170 EvictionManagerImpl.onEntryEviction(Map<Object,InternalCacheEntry>) line: 162 DefaultDataContainer$DefaultEvictionListener.onEntryEviction(Map<Object,InternalCacheEntry>) line: 201 BoundedConcurrentHashMap$Segment<K,V>.notifyEvictionListener(Set<HashEntry<K,V>>) line: 1176 BoundedConcurrentHashMap$Segment<K,V>.put(K, int, V, boolean) line: 1011 BoundedConcurrentHashMap<K,V>.put(K, V) line: 1556 DefaultDataContainer.put(Object, Object, long, long) line: 148 ReadCommittedEntry.commit(DataContainer) line: 177 LockingInterceptor.commitEntry(CacheEntry, boolean) line: 389 LockingInterceptor.cleanupLocks(InvocationContext, boolean) line: 367 LockingInterceptor.visitCommitCommand(TxInvocationContext, CommitCommand) line: 98 CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 CacheStoreInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, VisitableCommand) line: 119 CacheStoreInterceptor.visitCommitCommand(TxInvocationContext, CommitCommand) line: 148 CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 CacheLoaderInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, VisitableCommand) line: 119 CacheLoaderInterceptor(CommandInterceptor).handleDefault(InvocationContext, VisitableCommand) line: 133 CacheLoaderInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, CommitCommand) line: 116 CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 NotificationInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, VisitableCommand) line: 119 NotificationInterceptor.visitCommitCommand(TxInvocationContext, CommitCommand) line: 56 CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 TxInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, VisitableCommand) line: 119 TxInterceptor.visitCommitCommand(TxInvocationContext, CommitCommand) line: 142 CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 CacheMgmtInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, VisitableCommand) line: 119 CacheMgmtInterceptor(CommandInterceptor).handleDefault(InvocationContext, VisitableCommand) line: 133 CacheMgmtInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, CommitCommand) line: 116 CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 InvocationContextInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, VisitableCommand) line: 119 InvocationContextInterceptor.handleAll(InvocationContext, VisitableCommand) line: 96 InvocationContextInterceptor.handleDefault(InvocationContext, VisitableCommand) line: 63 InvocationContextInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, CommitCommand) line: 116 CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 BatchingInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, VisitableCommand) line: 119 BatchingInterceptor.handleDefault(InvocationContext, VisitableCommand) line: 77 BatchingInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, CommitCommand) line: 116 CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 InterceptorChain.invoke(InvocationContext, VisitableCommand) line: 274 TransactionCoordinator.commit(LocalTransaction, boolean) line: 136 TransactionXaAdapter.commit(Xid, boolean) line: 124 DummyTransaction.runCommitTx() line: 312 DummyTransaction.commit() line: 99 BatchModeTransactionManager(DummyBaseTransactionManager).commit() line: 97 BatchContainer.resolveTransaction(BatchContainer$BatchDetails, boolean) line: 131 BatchContainer.endBatch(boolean, boolean) line: 108 BatchContainer.endBatch(boolean) line: 93 CacheDelegate<K,V>.endBatch(boolean) line: 436 InfinispanIndexOutput.close() line: 208 IOUtils.closeSafely(Closeable...) line: 80 FieldsWriter.close() line: 111 StoredFieldsWriter.flush(SegmentWriteState) line: 52 DocFieldProcessor.flush(Collection<DocConsumerPerThread>, SegmentWriteState) line: 58 I would like to remove the lock operation from the eviction listener, but I'm not understanding why this locking is needed there and would appreciate some explanations or help with this. Looking at the code, my first thought was that it is needed for sync-ing cache store /passivator access on that key. But afaik the cache store takes care of key locking at its own level[1], so I think that should be remove entirely. [1] https://github.com/mmarkus/infinispan/blob/master/core/src/main/java/o... Shouldn't an evict operation be a "best effort" operation in all cases, +1. Even if my statement above doesn't stand and there's a reason to keep the lock for consistency, it should be a best effort: try to lock with 0 timeout, if fails just ignore it and move on.

From what you say, it seems that we can ignore it alltogether?

or is the idea here that we want the evictable data to be consistently evicted on multiple nodes, or maybe even rollback an evict operation? We don't advertise cluster-wise eviction consistency, so I doubt that is the case.

On 7 Jun 2011, at 13:13, Sanne Grinovero wrote: > Hello all, > in this scenario we have the Infinispan Lucene Directory using > batching (DummyTransaction), eviction and passivation to keep the > amount of memory being used for the index under control; I'm using > LIRS but experienced the same issue with all other strategies. > > As you can see from the following stacktrace, the batching ends by > sending a commit request, so the status of the transaction is 8 > (STATUS_COMMITTING) in this context. > The new data is stored in the DataContainer, then the > BoundedConcurrentHashMap notifies the EvictionManagerImpl as it has to > evict some values, and this one attempts to acquire a lock on the > to-be-evicted keys (which are obviously not the same I'm trying to > store). > Acquiring this lock is an invalid operation as the transaction is in > commit state, and so this operation fails with an exception. > > Thread [Hibernate Search: Directory writer-1] (Suspended (breakpoint > at line 92 in LockManagerImpl)) > LockManagerImpl.lockAndRecord(Object, InvocationContext) line: 92 > EvictionManagerImpl.acquireLock(InvocationContext, Object) line: 210 > EvictionManagerImpl.onEntryEviction(Object, InternalCacheEntry) line: 170 > EvictionManagerImpl.onEntryEviction(Map<Object,InternalCacheEntry>) line: 162 > DefaultDataContainer$DefaultEvictionListener.onEntryEviction(Map<Object,InternalCacheEntry>) > line: 201 > BoundedConcurrentHashMap$Segment<K,V>.notifyEvictionListener(Set<HashEntry<K,V>>) > line: 1176 > BoundedConcurrentHashMap$Segment<K,V>.put(K, int, V, boolean) line: 1011 > BoundedConcurrentHashMap<K,V>.put(K, V) line: 1556 > DefaultDataContainer.put(Object, Object, long, long) line: 148 > ReadCommittedEntry.commit(DataContainer) line: 177 > LockingInterceptor.commitEntry(CacheEntry, boolean) line: 389 > LockingInterceptor.cleanupLocks(InvocationContext, boolean) line: 367 > LockingInterceptor.visitCommitCommand(TxInvocationContext, > CommitCommand) line: 98 > CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 > CacheStoreInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, > VisitableCommand) line: 119 > CacheStoreInterceptor.visitCommitCommand(TxInvocationContext, > CommitCommand) line: 148 > CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 > CacheLoaderInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, > VisitableCommand) line: 119 > CacheLoaderInterceptor(CommandInterceptor).handleDefault(InvocationContext, > VisitableCommand) line: 133 > CacheLoaderInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, > CommitCommand) line: 116 > CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 > NotificationInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, > VisitableCommand) line: 119 > NotificationInterceptor.visitCommitCommand(TxInvocationContext, > CommitCommand) line: 56 > CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 > TxInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, > VisitableCommand) line: 119 > TxInterceptor.visitCommitCommand(TxInvocationContext, CommitCommand) line: 142 > CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 > CacheMgmtInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, > VisitableCommand) line: 119 > CacheMgmtInterceptor(CommandInterceptor).handleDefault(InvocationContext, > VisitableCommand) line: 133 > CacheMgmtInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, > CommitCommand) line: 116 > CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 > InvocationContextInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, > VisitableCommand) line: 119 > InvocationContextInterceptor.handleAll(InvocationContext, > VisitableCommand) line: 96 > InvocationContextInterceptor.handleDefault(InvocationContext, > VisitableCommand) line: 63 > InvocationContextInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, > CommitCommand) line: 116 > CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 > BatchingInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, > VisitableCommand) line: 119 > BatchingInterceptor.handleDefault(InvocationContext, > VisitableCommand) line: 77 > BatchingInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, > CommitCommand) line: 116 > CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 > InterceptorChain.invoke(InvocationContext, VisitableCommand) line: 274 > TransactionCoordinator.commit(LocalTransaction, boolean) line: 136 > TransactionXaAdapter.commit(Xid, boolean) line: 124 > DummyTransaction.runCommitTx() line: 312 > DummyTransaction.commit() line: 99 > BatchModeTransactionManager(DummyBaseTransactionManager).commit() line: 97 > BatchContainer.resolveTransaction(BatchContainer$BatchDetails, > boolean) line: 131 > BatchContainer.endBatch(boolean, boolean) line: 108 > BatchContainer.endBatch(boolean) line: 93 > CacheDelegate<K,V>.endBatch(boolean) line: 436 > InfinispanIndexOutput.close() line: 208 > IOUtils.closeSafely(Closeable...) line: 80 > FieldsWriter.close() line: 111 > StoredFieldsWriter.flush(SegmentWriteState) line: 52 > DocFieldProcessor.flush(Collection<DocConsumerPerThread>, > SegmentWriteState) line: 58 > > I would like to remove the lock operation from the eviction listener, > but I'm not understanding why this locking is needed there and would > appreciate some explanations or help with this. Looking at the code, my first thought was that it is needed for sync-ing cache store /passivator access on that key. But afaik the cache store takes care of key locking at its own level[1], so I think that should be remove entirely. [1] https://github.com/mmarkus/infinispan/blob/master/core/src/main/java/org/... > Shouldn't an evict operation be a "best effort" operation in all > cases, +1. Even if my statement above doesn't stand and there's a reason to keep the lock for consistency, it should be a best effort: try to lock with 0 timeout, if fails just ignore it and move on.

> or is the idea here that we want the evictable data to be > consistently evicted on multiple nodes, or maybe even rollback an > evict operation? We don't advertise cluster-wise eviction consistency, so I doubt that is the case.

> > Cheers, > Sanne > _______________________________________________ > 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

On 8 Jun 2011, at 11:44, Galder Zamarreño wrote: > > On Jun 7, 2011, at 2:41 PM, Mircea Markus wrote: > >> >> On 7 Jun 2011, at 13:13, Sanne Grinovero wrote: >> >>> Hello all, >>> in this scenario we have the Infinispan Lucene Directory using >>> batching (DummyTransaction), eviction and passivation to keep the >>> amount of memory being used for the index under control; I'm using >>> LIRS but experienced the same issue with all other strategies. >>> >>> As you can see from the following stacktrace, the batching ends by >>> sending a commit request, so the status of the transaction is 8 >>> (STATUS_COMMITTING) in this context. >>> The new data is stored in the DataContainer, then the >>> BoundedConcurrentHashMap notifies the EvictionManagerImpl as it has to >>> evict some values, and this one attempts to acquire a lock on the >>> to-be-evicted keys (which are obviously not the same I'm trying to >>> store). >>> Acquiring this lock is an invalid operation as the transaction is in >>> commit state, and so this operation fails with an exception. >>> >>> Thread [Hibernate Search: Directory writer-1] (Suspended (breakpoint >>> at line 92 in LockManagerImpl)) >>>     LockManagerImpl.lockAndRecord(Object, InvocationContext) line: 92 >>>     EvictionManagerImpl.acquireLock(InvocationContext, Object) line: 210 >>>     EvictionManagerImpl.onEntryEviction(Object, InternalCacheEntry) line: 170 >>>     EvictionManagerImpl.onEntryEviction(Map<Object,InternalCacheEntry>) line: 162 >>>     DefaultDataContainer$DefaultEvictionListener.onEntryEviction(Map<Object,InternalCacheEntry>) >>> line: 201 >>>     BoundedConcurrentHashMap$Segment<K,V>.notifyEvictionListener(Set<HashEntry<K,V>>) >>> line: 1176 >>>     BoundedConcurrentHashMap$Segment<K,V>.put(K, int, V, boolean) line: 1011 >>>     BoundedConcurrentHashMap<K,V>.put(K, V) line: 1556 >>>     DefaultDataContainer.put(Object, Object, long, long) line: 148 >>>     ReadCommittedEntry.commit(DataContainer) line: 177 >>>     LockingInterceptor.commitEntry(CacheEntry, boolean) line: 389 >>>     LockingInterceptor.cleanupLocks(InvocationContext, boolean) line: 367 >>>     LockingInterceptor.visitCommitCommand(TxInvocationContext, >>> CommitCommand) line: 98 >>>     CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 >>>     CacheStoreInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, >>> VisitableCommand) line: 119 >>>     CacheStoreInterceptor.visitCommitCommand(TxInvocationContext, >>> CommitCommand) line: 148 >>>     CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 >>>     CacheLoaderInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, >>> VisitableCommand) line: 119 >>>     CacheLoaderInterceptor(CommandInterceptor).handleDefault(InvocationContext, >>> VisitableCommand) line: 133 >>>     CacheLoaderInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, >>> CommitCommand) line: 116 >>>     CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 >>>     NotificationInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, >>> VisitableCommand) line: 119 >>>     NotificationInterceptor.visitCommitCommand(TxInvocationContext, >>> CommitCommand) line: 56 >>>     CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 >>>     TxInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, >>> VisitableCommand) line: 119 >>>     TxInterceptor.visitCommitCommand(TxInvocationContext, CommitCommand) line: 142 >>>     CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 >>>     CacheMgmtInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, >>> VisitableCommand) line: 119 >>>     CacheMgmtInterceptor(CommandInterceptor).handleDefault(InvocationContext, >>> VisitableCommand) line: 133 >>>     CacheMgmtInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, >>> CommitCommand) line: 116 >>>     CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 >>>     InvocationContextInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, >>> VisitableCommand) line: 119 >>>     InvocationContextInterceptor.handleAll(InvocationContext, >>> VisitableCommand) line: 96 >>>     InvocationContextInterceptor.handleDefault(InvocationContext, >>> VisitableCommand) line: 63 >>>     InvocationContextInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, >>> CommitCommand) line: 116 >>>     CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 >>>     BatchingInterceptor(CommandInterceptor).invokeNextInterceptor(InvocationContext, >>> VisitableCommand) line: 119 >>>     BatchingInterceptor.handleDefault(InvocationContext, >>> VisitableCommand) line: 77 >>>     BatchingInterceptor(AbstractVisitor).visitCommitCommand(TxInvocationContext, >>> CommitCommand) line: 116 >>>     CommitCommand.acceptVisitor(InvocationContext, Visitor) line: 60 >>>     InterceptorChain.invoke(InvocationContext, VisitableCommand) line: 274 >>>     TransactionCoordinator.commit(LocalTransaction, boolean) line: 136 >>>     TransactionXaAdapter.commit(Xid, boolean) line: 124 >>>     DummyTransaction.runCommitTx() line: 312 >>>     DummyTransaction.commit() line: 99 >>>     BatchModeTransactionManager(DummyBaseTransactionManager).commit() line: 97 >>>     BatchContainer.resolveTransaction(BatchContainer$BatchDetails, >>> boolean) line: 131 >>>     BatchContainer.endBatch(boolean, boolean) line: 108 >>>     BatchContainer.endBatch(boolean) line: 93 >>>     CacheDelegate<K,V>.endBatch(boolean) line: 436 >>>     InfinispanIndexOutput.close() line: 208 >>>     IOUtils.closeSafely(Closeable...) line: 80 >>>     FieldsWriter.close() line: 111 >>>     StoredFieldsWriter.flush(SegmentWriteState) line: 52 >>>     DocFieldProcessor.flush(Collection<DocConsumerPerThread>, >>> SegmentWriteState) line: 58 >>> >>> I would like to remove the lock operation from the eviction listener, >>> but I'm not understanding why this locking is needed there and would >>> appreciate some explanations or help with this. >> Looking at the code, my first thought was that it is needed for sync-ing cache store /passivator access on that key. >> But afaik the cache store takes care of key locking at its own level[1], so I think that should be remove entirely. >> >> [1] https://github.com/mmarkus/infinispan/blob/master/core/src/main/java/org/... >>> Shouldn't an evict operation be a "best effort" operation in all >>> cases, >> +1. Even if my statement above doesn't stand and there's a reason to keep the lock for consistency, it should be a best effort: try to lock with 0 timeout, if fails just ignore it and move on. > > Hmmm, if eviction was on its own, I can see the point of best effort. > > But, the point of passivation is that you evict from memory and you store in the cache store. What harm does it cause that eviction could not work for a passivated cache? You could potentially end up with entries both in cache and store for at least a brief period of time, until the eviction thread tries to evict it again. As per the stack trace, the lock acquisition fails within the scope of  BoundedConcurrentHashMap$Segment.notifyEvictionListener: that means that entries were removed from memory for sure. So the only possible problematic outcome is to have entires removed from memory but not passivated. Still a problem though. > Now, what happens in a passivated cache if the entry, which is supposed to have been passivated, is still in memory due to eviction not acquiring the lock, that's not going to happen, the failing lock acquisition fails after element is evicted. I don't think this lock acquisition is needed though, and the fact that tests didn't fail after Sanne removed the locking code from EvictionManagerImpl.onEntryEviction back my assumption. > and suddenly it gets modified in memory. Will this changes propagate to the passivated entry? > > I think this needs some thought... > > Btw, 2LC uses eviction but no passivation. > >>> or is the idea here that we want the evictable data to be >>> consistently evicted on multiple nodes, or maybe even rollback an >>> evict operation? >> We don't advertise cluster-wise eviction consistency, so I doubt that is the case. > > Indeed, eviction is a local only operation. > >>> >>> Cheers, >>> Sanne >>> _______________________________________________ >>> 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 > Sr. Software Engineer > Infinispan, JBoss Cache > > > _______________________________________________ > 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

On 8 Jun 2011, at 16:24, Sanne Grinovero wrote: > I was mentioning passivation was enabled as debugging I saw it was > going to invoke to the passivator, > but I just realized that passivation was actually disabled in my configuration. > So one thing I'm solving now, is that we don't need to lock if the > passivator is disabled, as we're not going to do any operation with > the lock held, besides potentially invoking user listeners. > So this affects the 2nd level cache as well, as the bug kicks in > regardless of the passivation being enabled). > > Some findings about this lock, after talking with Vladimir yesterday on irc: > > we realized that the lock is actually acquired *after* it was evicted > from the container; > so if the mission of this lock was to protect readers from reading the > non-existing value, then we should acquire it before it's actually > evicted; currently there's a smallish race condition possible, in > which the value has been evicted but not stored yet; during the proper > store operation which is likely the slowest part the lock is owned so > this might prevent a get with locks to not find the value anywhere. > > So for this case maybe having the lock is a good idea, but could > anybody confirm that having the lock would prevent an inconsistent > read? Is this lock good enough here?  There is still a race between evicting from the data container and acquiring the lock that a thread may attempt a read and find nothing. But even then this doesn't really help since this is a write lock we are talking about.  Concurrent readers will not acquire a write (or even read) lock. The correct approach really should be: 1) Passivate entry to a CacheStore 2) Evict entry from data container with no need for a lock.  This would mean the BoundedCHM would have a handle on the Passivator.

Cheers Manik -- Manik Surtani manik(a)jboss.org twitter.com/maniksurtani Lead, Infinispan http://www.infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 11-06-08 12:10 PM, Sanne Grinovero wrote: > Yes, we're on the same channel now. That's exactly what I meant > yesterday on IRC when mentioning the BoundedCHM, and what I meant > above with "we should acquire it before it's actually evicted". > But this issue is getting complex, I've split it in 4 different > problems and I'm going to solve the first three soon, the BoundedCHM > is the fourth and I'm not sure I'll fix that today. Sanne, Sounds good, please consult us when it comes to changing BCHM! Cheers. _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

It looks as a good improvement, especially as we remove the locking, but I still have some doubts: 1) as the passivation happens in the eviction thread, is that going to be fast enough or hindering the eviction performance? especially with LIRS, being this the user thread it means that put operations suffer a mayor performance hit (for async caheloaders, see next point). Well this was the case already, I guess.

2) with async stores, the issue is back. The value is not going to be found while it's "in flight" in the async storage queue; so unless you want to avoid supporting it for now, the actual removal from the container should be performed from the async cache loader code.

So it looks like that - when passivation is enabled - the proper way to implement this is to use the eviction policy only to identify what's going to be removed from the container, but not to actually remove it. I'd still focus on having the best performance possible for the case in which passivation is not enabled, that looks like the "performance needing" setup, or maybe we need separate eviction strategies fro when passivation is enabled.

3) so every time we evict an entry, we write it to the store.. I guess that leads to re-write quite often values which are already in the store, and for which there's no need to store again. I think this argument came up before, but we didn't want to track an additional boolean to keep the memory need low. Still when we're using it as a cache with eviction rules in place, it seems to me that overall performance would be better by avoiding more writes at the cost of being able to store some less entries in memory.

> With passivation enabled, the entry is only written to the store when removed from memory. And when activating (store -> memory) it is removed from the store. So you wouldn't really write twice to the store. Thanks, with that you answered an unasked question I had. So, should we check for race conditions while reading too, that the entry disappears from a shared cacheloader before all active owners have it in the container ? :)

Anyway these thoughts boil down to one question: what's the use case? Why should anybody use this? It looks to me that passivation=false & async cache loader is always superior, and keeps us with a nice and clean evict-from-memory-only problem, which can be performed efficiently.

> 2) with async stores, the issue is back. The value is not going to be > found while it's "in flight" in the async storage queue; so unless you > want to avoid supporting it for now, the actual removal from the > container should be performed from the async cache loader code. Ugh - yes, I agree. But I hate the way the mixing of concerns is starting to look. :-(

On 10 Jun 2011, at 14:18, Sanne Grinovero wrote: > It looks as a good improvement, especially as we remove the locking, > but I still have some doubts: > > 1) as the passivation happens in the eviction thread, is that going to > be fast enough or hindering the eviction performance? especially with > LIRS, being this the user thread it means that put operations suffer a > mayor performance hit (for async caheloaders, see next point). Well > this was the case already, I guess. Yes, this is already the case. > 2) with async stores, the issue is back. The value is not going to be > found while it's "in flight" in the async storage queue; so unless you > want to avoid supporting it for now, the actual removal from the > container should be performed from the async cache loader code. Ugh - yes, I agree. But I hate the way the mixing of concerns is starting to look. :-( > So it looks like that - when passivation is enabled - the proper way > to implement this is to use the eviction policy only to identify > what's going to be removed from the container, but not to actually > remove it. > > I'd still focus on having the best performance possible for the case > in which passivation is not enabled, that looks like the "performance > needing" setup, or maybe we need separate eviction strategies fro when > passivation is enabled. Probably true.

</snip> 3) so every time we evict an entry, we write it to the store.. I guess that leads to re-write quite often values which are already in the store, and for which there's no need to store again. I think this argument came up before, but we didn't want to track an additional boolean to keep the memory need low. Still when we're using it as a cache with eviction rules in place, it seems to me that overall performance would be better by avoiding more writes at the cost of being able to store some less entries in memory.

Cheers. Sanne 2011/6/10 Manik Surtani <manik(a)jboss.org&gt;: > So I've taken on this sub-task from Sanne (ISPN-1169), and here is the deal: > BCHM analyses whether or not to evict stuff every time an operation (put, > get, replace) is run, based on the configured eviction policy (LIRS, LRU or > NONE). It does so by calling evictionStrategy.execute(), which returns a > Set of hash entries removed from the BCHM. > https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... > Note that execute() actually removes entries from the BCHM, so after > execute() is called, these entries are no longer in the BCHM! > This is set is then sent to the EvictionManager > https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... > which in turn passivates the evicted entries: > https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... > So we have a window where the entry is neither accessible from memory or > from a cache store, since it is "in-fly". > My proposed fix is to: > 1) Make sure the PassivationManager is passed in to the DataContainer, and > available to any EvictionStrategy > 2) EvictionStrategies ensure passivation occurs *before* removing an entry > from the BCHM > 3) The additional window of a reader thread in the ActivationInterceptor > determining that an entry is in memory and bypasses activation, only to find > that the entry has been removed from the BCHM by the time it gets to it, > should not be a problem since when testing the DataContainer in the > ActivationInterceptor, the entry is placed in the caller's InvocationContext > so a handle on the entry is there. > Does anyone have any concerns with this? Vladimir, does this adversely > affect the BCHM/EvictionStrategy design? > Cheers > Manik > On 8 Jun 2011, at 19:15, Vladimir Blagojevic wrote: > > On 11-06-08 12:10 PM, Sanne Grinovero wrote: > > Yes, we're on the same channel now. That's exactly what I meant > > yesterday on IRC when mentioning the BoundedCHM, and what I meant > > above with "we should acquire it before it's actually evicted". > > But this issue is getting complex, I've split it in 4 different > > problems and I'm going to solve the first three soon, the BoundedCHM > > is the fourth and I'm not sure I'll fix that today. > > Sanne, > > Sounds good, please consult us when it comes to changing BCHM! > > Cheers. > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev > > -- > Manik Surtani > manik(a)jboss.org > twitter.com/maniksurtani > Lead, Infinispan > http://www.infinispan.org > > > > _______________________________________________ > 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

Anyway, to summarise, does this cover everything? 1)  Passivate before evict, as described below 2)  AsyncStore checks async queue before delegating to actual cache store on load Cheers Manik

On 10 Jun 2011, at 14:18, Sanne Grinovero wrote: > It looks as a good improvement, especially as we remove the locking, > but I still have some doubts: > > 1) as the passivation happens in the eviction thread, is that going to > be fast enough or hindering the eviction performance? especially with > LIRS, being this the user thread it means that put operations suffer a > mayor performance hit (for async caheloaders, see next point). Well > this was the case already, I guess. > > 2) with async stores, the issue is back. The value is not going to be > found while it's "in flight" in the async storage queue; so unless you > want to avoid supporting it for now, the actual removal from the > container should be performed from the async cache loader code. > > So it looks like that - when passivation is enabled - the proper way > to implement this is to use the eviction policy only to identify > what's going to be removed from the container, but not to actually > remove it. > > I'd still focus on having the best performance possible for the case > in which passivation is not enabled, that looks like the "performance > needing" setup, or maybe we need separate eviction strategies fro when > passivation is enabled. > > > 3) so every time we evict an entry, we write it to the store.. I guess > that leads to re-write quite often values which are already in the > store, and for which there's no need to store again. I think this > argument came up before, but we didn't want to track an additional > boolean to keep the memory need low. Still when we're using it as a > cache with eviction rules in place, it seems to me that overall > performance would be better by avoiding more writes at the cost of > being able to store some less entries in memory. > > Cheers. > Sanne > > 2011/6/10 Manik Surtani <manik(a)jboss.org&gt;: >> So I've taken on this sub-task from Sanne (ISPN-1169), and here is the deal: >> BCHM analyses whether or not to evict stuff every time an operation (put, >> get, replace) is run, based on the configured eviction policy (LIRS, LRU or >> NONE).  It does so by calling evictionStrategy.execute(), which returns a >> Set of hash entries removed from the BCHM. >> https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... >> Note that execute() actually removes entries from the BCHM, so after >> execute() is called, these entries are no longer in the BCHM! >> This is set is then sent to the EvictionManager >> https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... >> which in turn passivates the evicted entries: >> https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... >> So we have a window where the entry is neither accessible from memory or >> from a cache store, since it is "in-fly". >> My proposed fix is to: >> 1) Make sure the PassivationManager is passed in to the DataContainer, and >> available to any EvictionStrategy >> 2) EvictionStrategies ensure passivation occurs *before* removing an entry >> from the BCHM >> 3) The additional window of a reader thread in the ActivationInterceptor >> determining that an entry is in memory and bypasses activation, only to find >> that the entry has been removed from the BCHM by the time it gets to it, >> should not be a problem since when testing the DataContainer in the >> ActivationInterceptor, the entry is placed in the caller's InvocationContext >> so a handle on the entry is there. >> Does anyone have any concerns with this?  Vladimir, does this adversely >> affect the BCHM/EvictionStrategy design? >> Cheers >> Manik >> On 8 Jun 2011, at 19:15, Vladimir Blagojevic wrote: >> >> On 11-06-08 12:10 PM, Sanne Grinovero wrote: >> >> Yes, we're on the same channel now. That's exactly what I meant >> >> yesterday on IRC when mentioning the BoundedCHM, and what I meant >> >> above with "we should acquire it before it's actually evicted". >> >> But this issue is getting complex, I've split it in 4 different >> >> problems and I'm going to solve the first three soon, the BoundedCHM >> >> is the fourth and I'm not sure I'll fix that today. >> >> Sanne, >> >> Sounds good, please consult us when it comes to changing BCHM! >> >> Cheers. >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> -- >> Manik Surtani >> manik(a)jboss.org >> twitter.com/maniksurtani >> Lead, Infinispan >> http://www.infinispan.org >> >> >> >> _______________________________________________ >> 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 -- Manik Surtani manik(a)jboss.org twitter.com/maniksurtani Lead, Infinispan http://www.infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 10 Jun 2011, at 15:35, Sanne Grinovero wrote: > 2011/6/10 Manik Surtani <manik(a)jboss.org&gt;: >> Anyway, to summarise, does this cover everything? >> >> 1) Passivate before evict, as described below >> 2) AsyncStore checks async queue before delegating to actual cache store on load >> >> Cheers >> Manik > > Yes, looks good! 2) won't be easy, AFAIR the AsyncStore has several > regions / stages where the data might be. I'd say this is a second > issue we can work on later. Agreed. Could you pls create a JIRA and detail the problem? Thanks Manik -- Manik Surtani manik(a)jboss.org twitter.com/maniksurtani Lead, Infinispan http://www.infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

So I've taken on this sub-task from Sanne (ISPN-1169), and here is the deal: BCHM analyses whether or not to evict stuff every time an operation (put, get, replace) is run, based on the configured eviction policy (LIRS, LRU or NONE). It does so by calling evictionStrategy.execute(), which returns a Set of hash entries removed from the BCHM. https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... Note that execute() actually removes entries from the BCHM, so after execute() is called, these entries are no longer in the BCHM! This is set is then sent to the EvictionManager https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... which in turn passivates the evicted entries: https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o... So we have a window where the entry is neither accessible from memory or from a cache store, since it is "in-fly". My proposed fix is to: 1) Make sure the PassivationManager is passed in to the DataContainer, and available to any EvictionStrategy 2) EvictionStrategies ensure passivation occurs *before* removing an entry from the BCHM 3) The additional window of a reader thread in the ActivationInterceptor determining that an entry is in memory and bypasses activation, only to find that the entry has been removed from the BCHM by the time it gets to it, should not be a problem since when testing the DataContainer in the ActivationInterceptor, the entry is placed in the caller's InvocationContext so a handle on the entry is there. Does anyone have any concerns with this? Vladimir, does this adversely affect the BCHM/EvictionStrategy design? Cheers Manik On 8 Jun 2011, at 19:15, Vladimir Blagojevic wrote: > On 11-06-08 12:10 PM, Sanne Grinovero wrote: >> Yes, we're on the same channel now. That's exactly what I meant >> yesterday on IRC when mentioning the BoundedCHM, and what I meant >> above with "we should acquire it before it's actually evicted". >> But this issue is getting complex, I've split it in 4 different >> problems and I'm going to solve the first three soon, the BoundedCHM >> is the fourth and I'm not sure I'll fix that today. > Sanne, > > Sounds good, please consult us when it comes to changing BCHM! > > Cheers. > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Manik Surtani manik(a)jboss.org twitter.com/maniksurtani Lead, Infinispan http://www.infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

> > 2) EvictionStrategies ensure passivation occurs *before* removing an entry from the BCHM Does this cover the following scenario? 1. thread1.get determines that K needs to be passivated 2. thread2 remove(k) removes it from memory 3. thread1. passivates it to disk then removes it from memory as well (no op) 4. thread3.read(k) returns a value - wrong. I think this won't happen as along as we have a lock on K for the passivate-then-delete-from-data-container operation. AFIK we don't do that ATM.