[jbosscache-commits] JBoss Cache SVN: r7869 - in core/branches/flat/src: main/java/org/horizon/container and 10 other directories.

jbosscache-commits at lists.jboss.org jbosscache-commits at lists.jboss.org
Fri Mar 6 10:32:19 EST 2009


Author: manik.surtani at jboss.com
Date: 2009-03-06 10:32:19 -0500 (Fri, 06 Mar 2009)
New Revision: 7869

Added:
   core/branches/flat/src/main/java/org/horizon/util/BidirectionalMap.java
   core/branches/flat/src/test/java/org/horizon/atomic/AtomicMapFunctionalTest.java
Modified:
   core/branches/flat/src/main/java/org/horizon/atomic/AtomicHashMapProxy.java
   core/branches/flat/src/main/java/org/horizon/container/ReadCommittedEntry.java
   core/branches/flat/src/main/java/org/horizon/context/AbstractContext.java
   core/branches/flat/src/main/java/org/horizon/context/EntryLookup.java
   core/branches/flat/src/main/java/org/horizon/context/InvocationContextImpl.java
   core/branches/flat/src/main/java/org/horizon/context/TransactionContextImpl.java
   core/branches/flat/src/main/java/org/horizon/factories/EntryFactory.java
   core/branches/flat/src/main/java/org/horizon/factories/EntryFactoryImpl.java
   core/branches/flat/src/main/java/org/horizon/interceptors/CacheLoaderInterceptor.java
   core/branches/flat/src/main/java/org/horizon/interceptors/LockingInterceptor.java
   core/branches/flat/src/main/java/org/horizon/lock/LockManager.java
   core/branches/flat/src/main/java/org/horizon/lock/StripedLockManager.java
   core/branches/flat/src/main/java/org/horizon/notifications/cachelistener/CacheNotifierImpl.java
   core/branches/flat/src/main/java/org/horizon/util/BidirectionalLinkedHashMap.java
   core/branches/flat/src/main/java/org/horizon/util/HorizonCollections.java
   core/branches/flat/src/test/java/org/horizon/api/mvcc/LockAssert.java
   core/branches/flat/src/test/java/org/horizon/api/tree/TreeCacheAPITest.java
   core/branches/flat/src/test/java/org/horizon/loader/CacheLoaderFunctionalTest.java
Log:
Refactored the way locks are recorded in contexts, to reduce number of maps held in the context and reduce unnecessary put/get/remove on internal maps.

Modified: core/branches/flat/src/main/java/org/horizon/atomic/AtomicHashMapProxy.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/atomic/AtomicHashMapProxy.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/atomic/AtomicHashMapProxy.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -26,6 +26,8 @@
 import org.horizon.batch.BatchContainer;
 import org.horizon.invocation.InvocationContextContainer;
 import org.horizon.invocation.Options;
+import org.horizon.logging.Log;
+import org.horizon.logging.LogFactory;
 
 import java.util.Collection;
 import java.util.Map;
@@ -38,6 +40,8 @@
  * @since 1.0
  */
 public class AtomicHashMapProxy<K, V> extends AutoBatchSupport implements AtomicMap<K, V> {
+   private static final Log log = LogFactory.getLog(AtomicHashMapProxy.class);
+   private static final boolean trace = log.isTraceEnabled();
    Object deltaMapKey;
    Cache cache;
    InvocationContextContainer icc;
@@ -62,6 +66,13 @@
          boolean suppressLocks = icc.get().hasOption(Options.SKIP_LOCKING);
          if (!suppressLocks) icc.get().setOptions(Options.FORCE_WRITE_LOCK);
 
+         if (trace) {
+            if (suppressLocks)
+               log.trace("Skip locking option used.  Skipping locking.");
+            else
+               log.trace("Forcing write lock even for reads");
+         }
+
          AtomicHashMap map = getDeltaMapForRead();
          // copy for write
          AtomicHashMap copy = map == null ? new AtomicHashMap() : map.copyForWrite();

Modified: core/branches/flat/src/main/java/org/horizon/container/ReadCommittedEntry.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/container/ReadCommittedEntry.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/container/ReadCommittedEntry.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -26,7 +26,8 @@
 import org.horizon.logging.LogFactory;
 
 /**
- * A wrapper around a cached entry that encapsulates read committed semantics when writes are initiated, committed or rolled back.
+ * A wrapper around a cached entry that encapsulates read committed semantics when writes are initiated, committed or
+ * rolled back.
  *
  * @author Manik Surtani (<a href="mailto:manik at jboss.org">manik at jboss.org</a>)
  * @since 1.0
@@ -73,7 +74,7 @@
    }
 
    protected static enum Flags {
-      CHANGED(0x1), CREATED(0x2), DELETED(0x4), VALID(0x8);
+      CHANGED(1), CREATED(1 << 1), DELETED(1 << 2), VALID(1 << 3);
       final byte mask;
 
       Flags(int mask) {
@@ -144,8 +145,10 @@
    }
 
    public void rollbackUpdate() {
-      value = oldValue;
-      reset();
+      if (isFlagSet(CHANGED)) {
+         value = oldValue;
+         reset();
+      }
    }
 
    public boolean isChanged() {

Modified: core/branches/flat/src/main/java/org/horizon/context/AbstractContext.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/context/AbstractContext.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/context/AbstractContext.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -2,9 +2,8 @@
 
 import org.horizon.container.MVCCEntry;
 import org.horizon.invocation.Options;
-import org.horizon.util.FastCopyHashMap;
-import org.horizon.util.ReversibleOrderedSet;
-import org.horizon.util.VisitableBidirectionalLinkedHashSet;
+import org.horizon.util.BidirectionalLinkedHashMap;
+import org.horizon.util.BidirectionalMap;
 
 import java.util.Arrays;
 import java.util.Collection;
@@ -20,14 +19,13 @@
  */
 public abstract class AbstractContext {
 
-   protected EnumSet<Options> options;
+   protected volatile EnumSet<Options> options;
    protected byte contextFlags;
-   protected ReversibleOrderedSet<Object> locks;
-   protected FastCopyHashMap<Object, MVCCEntry> lookedUpEntries = null;
+   protected BidirectionalLinkedHashMap<Object, MVCCEntry> lookedUpEntries = null;
 
-
    protected static enum ContextFlags {
-      FORCE_SYNCHRONOUS(0x1), FORCE_ASYNCHRONOUS(0x2), ORIGIN_LOCAL(0x4), LOCAL_ROLLBACK_ONLY(0x8);
+      FORCE_SYNCHRONOUS(1), FORCE_ASYNCHRONOUS(1 << 1), ORIGIN_LOCAL(1 << 2), LOCAL_ROLLBACK_ONLY(1 << 3),
+      CONTAINS_MODS(1 << 4), CONTAINS_LOCKS(1 << 5);
       final byte mask;
 
       ContextFlags(int mask) {
@@ -88,29 +86,11 @@
 
    protected abstract int getLockSetSize();
 
-   public void addKeyLocked(Object lock) {
-      // no need to worry about concurrency here - a context is only valid for a single thread.
-      if (locks == null) locks = new VisitableBidirectionalLinkedHashSet<Object>(false, getLockSetSize());
-      locks.add(lock);
+   public boolean hasLockedKey(Object key) {
+      MVCCEntry e = lookupEntry(key);
+      return e != null && e.isChanged();
    }
 
-   public void removeKeyLocked(Object lock) {
-      if (locks != null) locks.remove(lock);
-   }
-
-   public void clearKeysLocked() {
-      if (locks != null) locks.clear();
-   }
-
-   public boolean hasLockedKey(Object lock) {
-      return locks != null && locks.contains(lock);
-   }
-
-   public void addAllKeysLocked(Collection<Object> keys) {
-      if (locks == null) locks = new VisitableBidirectionalLinkedHashSet<Object>(false, getLockSetSize());
-      locks.addAll(keys);
-   }
-
    public MVCCEntry lookupEntry(Object key) {
       return lookedUpEntries.get(key);
    }
@@ -127,7 +107,7 @@
       lookedUpEntries.clear();
    }
 
-   public Map<Object, MVCCEntry> getLookedUpEntries() {
+   public BidirectionalMap<Object, MVCCEntry> getLookedUpEntries() {
       return lookedUpEntries;
    }
 
@@ -137,7 +117,6 @@
 
    public void reset() {
       if (lookedUpEntries != null) lookedUpEntries.clear();
-      if (locks != null) locks.clear();
       options = null;
       contextFlags = 0;
    }
@@ -150,7 +129,6 @@
       AbstractContext that = (AbstractContext) o;
 
       if (contextFlags != that.contextFlags) return false;
-      if (locks != null ? !locks.equals(that.locks) : that.locks != null) return false;
       if (lookedUpEntries != null ? !lookedUpEntries.equals(that.lookedUpEntries) : that.lookedUpEntries != null)
          return false;
       if (options != null ? !options.equals(that.options) : that.options != null) return false;
@@ -162,7 +140,6 @@
    public int hashCode() {
       int result = options != null ? options.hashCode() : 0;
       result = 31 * result + (int) contextFlags;
-      result = 31 * result + (locks != null ? locks.hashCode() : 0);
       result = 31 * result + (lookedUpEntries != null ? lookedUpEntries.hashCode() : 0);
       return result;
    }
@@ -171,7 +148,23 @@
    protected void copyInto(AbstractContext ctx) {
       if (options != null) ctx.options = EnumSet.copyOf(options);
       ctx.contextFlags = contextFlags;
-      if (locks != null) ctx.locks = new VisitableBidirectionalLinkedHashSet<Object>(false, locks);
-      if (lookedUpEntries != null) ctx.lookedUpEntries = (FastCopyHashMap<Object, MVCCEntry>) lookedUpEntries.clone();
+      if (lookedUpEntries != null)
+         ctx.lookedUpEntries = (BidirectionalLinkedHashMap<Object, MVCCEntry>) lookedUpEntries.clone();
    }
+
+   public boolean isContainsModifications() {
+      return isFlagSet(ContextFlags.CONTAINS_MODS);
+   }
+
+   public void setContainsModifications(boolean b) {
+      setFlag(ContextFlags.CONTAINS_MODS, b);
+   }
+
+   public boolean isContainsLocks() {
+      return isFlagSet(ContextFlags.CONTAINS_LOCKS);
+   }
+
+   public void setContainsLocks(boolean b) {
+      setFlag(ContextFlags.CONTAINS_LOCKS, b);
+   }
 }

Modified: core/branches/flat/src/main/java/org/horizon/context/EntryLookup.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/context/EntryLookup.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/context/EntryLookup.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -22,9 +22,8 @@
 package org.horizon.context;
 
 import org.horizon.container.MVCCEntry;
-import org.horizon.util.ReversibleOrderedSet;
+import org.horizon.util.BidirectionalMap;
 
-import java.util.Collection;
 import java.util.Map;
 
 /**
@@ -55,7 +54,7 @@
     *
     * @return a map of looked up entries.
     */
-   Map<Object, MVCCEntry> getLookedUpEntries();
+   BidirectionalMap<Object, MVCCEntry> getLookedUpEntries();
 
    /**
     * Puts an entry in the registry of looked up entries in the current scope.
@@ -79,65 +78,31 @@
    void clearLookedUpEntries();
 
    /**
-    * Returns the set of locks currently maintained for the current scope.  Note that this set is ordered.
-    * <p/>
-    * Note that if a transaction is in scope, implementations should retrieve these locks from the {@link
-    * TransactionContext}. Retrieving locks from this method should always ensure they are retrieved from  the
-    * appropriate scope.
+    * Note that if a transaction is in scope, implementations should test this lock from on {@link TransactionContext}.
+    * Using this method should always ensure locks checked in the appropriate scope.
     *
-    * @return keys locked in current scope.
+    * @param key lock to test
+    * @return true if the lock being tested is already held in the current scope, false otherwise.
     */
-   ReversibleOrderedSet<Object> getKeysLocked();
+   boolean hasLockedKey(Object key);
 
    /**
-    * Adds a List of locks to the currently maintained collection of locks acquired.
-    * <p/>
-    * Note that if a transaction is in scope, implementations should record locks on the {@link TransactionContext}.
-    * Adding locks using this method should always ensure they are applied to the appropriate scope.
-    * <p/>
-    *
-    * @param keys keys locked
+    * @return true if the context contains modifications, false otherwise
     */
-   void addAllKeysLocked(Collection<Object> keys);
+   boolean isContainsModifications();
 
    /**
-    * Adds a lock to the currently maintained collection of locks acquired.
-    * <p/>
-    * Note that if a transaction is in scope, implementations should record this lock on the {@link TransactionContext}.
-    * Using this method should always ensure that the appropriate scope is used.
-    * <p/>
-    *
-    * @param key key that was locked
+    * Sets whether modifications have been made in the current context
     */
-   void addKeyLocked(Object key);
+   void setContainsModifications(boolean b);
 
    /**
-    * Removes a lock from the currently maintained collection of locks acquired.
-    * <p/>
-    * Note that if a transaction is in scope, implementations should remove this lock from the {@link
-    * TransactionContext}. Using this method should always ensure that the lock is removed from the appropriate scope.
-    * <p/>
-    *
-    * @param key key that was unlocked
+    * @return true if the context contains locks, false otherwise
     */
-   void removeKeyLocked(Object key);
+   boolean isContainsLocks();
 
    /**
-    * Clears all locks from the currently maintained collection of locks acquired.
-    * <p/>
-    * Note that if a transaction is in scope, implementations should clear locks from the {@link TransactionContext}.
-    * Using this method should always ensure locks are cleared in the appropriate scope.
-    * <p/>
+    * Sets whether locks have been acquired in the current context
     */
-   void clearKeysLocked();
-
-   /**
-    * Note that if a transaction is in scope, implementations should test this lock from on {@link TransactionContext}.
-    * Using this method should always ensure locks checked in the appropriate scope.
-    *
-    * @param key lock to test
-    * @return true if the lock being tested is already held in the current scope, false otherwise.
-    */
-   boolean hasLockedKey(Object key);
-
+   void setContainsLocks(boolean b);
 }

Modified: core/branches/flat/src/main/java/org/horizon/context/InvocationContextImpl.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/context/InvocationContextImpl.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/context/InvocationContextImpl.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -24,13 +24,11 @@
 import org.horizon.container.MVCCEntry;
 import org.horizon.transaction.GlobalTransaction;
 import org.horizon.transaction.TransactionTable;
-import org.horizon.util.FastCopyHashMap;
+import org.horizon.util.BidirectionalLinkedHashMap;
+import org.horizon.util.BidirectionalMap;
 import org.horizon.util.HorizonCollections;
-import org.horizon.util.ReversibleOrderedSet;
 
 import javax.transaction.Transaction;
-import java.util.Collection;
-import java.util.Collections;
 import java.util.Map;
 
 public class InvocationContextImpl extends AbstractContext implements InvocationContext {
@@ -49,10 +47,16 @@
    }
 
    private void initLookedUpEntries() {
-      if (lookedUpEntries == null) lookedUpEntries = new FastCopyHashMap<Object, MVCCEntry>(4);
+      if (lookedUpEntries == null) lookedUpEntries = new BidirectionalLinkedHashMap<Object, MVCCEntry>(4);
    }
 
    @Override
+   public boolean hasLockedKey(Object key) {
+      if (transactionContext != null) return transactionContext.hasLockedKey(key);
+      return super.hasLockedKey(key);
+   }
+
+   @Override
    public MVCCEntry lookupEntry(Object k) {
       if (transactionContext != null) {
          return transactionContext.lookupEntry(k);
@@ -86,7 +90,7 @@
          transactionContext.putLookedUpEntries(lookedUpEntries);
       else {
          initLookedUpEntries();
-         lookedUpEntries.putAll(lookedUpEntries);
+         this.lookedUpEntries.putAll(lookedUpEntries);
       }
    }
 
@@ -96,9 +100,10 @@
    }
 
    @SuppressWarnings("unchecked")
-   public Map<Object, MVCCEntry> getLookedUpEntries() {
+   public BidirectionalMap<Object, MVCCEntry> getLookedUpEntries() {
       if (transactionContext != null) return transactionContext.getLookedUpEntries();
-      return (Map<Object, MVCCEntry>) (lookedUpEntries == null ? Collections.emptyMap() : lookedUpEntries);
+      return (BidirectionalMap<Object, MVCCEntry>)
+            (lookedUpEntries == null ? HorizonCollections.emptyBidirectionalMap() : lookedUpEntries);
    }
 
    @SuppressWarnings("unchecked")
@@ -185,57 +190,6 @@
       return isFlagSet(ContextFlags.ORIGIN_LOCAL);
    }
 
-   public ReversibleOrderedSet<Object> getKeysLocked() {
-      // first check transactional scope
-      if (transactionContext != null) return transactionContext.getKeysLocked();
-      return locks == null ? HorizonCollections.emptyReversibleOrderedSet() : locks;
-   }
-
-   @Override
-   public void addAllKeysLocked(Collection<Object> keys) {
-      // first check transactional scope
-      if (transactionContext != null)
-         transactionContext.addAllKeysLocked(keys);
-      else
-         super.addAllKeysLocked(keys);
-   }
-
-   @Override
-   public void addKeyLocked(Object key) {
-      // first check transactional scope
-      if (transactionContext != null)
-         transactionContext.addKeyLocked(key);
-      else
-         super.addKeyLocked(key);
-   }
-
-   @Override
-   public void removeKeyLocked(Object key) {
-      // first check transactional scope
-      if (transactionContext != null)
-         transactionContext.removeKeyLocked(key);
-      else
-         super.removeKeyLocked(key);
-   }
-
-   @Override
-   public void clearKeysLocked() {
-      // first check transactional scope
-      if (transactionContext != null)
-         transactionContext.clearKeysLocked();
-      else
-         super.clearKeysLocked();
-   }
-
-   @Override
-   public boolean hasLockedKey(Object key) {
-      // first check transactional scope
-      if (transactionContext != null)
-         return transactionContext.hasLockedKey(key);
-      else
-         return super.hasLockedKey(key);
-   }
-
    /**
     * If set to true, the invocation is assumed to have originated locally.  If set to false, assumed to have originated
     * from a remote cache.
@@ -292,6 +246,32 @@
    }
 
    @Override
+   public boolean isContainsModifications() {
+      return transactionContext == null ? super.isContainsModifications() : transactionContext.isContainsModifications();
+   }
+
+   @Override
+   public void setContainsModifications(boolean b) {
+      if (transactionContext == null)
+         super.setContainsModifications(b);
+      else
+         transactionContext.setContainsModifications(b);
+   }
+
+   @Override
+   public boolean isContainsLocks() {
+      return transactionContext == null ? super.isContainsLocks() : transactionContext.isContainsLocks();
+   }
+
+   @Override
+   public void setContainsLocks(boolean b) {
+      if (transactionContext == null)
+         super.setContainsLocks(b);
+      else
+         transactionContext.setContainsLocks(b);
+   }
+
+   @Override
    public boolean equals(Object o) {
       if (this == o) return true;
       if (o == null || getClass() != o.getClass()) return false;
@@ -325,8 +305,8 @@
             ", transactionContext=" + transactionContext +
             ", options=" + options +
             ", contextFlags=" + contextFlags +
-            ", invocationLocks=" + locks +
-            ", lookedUpEntries=" + lookedUpEntries +
+//            ", invocationLocks=" + locks +
+            ", lookedUpEntries size=" + (lookedUpEntries == null ? 0 : lookedUpEntries.size()) +
             '}';
    }
 }

Modified: core/branches/flat/src/main/java/org/horizon/context/TransactionContextImpl.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/context/TransactionContextImpl.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/context/TransactionContextImpl.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -24,10 +24,7 @@
 import org.horizon.commands.write.WriteCommand;
 import org.horizon.container.MVCCEntry;
 import org.horizon.transaction.GlobalTransaction;
-import org.horizon.util.FastCopyHashMap;
-import org.horizon.util.HorizonCollections;
-import org.horizon.util.Immutables;
-import org.horizon.util.ReversibleOrderedSet;
+import org.horizon.util.BidirectionalLinkedHashMap;
 
 import javax.transaction.RollbackException;
 import javax.transaction.SystemException;
@@ -80,7 +77,7 @@
 
    public TransactionContextImpl(Transaction tx) throws SystemException, RollbackException {
       ltx = tx;
-      lookedUpEntries = new FastCopyHashMap<Object, MVCCEntry>(8);
+      lookedUpEntries = new BidirectionalLinkedHashMap<Object, MVCCEntry>(8);
    }
 
    public void reset() {
@@ -89,7 +86,7 @@
       localModifications = null;
       if (dummyEntriesCreatedByCacheLoader != null) dummyEntriesCreatedByCacheLoader.clear();
       if (removedKeys != null) removedKeys.clear();
-      lookedUpEntries = new FastCopyHashMap<Object, MVCCEntry>(8);
+      lookedUpEntries = new BidirectionalLinkedHashMap<Object, MVCCEntry>(8);
    }
 
    public GlobalTransaction getGobalTransaction() {
@@ -197,9 +194,9 @@
       return hasModifications() || hasLocalModifications();
    }
 
-   public ReversibleOrderedSet<Object> getKeysLocked() {
-      return locks == null ? HorizonCollections.emptyReversibleOrderedSet() : Immutables.immutableReversibleOrderedSetCopy(locks);
-   }
+//   public ReversibleOrderedSet<Object> getKeysLocked() {
+//      return locks == null ? HorizonCollections.emptyReversibleOrderedSet() : Immutables.immutableReversibleOrderedSetCopy(locks);
+//   }
 
    @Override
    public boolean equals(Object o) {

Modified: core/branches/flat/src/main/java/org/horizon/factories/EntryFactory.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/factories/EntryFactory.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/factories/EntryFactory.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -32,7 +32,7 @@
  * @since 1.0
  */
 public interface EntryFactory {
-   void releaseLock(InvocationContext ctx, Object key);
+   void releaseLock(Object key);
 
    /**
     * Attempts to lock an entry if the lock isn't already held in the current scope, and records the lock in the
@@ -48,7 +48,7 @@
     */
    boolean acquireLock(InvocationContext ctx, Object key) throws InterruptedException, TimeoutException;
 
-   MVCCEntry wrapEntryForWriting(InvocationContext ctx, Object key, boolean createIfAbsent, boolean forceLockIfAbsent) throws InterruptedException;
+   MVCCEntry wrapEntryForWriting(InvocationContext ctx, Object key, boolean createIfAbsent, boolean forceLockIfAbsent, boolean alreadyLocked) throws InterruptedException;
 
    MVCCEntry wrapEntryForReading(InvocationContext ctx, Object key) throws InterruptedException;
 }

Modified: core/branches/flat/src/main/java/org/horizon/factories/EntryFactoryImpl.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/factories/EntryFactoryImpl.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/factories/EntryFactoryImpl.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -75,7 +75,7 @@
       MVCCEntry mvccEntry;
       if (ctx.hasOption(Options.FORCE_WRITE_LOCK)) {
          if (trace) log.trace("Forcing lock on reading");
-         return wrapEntryForWriting(ctx, key, false, false);
+         return wrapEntryForWriting(ctx, key, false, false, false);
       } else if ((mvccEntry = ctx.lookupEntry(key)) == null) {
          if (trace) log.trace("Key " + key + " is not in context, fetching from container.");
          // simple implementation.  Peek the entry, wrap it, put wrapped entry in the context.
@@ -99,13 +99,13 @@
       }
    }
 
-   public final MVCCEntry wrapEntryForWriting(InvocationContext ctx, Object key, boolean createIfAbsent, boolean forceLockIfAbsent) throws InterruptedException {
+   public final MVCCEntry wrapEntryForWriting(InvocationContext ctx, Object key, boolean createIfAbsent, boolean forceLockIfAbsent, boolean alreadyLocked) throws InterruptedException {
       MVCCEntry mvccEntry = ctx.lookupEntry(key);
       if (createIfAbsent && mvccEntry != null && mvccEntry.isNullEntry()) mvccEntry = null;
       if (mvccEntry != null) // exists in context!  Just acquire lock if needed, and wrap.
       {
          // acquire lock if needed
-         if (acquireLock(ctx, key)) {
+         if (alreadyLocked || acquireLock(ctx, key)) {
             // create a copy of the underlying entry
             mvccEntry.copyForUpdate(container, writeSkewCheck);
          }
@@ -122,8 +122,7 @@
             if (trace) log.trace("Retrieved from container.");
             // exists in cache!  Just acquire lock if needed, and wrap.
             // do we need a lock?
-            boolean needToCopy = false;
-            if (acquireLock(ctx, key)) needToCopy = true;
+            boolean needToCopy = alreadyLocked || acquireLock(ctx, key) || ctx.hasOption(Options.SKIP_LOCKING); // even if we do not acquire a lock, if skip-locking is enabled we should copy
             mvccEntry = createWrappedEntry(key, cachedValue.getValue(), false, cachedValue.getLifespan());
             ctx.putLookedUpEntry(key, mvccEntry);
             if (needToCopy) mvccEntry.copyForUpdate(container, writeSkewCheck);
@@ -131,7 +130,7 @@
             // this is the *only* point where new entries can be created!!
             if (trace) log.trace("Creating new entry.");
             // now to lock and create the entry.  Lock first to prevent concurrent creation!
-            acquireLock(ctx, key);
+            if (!alreadyLocked) acquireLock(ctx, key);
             notifier.notifyCacheEntryCreated(key, true, ctx);
             mvccEntry = createWrappedEntry(key, null, true, -1);
             mvccEntry.setCreated(true);
@@ -142,7 +141,10 @@
       }
 
       // see if we need to force the lock on nonexistent entries.
-      if (mvccEntry == null && forceLockIfAbsent) acquireLock(ctx, key);
+      if (mvccEntry == null && forceLockIfAbsent) {
+         // make sure we record this! Null value since this is a forced lock on the key
+         if (acquireLock(ctx, key)) ctx.putLookedUpEntry(key, null);
+      }
 
       return mvccEntry;
    }
@@ -164,17 +166,20 @@
       // lock which may be shared with another key that we have a lock for already.
       // nothing wrong, just means that we fail to record the lock.  And that is a problem.
       // Better to check our records and lock again if necessary.
-      if (!ctx.hasLockedKey(key)) {
-         if (ctx.hasOption(Options.SKIP_LOCKING)) {
-            // just record this in the ctx and rtn
-            ctx.addKeyLocked(key);
-         } else if (!lockManager.lockAndRecord(key, ctx)) {
+
+      boolean shouldSkipLocking = ctx.hasOption(Options.SKIP_LOCKING);
+
+      if (!ctx.hasLockedKey(key) && !shouldSkipLocking) {
+         if (lockManager.lockAndRecord(key, ctx)) {
+            // successfully locked!
+            return true;
+         } else {
             Object owner = lockManager.getOwner(key);
             throw new TimeoutException("Unable to acquire lock on key [" + key + "] after [" + getLockAcquisitionTimeout(ctx)
                   + "] milliseconds for requestor [" + lockManager.getLockOwner(ctx) + "]! Lock held by [" + owner + "]");
          }
-         return true;
       }
+
       return false;
    }
 
@@ -183,8 +188,7 @@
             0 : configuration.getLockAcquisitionTimeout();
    }
 
-   public final void releaseLock(InvocationContext ctx, Object key) {
+   public final void releaseLock(Object key) {
       lockManager.unlock(key, lockManager.getOwner(key));
-      ctx.removeKeyLocked(key);
    }
 }

Modified: core/branches/flat/src/main/java/org/horizon/interceptors/CacheLoaderInterceptor.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/interceptors/CacheLoaderInterceptor.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/interceptors/CacheLoaderInterceptor.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -69,33 +69,38 @@
 
    @Override
    public Object visitPutKeyValueCommand(InvocationContext ctx, PutKeyValueCommand command) throws Throwable {
-      if (command.getKey() != null) loadIfNeeded(ctx, command.getKey());
+      Object key;
+      if ((key = command.getKey()) != null) loadIfNeeded(ctx, key);
       return invokeNextInterceptor(ctx, command);
    }
 
 
    @Override
    public Object visitGetKeyValueCommand(InvocationContext ctx, GetKeyValueCommand command) throws Throwable {
-      if (command.getKey() != null) loadIfNeeded(ctx, command.getKey());
+      Object key;
+      if ((key = command.getKey()) != null) loadIfNeeded(ctx, key);
       return invokeNextInterceptor(ctx, command);
    }
 
    @Override
    public Object visitInvalidateCommand(InvocationContext ctx, InvalidateCommand command) throws Throwable {
-      if (command.getKeys() != null)
+      Object[] keys;
+      if ((keys = command.getKeys()) != null && keys.length > 0)
          for (Object key : command.getKeys()) loadIfNeeded(ctx, key);
       return invokeNextInterceptor(ctx, command);
    }
 
    @Override
    public Object visitRemoveCommand(InvocationContext ctx, RemoveCommand command) throws Throwable {
-      if (command.getKey() != null) loadIfNeeded(ctx, command.getKey());
+      Object key;
+      if ((key = command.getKey()) != null) loadIfNeeded(ctx, key);
       return invokeNextInterceptor(ctx, command);
    }
 
    @Override
    public Object visitReplaceCommand(InvocationContext ctx, ReplaceCommand command) throws Throwable {
-      if (command.getKey() != null) loadIfNeeded(ctx, command.getKey());
+      Object key;
+      if ((key = command.getKey()) != null) loadIfNeeded(ctx, key);
       return invokeNextInterceptor(ctx, command);
    }
 
@@ -106,16 +111,17 @@
       }
 
       // Obtain a temporary lock to verify the key is not being concurrently added
-      boolean release = entryFactory.acquireLock(ctx, key);
+      boolean keyLocked = entryFactory.acquireLock(ctx, key);
       if (dataContainer.containsKey(key)) {
-         if (release) entryFactory.releaseLock(ctx, key);
+         if (keyLocked) entryFactory.releaseLock(key);
          log.trace("No need to load.  Key exists in the data container.");
          return;
       }
 
       // Reuse the lock and create a new entry for loading
-      MVCCEntry n = entryFactory.wrapEntryForWriting(ctx, key, true, false);
+      MVCCEntry n = entryFactory.wrapEntryForWriting(ctx, key, true, false, keyLocked);
       n = loadEntry(ctx, key, n);
+      ctx.setContainsModifications(true);
    }
 
    /**

Modified: core/branches/flat/src/main/java/org/horizon/interceptors/LockingInterceptor.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/interceptors/LockingInterceptor.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/interceptors/LockingInterceptor.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -78,8 +78,7 @@
    public Object visitCommitCommand(InvocationContext ctx, CommitCommand command) throws Throwable {
       try {
          return invokeNextInterceptor(ctx, command);
-      }
-      finally {
+      } finally {
          transactionalCleanup(true, ctx);
       }
    }
@@ -88,8 +87,7 @@
    public Object visitRollbackCommand(InvocationContext ctx, RollbackCommand command) throws Throwable {
       try {
          return invokeNextInterceptor(ctx, command);
-      }
-      finally {
+      } finally {
          transactionalCleanup(false, ctx);
       }
    }
@@ -98,8 +96,7 @@
    public Object visitPrepareCommand(InvocationContext ctx, PrepareCommand command) throws Throwable {
       try {
          return invokeNextInterceptor(ctx, command);
-      }
-      finally {
+      } finally {
          if (command.isOnePhaseCommit()) transactionalCleanup(true, ctx);
       }
    }
@@ -111,8 +108,7 @@
       try {
          entryFactory.wrapEntryForReading(ctx, command.getKey());
          return invokeNextInterceptor(ctx, command);
-      }
-      finally {
+      } finally {
          doAfterCall(ctx);
       }
    }
@@ -122,8 +118,7 @@
       try {
          if (log.isDebugEnabled()) log.debug("No locking performed for SizeCommands");
          return invokeNextInterceptor(ctx, command);
-      }
-      finally {
+      } finally {
          doAfterCall(ctx);
       }
    }
@@ -134,11 +129,10 @@
    public Object visitClearCommand(InvocationContext ctx, ClearCommand command) throws Throwable {
       try {
          // get a snapshot of all keys in the data container
-         for (Object key : dataContainer.keySet()) entryFactory.wrapEntryForWriting(ctx, key, false, false);
-
+         for (Object key : dataContainer.keySet()) entryFactory.wrapEntryForWriting(ctx, key, false, false, false);
+         ctx.setContainsModifications(true);
          return invokeNextInterceptor(ctx, command);
-      }
-      finally {
+      } finally {
          doAfterCall(ctx);
       }
    }
@@ -153,11 +147,12 @@
    public Object visitInvalidateCommand(InvocationContext ctx, InvalidateCommand command) throws Throwable {
       try {
          if (command.getKeys() != null) {
-            for (Object key : command.getKeys()) entryFactory.wrapEntryForWriting(ctx, key, false, true);
+            for (Object key : command.getKeys()) entryFactory.wrapEntryForWriting(ctx, key, false, true, false);
          }
-         return invokeNextInterceptor(ctx, command);
-      }
-      finally {
+         Object o = invokeNextInterceptor(ctx, command);
+         if (!ctx.isContainsModifications()) ctx.setContainsModifications(command.isSuccessful());
+         return o;
+      } finally {
          doAfterCall(ctx);
       }
    }
@@ -165,10 +160,11 @@
    @Override
    public Object visitPutKeyValueCommand(InvocationContext ctx, PutKeyValueCommand command) throws Throwable {
       try {
-         entryFactory.wrapEntryForWriting(ctx, command.getKey(), true, false);
-         return invokeNextInterceptor(ctx, command);
-      }
-      finally {
+         entryFactory.wrapEntryForWriting(ctx, command.getKey(), true, false, false);
+         Object o = invokeNextInterceptor(ctx, command);
+         if (!ctx.isContainsModifications()) ctx.setContainsModifications(command.isSuccessful());
+         return o;
+      } finally {
          doAfterCall(ctx);
       }
    }
@@ -177,9 +173,11 @@
    public Object visitPutMapCommand(InvocationContext ctx, PutMapCommand command) throws Throwable {
       try {
          for (Object key : command.getMap().keySet()) {
-            entryFactory.wrapEntryForWriting(ctx, key, true, false);
+            entryFactory.wrapEntryForWriting(ctx, key, true, false, false);
          }
-         return invokeNextInterceptor(ctx, command);
+         Object o = invokeNextInterceptor(ctx, command);
+         if (!ctx.isContainsModifications()) ctx.setContainsModifications(command.isSuccessful());
+         return o;
       }
       finally {
          doAfterCall(ctx);
@@ -189,8 +187,10 @@
    @Override
    public Object visitRemoveCommand(InvocationContext ctx, RemoveCommand command) throws Throwable {
       try {
-         entryFactory.wrapEntryForWriting(ctx, command.getKey(), false, true);
-         return invokeNextInterceptor(ctx, command);
+         entryFactory.wrapEntryForWriting(ctx, command.getKey(), false, true, false);
+         Object o = invokeNextInterceptor(ctx, command);
+         if (!ctx.isContainsModifications()) ctx.setContainsModifications(command.isSuccessful());
+         return o;
       }
       finally {
          doAfterCall(ctx);
@@ -200,8 +200,10 @@
    @Override
    public Object visitReplaceCommand(InvocationContext ctx, ReplaceCommand command) throws Throwable {
       try {
-         entryFactory.wrapEntryForWriting(ctx, command.getKey(), false, true);
-         return invokeNextInterceptor(ctx, command);
+         entryFactory.wrapEntryForWriting(ctx, command.getKey(), false, true, false);
+         Object o = invokeNextInterceptor(ctx, command);
+         if (!ctx.isContainsModifications()) ctx.setContainsModifications(command.isSuccessful());
+         return o;
       }
       finally {
          doAfterCall(ctx);
@@ -212,9 +214,8 @@
    private void doAfterCall(InvocationContext ctx) {
       // for non-transactional stuff.
       if (ctx.getTransactionContext() == null) {
-         ReversibleOrderedSet<Object> locks;
-         if (!(locks = ctx.getKeysLocked()).isEmpty()) {
-            cleanupLocks(locks, ctx, Thread.currentThread(), true);
+         if (ctx.isContainsModifications() || ctx.isContainsLocks()) {
+            cleanupLocks(ctx, Thread.currentThread(), true);
          } else {
             if (trace) log.trace("Nothing to do since there are no modifications in scope.");
          }
@@ -227,54 +228,80 @@
                // should never resize.
                List<Object> keysToRemove = new ArrayList<Object>(lookedUpEntries.size());
                for (Map.Entry<Object, MVCCEntry> e : lookedUpEntries.entrySet()) {
-                  if (!e.getValue().isChanged()) keysToRemove.add(e.getKey());
+                  if (!lockManager.possiblyLocked(e.getValue())) keysToRemove.add(e.getKey());
                }
 
                if (!keysToRemove.isEmpty()) {
+                  if (trace)
+                     log.trace("Removing keys {0} since they have not been modified.  Context currently contains {1} keys", keysToRemove, ctx.getLookedUpEntries().size());
                   for (Object key : keysToRemove) ctx.removeLookedUpEntry(key);
+                  if (trace) log.trace("After removal, context contains {0} keys", ctx.getLookedUpEntries().size());
                }
             }
          }
       }
    }
 
-   private void cleanupLocks(ReversibleOrderedSet<Object> keysLocked, InvocationContext ctx, Object owner, boolean commit) {
+   private void cleanupLocks(InvocationContext ctx, Object owner, boolean commit) {
       // clean up.
       // unlocking needs to be done in reverse order.
-      Iterator<Object> it = keysLocked.reverseIterator();
+      ReversibleOrderedSet entries = ctx.getLookedUpEntries().entrySet();
+      Iterator<Map.Entry<Object, MVCCEntry>> it = entries.reverseIterator();
+      if (trace) log.trace("Number of entries in context: {0}", entries.size());
 
       if (commit) {
          while (it.hasNext()) {
-            Object key = it.next();
-            MVCCEntry entry = ctx.lookupEntry(key);
+            Map.Entry<Object, MVCCEntry> e = it.next();
+            MVCCEntry entry = e.getValue();
+            Object key = e.getKey();
+            boolean needToUnlock = lockManager.possiblyLocked(entry);
             // could be null with read-committed
-            if (entry != null) entry.commitUpdate(dataContainer);
+            if (entry != null)
+               entry.commitUpdate(dataContainer);
+            else {
+               if (trace) log.trace("Entry for key {0} is null, not calling commitUpdate", key);
+            }
+
             // and then unlock
-            if (trace) log.trace("Releasing lock on [" + key + "] for owner " + owner);
-            lockManager.unlock(key, owner);
+            if (needToUnlock) {
+               if (trace) log.trace("Releasing lock on [" + key + "] for owner " + owner);
+               lockManager.unlock(key, owner);
+            }
          }
       } else {
          while (it.hasNext()) {
-            Object key = it.next();
-            MVCCEntry entry = ctx.lookupEntry(key);
+            Map.Entry<Object, MVCCEntry> e = it.next();
+            MVCCEntry entry = e.getValue();
+            Object key = e.getKey();
+            boolean needToUnlock = lockManager.possiblyLocked(entry);
             // could be null with read-committed
-            if (entry != null) entry.rollbackUpdate();
+            if (entry != null)
+               entry.rollbackUpdate();
+            else {
+               if (trace) log.trace("Entry for key {0} is null, not calling rollbackUpdate", key);
+            }
             // and then unlock
-            if (trace) log.trace("Releasing lock on [" + key + "] for owner " + owner);
-            lockManager.unlock(key, owner);
+            if (needToUnlock) {
+               if (trace) log.trace("Releasing lock on [" + key + "] for owner " + owner);
+               lockManager.unlock(key, owner);
+            }
          }
       }
-      ctx.clearKeysLocked();
+      ctx.setContainsModifications(false);
+      ctx.setContainsLocks(false);
    }
 
    @SuppressWarnings("unchecked")
    private void transactionalCleanup(boolean commit, InvocationContext ctx) {
       if (ctx.getTransactionContext() != null) {
-         ReversibleOrderedSet<Object> locks = ctx.getTransactionContext().getKeysLocked();
-         if (!locks.isEmpty())
-            cleanupLocks(locks, ctx, ctx.getGlobalTransaction(), commit);
-         else if (trace)
-            log.trace("At transaction boundary (" + (commit ? "commit" : "rollback") + "), and we have no locks in context!");
+         if (ctx.isContainsModifications() || ctx.isContainsLocks()) {
+            if (trace)
+               log.trace("Performing cleanup.  Contains mods? {0} Contains locks? {1}", ctx.isContainsModifications(), ctx.isContainsLocks());
+            cleanupLocks(ctx, ctx.getGlobalTransaction(), commit);
+         } else {
+            if (trace)
+               log.trace("At transaction boundary (" + (commit ? "commit" : "rollback") + "), and we have no locks in context!");
+         }
       } else {
          throw new IllegalStateException("Attempting to do a commit or rollback but there is no transactional context in scope. " + ctx);
       }

Modified: core/branches/flat/src/main/java/org/horizon/lock/LockManager.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/lock/LockManager.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/lock/LockManager.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -21,6 +21,7 @@
  */
 package org.horizon.lock;
 
+import org.horizon.container.MVCCEntry;
 import org.horizon.context.InvocationContext;
 
 /**
@@ -41,30 +42,6 @@
    Object getLockOwner(InvocationContext ctx);
 
    /**
-    * Acquires a lock of type lockType, for a given owner, on a specific entry in the cache.  This method will try for
-    * {@link org.horizon.config.Configuration#getLockAcquisitionTimeout()} milliseconds and give up if it is unable to
-    * acquire the required lock.
-    *
-    * @param key   key to lock
-    * @param owner owner to acquire the lock for
-    * @return true if the lock was acquired, false otherwise.
-    * @throws InterruptedException
-    */
-   boolean lock(Object key, Object owner) throws InterruptedException;
-
-   /**
-    * Acquires a lock of type lockType, for a given owner, on a specific entry in the cache.  This method will try for
-    * timeout milliseconds and give up if it is unable to acquire the required lock.
-    *
-    * @param key     key to lock
-    * @param owner   owner to acquire the lock for
-    * @param timeout maximum length of time to wait for (in millis)
-    * @return true if the lock was acquired, false otherwise.
-    * @throws InterruptedException if interrupted
-    */
-   boolean lock(Object key, Object owner, long timeout) throws InterruptedException;
-
-   /**
     * Acquires a lock of type lockType, on a specific entry in the cache.  This method will try for a period of time and
     * give up if it is unable to acquire the required lock.  The period of time is specified in {@link
     * org.horizon.config.Option#getLockAcquisitionTimeout()} and, if this is unset, the default timeout set in {@link
@@ -131,4 +108,18 @@
     * @return lock information
     */
    String printLockInfo();
+
+   /**
+    * Inspects the entry for signs that it is possibly locked, and hence would need to be unlocked.  Note that this is
+    * not deterministic, and is pessimistic in that even if an entry is not locked but *might* be locked, this will
+    * return true.
+    * <p/>
+    * As such, this should only be used to determine whether *unlocking* is necessary, not whether locking is necessary.
+    * Unlocking an entry that has not been locked has no effect, so this is just an optimisation.
+    * <p/>
+    *
+    * @param entry entry to inspect
+    * @return true if the entry *might* be locked, false if the entry definitely is *not* locked.
+    */
+   boolean possiblyLocked(MVCCEntry entry);
 }

Modified: core/branches/flat/src/main/java/org/horizon/lock/StripedLockManager.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/lock/StripedLockManager.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/lock/StripedLockManager.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -22,6 +22,7 @@
 package org.horizon.lock;
 
 import org.horizon.config.Configuration;
+import org.horizon.container.MVCCEntry;
 import org.horizon.context.InvocationContext;
 import org.horizon.factories.annotations.Inject;
 import org.horizon.factories.annotations.Start;
@@ -37,6 +38,7 @@
 
 import javax.transaction.TransactionManager;
 import java.util.Iterator;
+import java.util.Map;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import java.util.concurrent.locks.Lock;
 
@@ -70,23 +72,12 @@
       return ctx.getGlobalTransaction() != null ? ctx.getGlobalTransaction() : Thread.currentThread();
    }
 
-   public boolean lock(Object key, Object owner) throws InterruptedException {
-      if (trace) log.trace("Attempting to lock " + key);
-      Lock lock = lockContainer.getLock(key);
-      return lock.tryLock(configuration.getLockAcquisitionTimeout(), MILLISECONDS);
-   }
-
-   public boolean lock(Object key, Object owner, long timeoutMillis) throws InterruptedException {
-      if (trace) log.trace("Attempting to lock " + key);
-      Lock lock = lockContainer.getLock(key);
-      return lock.tryLock(timeoutMillis, MILLISECONDS);
-   }
-
    public boolean lockAndRecord(Object key, InvocationContext ctx) throws InterruptedException {
-      if (trace) log.trace("Attempting to lock " + key);
+      long lockTimeout = getLockAcquisitionTimeout(ctx);
+      if (trace) log.trace("Attempting to lock {0} with acquisition timeout of {1} millis", key, lockTimeout);
       Lock lock = lockContainer.getLock(key);
-      if (lock.tryLock(getLockAcquisitionTimeout(ctx), MILLISECONDS)) {
-         ctx.addKeyLocked(key);
+      if (lock.tryLock(lockTimeout, MILLISECONDS)) {
+         ctx.setContainsLocks(true);
          return true;
       }
 
@@ -107,14 +98,19 @@
 
    @SuppressWarnings("unchecked")
    public void unlock(InvocationContext ctx) {
-      ReversibleOrderedSet<Object> locks = ctx.getKeysLocked();
-      if (!locks.isEmpty()) {
+      ReversibleOrderedSet<Map.Entry<Object, MVCCEntry>> entries = ctx.getLookedUpEntries().entrySet();
+      if (!entries.isEmpty()) {
          // unlocking needs to be done in reverse order.
-         Iterator<Object> it = locks.reverseIterator();
+         Iterator<Map.Entry<Object, MVCCEntry>> it = entries.reverseIterator();
          while (it.hasNext()) {
-            Object k = it.next();
-            if (trace) log.trace("Attempting to unlock " + k);
-            lockContainer.getLock(k).unlock();
+            Map.Entry<Object, MVCCEntry> e = it.next();
+            MVCCEntry entry = e.getValue();
+            if (possiblyLocked(entry)) {
+               // has been locked!
+               Object k = e.getKey();
+               if (trace) log.trace("Attempting to unlock " + k);
+               lockContainer.getLock(k).unlock();
+            }
          }
       }
    }
@@ -143,4 +139,8 @@
    public String printLockInfo() {
       return lockContainer.toString();
    }
+
+   public final boolean possiblyLocked(MVCCEntry entry) {
+      return entry == null || entry.isChanged() || entry.isNullEntry();
+   }
 }

Modified: core/branches/flat/src/main/java/org/horizon/notifications/cachelistener/CacheNotifierImpl.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/notifications/cachelistener/CacheNotifierImpl.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/notifications/cachelistener/CacheNotifierImpl.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -306,7 +306,7 @@
       // get a new Invocation Context
       InvocationContext newContext = icc.get();
       newContext.putLookedUpEntries(ctx.getLookedUpEntries());
-      newContext.addAllKeysLocked(ctx.getKeysLocked());
+//      newContext.addAllKeysLocked(ctx.getKeysLocked());
       return ctx;
    }
 }

Modified: core/branches/flat/src/main/java/org/horizon/util/BidirectionalLinkedHashMap.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/util/BidirectionalLinkedHashMap.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/util/BidirectionalLinkedHashMap.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -22,7 +22,7 @@
  * @author Manik Surtani
  * @since 1.0
  */
-public class BidirectionalLinkedHashMap<K, V> extends AbstractMap<K, V> implements Cloneable {
+public class BidirectionalLinkedHashMap<K, V> extends AbstractMap<K, V> implements BidirectionalMap<K, V>, Cloneable {
 
    /**
     * The head of the doubly linked list.

Added: core/branches/flat/src/main/java/org/horizon/util/BidirectionalMap.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/util/BidirectionalMap.java	                        (rev 0)
+++ core/branches/flat/src/main/java/org/horizon/util/BidirectionalMap.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -0,0 +1,16 @@
+package org.horizon.util;
+
+import java.util.Map;
+
+/**
+ * An extension of Map that returns ReversibleOrderedSets
+ *
+ * @author Manik Surtani
+ * @since 1.0
+ */
+public interface BidirectionalMap<K, V> extends Map<K, V> {
+
+   ReversibleOrderedSet<K> keySet();
+
+   ReversibleOrderedSet<Map.Entry<K, V>> entrySet();
+}

Modified: core/branches/flat/src/main/java/org/horizon/util/HorizonCollections.java
===================================================================
--- core/branches/flat/src/main/java/org/horizon/util/HorizonCollections.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/main/java/org/horizon/util/HorizonCollections.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -1,7 +1,10 @@
 package org.horizon.util;
 
 import java.util.AbstractSet;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.Iterator;
+import java.util.Map;
 import java.util.NoSuchElementException;
 
 /**
@@ -12,12 +15,18 @@
  */
 public class HorizonCollections {
    private static final ReversibleOrderedSet EMPTY_ROS = new EmptyReversibleOrderedSet();
+   private static final BidirectionalMap EMPTY_BIDI_MAP = new EmptyBidiMap();
 
    @SuppressWarnings("unchecked")
    public static final <T> ReversibleOrderedSet<T> emptyReversibleOrderedSet() {
       return (ReversibleOrderedSet<T>) EMPTY_ROS;
    }
 
+   @SuppressWarnings("unchecked")
+   public static final <K, V> BidirectionalMap<K, V> emptyBidirectionalMap() {
+      return (BidirectionalMap<K, V>) EMPTY_BIDI_MAP;
+   }
+
    private static final class EmptyReversibleOrderedSet extends AbstractSet implements ReversibleOrderedSet {
 
       Iterator it = new Iterator() {
@@ -47,4 +56,55 @@
          return it;
       }
    }
+
+   private static final class EmptyBidiMap extends AbstractMap implements BidirectionalMap {
+
+      public int size() {
+         return 0;
+      }
+
+      public boolean isEmpty() {
+         return true;
+      }
+
+      public boolean containsKey(Object key) {
+         return false;
+      }
+
+      public boolean containsValue(Object value) {
+         return false;
+      }
+
+      public Object get(Object key) {
+         return null;
+      }
+
+      public Object put(Object key, Object value) {
+         throw new UnsupportedOperationException();
+      }
+
+      public Object remove(Object key) {
+         throw new UnsupportedOperationException();
+      }
+
+      public void putAll(Map t) {
+         throw new UnsupportedOperationException();
+      }
+
+      public void clear() {
+         throw new UnsupportedOperationException();
+      }
+
+      public ReversibleOrderedSet keySet() {
+         return EMPTY_ROS;
+      }
+
+      public Collection values() {
+         return Collections.emptySet();
+      }
+
+      public ReversibleOrderedSet entrySet() {
+         return EMPTY_ROS;
+      }
+   }
 }

Modified: core/branches/flat/src/test/java/org/horizon/api/mvcc/LockAssert.java
===================================================================
--- core/branches/flat/src/test/java/org/horizon/api/mvcc/LockAssert.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/test/java/org/horizon/api/mvcc/LockAssert.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -1,5 +1,6 @@
 package org.horizon.api.mvcc;
 
+import org.horizon.Cache;
 import org.horizon.invocation.InvocationContextContainer;
 import org.horizon.lock.LockManager;
 import org.horizon.test.TestingUtil;
@@ -18,12 +19,19 @@
 
    public static void assertNotLocked(Object key, InvocationContextContainer icc) {
       // can't rely on the negative test since other entries may share the same lock with lock striping.
-      assert !icc.get().getKeysLocked().contains(key) : key + " lock recorded!";
+      assert !icc.get().hasLockedKey(key) : key + " lock recorded!";
    }
 
    public static void assertNoLocks(LockManager lockManager, InvocationContextContainer icc) {
       LockContainer lc = (LockContainer) TestingUtil.extractField(lockManager, "lockContainer");
       assert lc.getNumLocksHeld() == 0 : "Stale locks exist! NumLocksHeld is " + lc.getNumLocksHeld() + " and lock info is " + lockManager.printLockInfo();
-      assert icc.get().getKeysLocked().isEmpty() : "Stale (?) locks recorded! " + icc.get().getKeysLocked();
+      assert !icc.get().isContainsModifications() : "Stale (?) locks recorded!";
    }
+
+   public static void assertNoLocks(Cache cache) {
+      LockManager lockManager = TestingUtil.extractComponentRegistry(cache).getComponent(LockManager.class);
+      InvocationContextContainer icc = TestingUtil.extractComponentRegistry(cache).getComponent(InvocationContextContainer.class);
+
+      assertNoLocks(lockManager, icc);
+   }
 }

Modified: core/branches/flat/src/test/java/org/horizon/api/tree/TreeCacheAPITest.java
===================================================================
--- core/branches/flat/src/test/java/org/horizon/api/tree/TreeCacheAPITest.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/test/java/org/horizon/api/tree/TreeCacheAPITest.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -1,14 +1,19 @@
 package org.horizon.api.tree;
 
 import org.horizon.Cache;
+import org.horizon.atomic.AtomicMap;
+import org.horizon.atomic.AtomicMapCache;
 import org.horizon.config.Configuration;
+import org.horizon.logging.Log;
+import org.horizon.logging.LogFactory;
 import org.horizon.manager.DefaultCacheManager;
+import org.horizon.test.TestingUtil;
 import org.horizon.transaction.DummyTransactionManagerLookup;
 import org.horizon.tree.Fqn;
 import org.horizon.tree.Node;
+import org.horizon.tree.NodeKey;
 import org.horizon.tree.TreeCache;
 import org.horizon.tree.TreeCacheImpl;
-import org.horizon.test.TestingUtil;
 import static org.testng.AssertJUnit.*;
 import org.testng.annotations.AfterMethod;
 import org.testng.annotations.BeforeMethod;
@@ -28,6 +33,7 @@
 public class TreeCacheAPITest {
    private TreeCache<String, String> cache;
    private TransactionManager tm;
+   private Log log = LogFactory.getLog(TreeCacheAPITest.class);
 
    @BeforeMethod(alwaysRun = true)
    public void setUp() throws Exception {
@@ -94,14 +100,33 @@
 
       // Check that it's removed if it has a child
       Fqn child = Fqn.fromString("/test/fqn/child");
+      log.error("TEST: Adding child " + child);
       cache.getRoot().addChild(child);
-      assertTrue(cache.getRoot().hasChild(child));
+      assertStructure(cache, "/test/fqn/child");
 
       assertEquals(true, cache.removeNode(fqn));
       assertFalse(cache.getRoot().hasChild(fqn));
       assertEquals(false, cache.removeNode(fqn));
    }
 
+   private void assertStructure(TreeCache tc, String fqnStr) {
+      // make sure structure nodes are properly built and maintained
+      AtomicMapCache c = (AtomicMapCache) tc.getCache();
+      Fqn fqn = Fqn.fromString(fqnStr);
+      // loop thru the Fqn, starting at its root, and make sure all of its children exist in proper NodeKeys
+      for (int i = 0; i < fqn.size(); i++) {
+         Fqn parent = fqn.getSubFqn(0, i);
+         Object childName = fqn.get(i);
+         // make sure a data key exists in the cache
+         assert c.containsKey(new NodeKey(parent, NodeKey.Type.DATA)) : "Node [" + parent + "] does not have a Data atomic map!";
+         assert c.containsKey(new NodeKey(parent, NodeKey.Type.STRUCTURE)) : "Node [" + parent + "] does not have a Structure atomic map!";
+         AtomicMap am = c.getAtomicMap(new NodeKey(parent, NodeKey.Type.STRUCTURE));
+         boolean hasChild = am.containsKey(childName);
+         assert hasChild : "Node [" + parent + "] does not have a child [" + childName + "] in its Structure atomic map!";
+      }
+   }
+
+
    public void testStopClearsData() throws Exception {
       Fqn a = Fqn.fromString("/a");
       Fqn b = Fqn.fromString("/a/b");

Added: core/branches/flat/src/test/java/org/horizon/atomic/AtomicMapFunctionalTest.java
===================================================================
--- core/branches/flat/src/test/java/org/horizon/atomic/AtomicMapFunctionalTest.java	                        (rev 0)
+++ core/branches/flat/src/test/java/org/horizon/atomic/AtomicMapFunctionalTest.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -0,0 +1,120 @@
+package org.horizon.atomic;
+
+import org.horizon.config.Configuration;
+import org.horizon.context.InvocationContext;
+import org.horizon.invocation.InvocationContextContainer;
+import static org.horizon.invocation.Options.SKIP_LOCKING;
+import org.horizon.logging.Log;
+import org.horizon.logging.LogFactory;
+import org.horizon.manager.CacheManager;
+import org.horizon.manager.DefaultCacheManager;
+import org.horizon.test.TestingUtil;
+import org.horizon.transaction.DummyTransactionManagerLookup;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import javax.transaction.Transaction;
+import javax.transaction.TransactionManager;
+
+ at Test(groups = "functional", testName = "atomic.AtomicMapFunctionalTest")
+public class AtomicMapFunctionalTest {
+   private static final Log log = LogFactory.getLog(AtomicMapFunctionalTest.class);
+   AtomicMapCache<String, String> cache;
+   TransactionManager tm;
+
+   @BeforeMethod
+   @SuppressWarnings("unchecked")
+   public void setUp() {
+      Configuration c = new Configuration();
+      // these 2 need to be set to use the AtomicMapCache
+      c.setTransactionManagerLookupClass(DummyTransactionManagerLookup.class.getName());
+      c.setInvocationBatchingEnabled(true);
+      CacheManager cm = new DefaultCacheManager(c);
+      cache = (AtomicMapCache<String, String>) cm.getCache();
+      tm = TestingUtil.getTransactionManager(cache);
+   }
+
+   @AfterMethod
+   public void tearDown() {
+      TestingUtil.killCaches(cache);
+   }
+
+   public void testChangesOnAtomicMap() {
+      AtomicMap<String, String> map = cache.getAtomicMap("key", String.class, String.class);
+      assert map.isEmpty();
+      map.put("a", "b");
+      assert map.get("a").equals("b");
+
+      // now re-retrieve the map and make sure we see the diffs
+      assert cache.getAtomicMap("key", String.class, String.class).get("a").equals("b");
+   }
+
+   public void testTxChangesOnAtomicMap() throws Exception {
+      AtomicMap<String, String> map = cache.getAtomicMap("key", String.class, String.class);
+      tm.begin();
+      assert map.isEmpty();
+      map.put("a", "b");
+      assert map.get("a").equals("b");
+      Transaction t = tm.suspend();
+
+      assert cache.getAtomicMap("key", String.class, String.class).get("a") == null;
+
+      tm.resume(t);
+      tm.commit();
+
+      // now re-retrieve the map and make sure we see the diffs
+      assert cache.getAtomicMap("key", String.class, String.class).get("a").equals("b");
+   }
+
+   public void testChangesOnAtomicMapNoLocks() {
+      AtomicMap<String, String> map = cache.getAtomicMap("key", String.class, String.class);
+      assert map.isEmpty();
+      InvocationContextContainer icc = TestingUtil.extractComponent(cache, InvocationContextContainer.class);
+      InvocationContext ic = icc.get();
+      ic.setOptions(SKIP_LOCKING);
+      log.debug("Doing a put");
+      assert icc.get().hasOption(SKIP_LOCKING);
+      map.put("a", "b");
+      log.debug("Put complete");
+      assert map.get("a").equals("b");
+
+      // now re-retrieve the map and make sure we see the diffs
+      assert cache.getAtomicMap("key", String.class, String.class).get("a").equals("b");
+   }
+
+   public void testTxChangesOnAtomicMapNoLocks() throws Exception {
+      AtomicMap<String, String> map = cache.getAtomicMap("key", String.class, String.class);
+      tm.begin();
+      assert map.isEmpty();
+      TestingUtil.extractComponent(cache, InvocationContextContainer.class).get().setOptions(SKIP_LOCKING);
+      map.put("a", "b");
+      assert map.get("a").equals("b");
+      Transaction t = tm.suspend();
+
+      assert cache.getAtomicMap("key", String.class, String.class).get("a") == null;
+
+      tm.resume(t);
+      tm.commit();
+
+      // now re-retrieve the map and make sure we see the diffs
+      assert cache.getAtomicMap("key", String.class, String.class).get("a").equals("b");
+   }
+
+   public void testChangesOnAtomicMapNoLocksExistingData() {
+      AtomicMap<String, String> map = cache.getAtomicMap("key", String.class, String.class);
+      assert map.isEmpty();
+      map.put("x", "y");
+      assert map.get("x").equals("y");
+      TestingUtil.extractComponent(cache, InvocationContextContainer.class).get().setOptions(SKIP_LOCKING);
+      log.debug("Doing a put");
+      map.put("a", "b");
+      log.debug("Put complete");
+      assert map.get("a").equals("b");
+      assert map.get("x").equals("y");
+
+      // now re-retrieve the map and make sure we see the diffs
+      assert cache.getAtomicMap("key", String.class, String.class).get("x").equals("y");
+      assert cache.getAtomicMap("key", String.class, String.class).get("a").equals("b");
+   }
+}

Modified: core/branches/flat/src/test/java/org/horizon/loader/CacheLoaderFunctionalTest.java
===================================================================
--- core/branches/flat/src/test/java/org/horizon/loader/CacheLoaderFunctionalTest.java	2009-03-05 19:45:48 UTC (rev 7868)
+++ core/branches/flat/src/test/java/org/horizon/loader/CacheLoaderFunctionalTest.java	2009-03-06 15:32:19 UTC (rev 7869)
@@ -1,6 +1,7 @@
 package org.horizon.loader;
 
 import org.horizon.Cache;
+import static org.horizon.api.mvcc.LockAssert.assertNoLocks;
 import org.horizon.config.CacheLoaderManagerConfig;
 import org.horizon.config.Configuration;
 import org.horizon.container.DataContainer;
@@ -148,26 +149,38 @@
       assertNotInCacheAndStore("k1", "k2", "k3", "k4");
 
       cache.replace("k1", "v1-SHOULD-NOT-STORE");
+      assertNoLocks(cache);
       cache.replace("k2", "v2-SHOULD-NOT-STORE", lifespan, MILLISECONDS);
+      assertNoLocks(cache);
 
       assertNotInCacheAndStore("k1", "k2", "k3", "k4");
 
       cache.put("k1", "v1");
+      assertNoLocks(cache);
       cache.put("k2", "v2");
+      assertNoLocks(cache);
       cache.put("k3", "v3");
+      assertNoLocks(cache);
       cache.put("k4", "v4");
+      assertNoLocks(cache);
 
       for (int i = 1; i < 5; i++) assertInCacheAndStore("k" + i, "v" + i);
 
       cache.replace("k1", "v1-SHOULD-NOT-STORE", "v1-STILL-SHOULD-NOT-STORE");
+      assertNoLocks(cache);
       cache.replace("k2", "v2-SHOULD-NOT-STORE", "v2-STILL-SHOULD-NOT-STORE", lifespan, MILLISECONDS);
+      assertNoLocks(cache);
 
       for (int i = 1; i < 5; i++) assertInCacheAndStore("k" + i, "v" + i);
 
       cache.replace("k1", "v1-REPLACED");
+      assertNoLocks(cache);
       cache.replace("k2", "v2-REPLACED", lifespan, MILLISECONDS);
+      assertNoLocks(cache);
       cache.replace("k3", "v3", "v3-REPLACED");
+      assertNoLocks(cache);
       cache.replace("k4", "v4", "v4-REPLACED", lifespan, MILLISECONDS);
+      assertNoLocks(cache);
 
       for (int i = 1; i < 5; i++) {
          // even numbers have lifespans
@@ -177,6 +190,7 @@
             assertInCacheAndStore("k" + i, "v" + i + "-REPLACED", lifespan);
       }
 
+      assertNoLocks(cache);
    }
 
    public void testLoading() throws CacheLoaderException {
@@ -187,21 +201,32 @@
       store.store(new StoredEntry("k4", "v4"));
 
       for (int i = 1; i < 5; i++) assert cache.get("k" + i).equals("v" + i);
+      // make sure we have no stale locks!!
+      assertNoLocks(cache);
 
       for (int i = 1; i < 5; i++) cache.evict("k" + i);
+      // make sure we have no stale locks!!
+      assertNoLocks(cache);
 
       assert cache.putIfAbsent("k1", "v1-SHOULD-NOT-STORE").equals("v1");
       assert cache.remove("k2").equals("v2");
       assert cache.replace("k3", "v3-REPLACED").equals("v3");
       assert cache.replace("k4", "v4", "v4-REPLACED");
+      // make sure we have no stale locks!!
+      assertNoLocks(cache);
 
       assert cache.size() == 3 : "Expected the cache to contain 3 elements but contained " + cache.size();
 
       for (int i = 1; i < 5; i++) cache.evict("k" + i);
+      // make sure we have no stale locks!!
+      assertNoLocks(cache);
+
       assert cache.size() == 0; // cache size ops will not trigger a load
 
       cache.clear(); // this should propagate to the loader though
       assertNotInCacheAndStore("k1", "k2", "k3", "k4");
+      // make sure we have no stale locks!!
+      assertNoLocks(cache);
    }
 
    public void testPreloading() throws CacheLoaderException {




More information about the jbosscache-commits mailing list