[576969] Revisions in the CDORevisionCache should be interned

https://bugs.eclipse.org/bugs/show_bug.cgi?id=576969

5 files changed, 114 insertions, 61 deletions

diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/AbstractCDORevisionCache.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/AbstractCDORevisionCache.java
index c60b0c084f..14ef402913 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/AbstractCDORevisionCache.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/AbstractCDORevisionCache.java
@@ -122,21 +122,6 @@ public abstract class AbstractCDORevisionCache extends Lifecycle implements Inte
   }
 
   @Override
-  public final void addRevision(CDORevision revision)
-  {
-    referenceQueue.register((InternalCDORevision)revision);
-    doAddRevision(revision);
-
-    IListener[] listeners = getListeners();
-    if (listeners != null)
-    {
-      fireEvent(new CacheAdditionEvent(this, revision));
-    }
-  }
-
-  protected abstract void doAddRevision(CDORevision revision);
-
-  @Override
   public final CDORevision removeRevision(CDOID id, CDOBranchVersion branchVersion)
   {
     referenceQueue.clean();
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionCacheAuditing.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionCacheAuditing.java
index 854089eec7..423304a896 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionCacheAuditing.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionCacheAuditing.java
@@ -23,6 +23,7 @@ import org.eclipse.emf.cdo.spi.common.revision.InternalCDORevision;
 import org.eclipse.emf.cdo.spi.common.revision.InternalCDORevisionCache;
 
 import org.eclipse.net4j.util.CheckUtil;
+import org.eclipse.net4j.util.event.IListener;
 import org.eclipse.net4j.util.om.trace.ContextTracer;
 
 import org.eclipse.emf.ecore.EClass;
@@ -36,6 +37,7 @@ import java.util.List;
 import java.util.ListIterator;
 import java.util.Map;
 import java.util.function.Consumer;
+import java.util.function.Supplier;
 
 /**
  * @author Eike Stepper
@@ -183,7 +185,7 @@ public class CDORevisionCacheAuditing extends AbstractCDORevisionCache
   }
 
   @Override
-  protected void doAddRevision(CDORevision revision)
+  public CDORevision internRevision(CDORevision revision)
   {
     CheckUtil.checkArg(revision, "revision");
 
@@ -193,18 +195,41 @@ public class CDORevisionCacheAuditing extends AbstractCDORevisionCache
     CDOID id = revision.getID();
     Object key = createKey(id, branch);
 
-    synchronized (revisionLists)
+    IListener[] listeners = getListeners();
+    CacheAdditionEvent event = null;
+
+    try
     {
-      RevisionList list = revisionLists.get(key);
-      if (list == null)
+      synchronized (revisionLists)
       {
-        list = new RevisionList();
-        revisionLists.put(key, list);
+        RevisionList list = revisionLists.get(key);
+        if (list == null)
+        {
+          list = new RevisionList();
+          revisionLists.put(key, list);
+        }
+
+        CDORevision cachedRevision = list.addRevision(revision, () -> createReference(revision));
+        if (cachedRevision != revision)
+        {
+          return cachedRevision;
+        }
+
+        typeRefIncrease(id, revision.getEClass());
       }
 
-      if (list.addRevision((InternalCDORevision)revision, createReference(revision)))
+      if (listeners.length != 0)
       {
-        typeRefIncrease(id, revision.getEClass());
+        event = new CacheAdditionEvent(this, revision);
+      }
+
+      return revision;
+    }
+    finally
+    {
+      if (event != null)
+      {
+        fireEvent(event, listeners);
       }
     }
   }
@@ -293,7 +318,7 @@ public class CDORevisionCacheAuditing extends AbstractCDORevisionCache
     {
     }
 
-    public synchronized InternalCDORevision getRevision(long timeStamp)
+    public InternalCDORevision getRevision(long timeStamp)
     {
       if (timeStamp == CDORevision.UNSPECIFIED_DATE)
       {
@@ -344,7 +369,7 @@ public class CDORevisionCacheAuditing extends AbstractCDORevisionCache
       return null;
     }
 
-    public synchronized InternalCDORevision getRevisionByVersion(int version)
+    public InternalCDORevision getRevisionByVersion(int version)
     {
       for (Iterator<Reference<InternalCDORevision>> it = iterator(); it.hasNext();)
       {
@@ -371,7 +396,7 @@ public class CDORevisionCacheAuditing extends AbstractCDORevisionCache
       return null;
     }
 
-    public synchronized boolean addRevision(InternalCDORevision revision, Reference<InternalCDORevision> reference)
+    public CDORevision addRevision(CDORevision revision, Supplier<Reference<InternalCDORevision>> referenceCreator)
     {
       int version = revision.getVersion();
       for (ListIterator<Reference<InternalCDORevision>> it = listIterator(); it.hasNext();)
@@ -384,14 +409,14 @@ public class CDORevisionCacheAuditing extends AbstractCDORevisionCache
           int v = key.getVersion();
           if (v == version)
           {
-            return false;
+            return foundRevision;
           }
 
           if (v < version)
           {
             it.previous();
-            it.add(reference);
-            return true;
+            it.add(referenceCreator.get());
+            return revision;
           }
         }
         else
@@ -400,11 +425,11 @@ public class CDORevisionCacheAuditing extends AbstractCDORevisionCache
         }
       }
 
-      addLast(reference);
-      return true;
+      addLast(referenceCreator.get());
+      return revision;
     }
 
-    public synchronized void removeRevision(int version)
+    public void removeRevision(int version)
     {
       for (Iterator<Reference<InternalCDORevision>> it = iterator(); it.hasNext();)
       {
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionCacheNonAuditing.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionCacheNonAuditing.java
index c598346bda..2dc2a99723 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionCacheNonAuditing.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionCacheNonAuditing.java
@@ -22,6 +22,7 @@ import org.eclipse.emf.cdo.spi.common.revision.InternalCDORevision;
 import org.eclipse.emf.cdo.spi.common.revision.InternalCDORevisionCache;
 
 import org.eclipse.net4j.util.CheckUtil;
+import org.eclipse.net4j.util.event.IListener;
 
 import org.eclipse.emf.ecore.EClass;
 
@@ -194,7 +195,7 @@ public class CDORevisionCacheNonAuditing extends AbstractCDORevisionCache
   }
 
   @Override
-  protected void doAddRevision(CDORevision revision)
+  public CDORevision internRevision(CDORevision revision)
   {
     CheckUtil.checkArg(revision, "revision");
     checkBranch(revision.getBranch());
@@ -202,25 +203,56 @@ public class CDORevisionCacheNonAuditing extends AbstractCDORevisionCache
     if (!revision.isHistorical())
     {
       CDOID id = revision.getID();
-      Reference<InternalCDORevision> reference = createReference(revision);
+      CDORevision passedRevision = revision;
+
+      IListener[] listeners = getListeners();
+      CacheAdditionEvent[] event = { null };
 
       synchronized (revisions)
       {
-        Reference<InternalCDORevision> oldReference = revisions.put(id, reference);
-        if (oldReference != null)
+        try
         {
-          InternalCDORevision oldRevision = oldReference.get();
-          if (oldRevision != null)
-          {
-            if (oldRevision.getVersion() > revision.getVersion())
+          revisions.compute(id, (k, cachedReference) -> {
+            if (cachedReference != null)
             {
-              // Put the old revision back because it's newer.
-              revisions.put(id, oldReference);
+              InternalCDORevision cachedRevision = cachedReference.get();
+              if (cachedRevision != null)
+              {
+                if (cachedRevision.getVersion() > passedRevision.getVersion())
+                {
+                  // Keep the cachedRevision in the cache because it's basically newer than the passedRevision,
+                  // but don't change the result of internRevision().
+                  throw new KeepCachedRevision(passedRevision);
+                }
+
+                if (cachedRevision.equals(passedRevision))
+                {
+                  // Keep the cachedRevision in the cache because it's basically equal to the passedRevision,
+                  // and change the result of internRevision() to the already cachedRevision.
+                  throw new KeepCachedRevision(cachedRevision);
+                }
+              }
             }
-          }
+
+            // No revision is already cached, so cache and return the passedRevision.
+            if (listeners.length != 0)
+            {
+              event[0] = new CacheAdditionEvent(this, passedRevision);
+            }
+
+            return createReference(passedRevision);
+          });
+        }
+        catch (KeepCachedRevision ex)
+        {
+          revision = ex.returnValue;
         }
       }
+
+      fireEvent(event[0]);
     }
+
+    return revision;
   }
 
   @Override
@@ -259,4 +291,19 @@ public class CDORevisionCacheNonAuditing extends AbstractCDORevisionCache
       revisions.clear();
     }
   }
+
+  /**
+   * @author Eike Stepper
+   */
+  private static final class KeepCachedRevision extends RuntimeException
+  {
+    private static final long serialVersionUID = 1L;
+
+    public final CDORevision returnValue;
+
+    public KeepCachedRevision(CDORevision returnValue)
+    {
+      this.returnValue = returnValue;
+    }
+  }
 }
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionManagerImpl.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionManagerImpl.java
index e44d27e1df..7911df07a6 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionManagerImpl.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/CDORevisionManagerImpl.java
@@ -26,7 +26,6 @@ import org.eclipse.emf.cdo.common.revision.CDORevisionManager;
 import org.eclipse.emf.cdo.common.revision.CDORevisionUtil;
 import org.eclipse.emf.cdo.common.revision.CDORevisionsLoadedEvent;
 import org.eclipse.emf.cdo.internal.common.bundle.OM;
-import org.eclipse.emf.cdo.spi.common.branch.CDOBranchUtil;
 import org.eclipse.emf.cdo.spi.common.revision.DetachedCDORevision;
 import org.eclipse.emf.cdo.spi.common.revision.InternalCDORevision;
 import org.eclipse.emf.cdo.spi.common.revision.InternalCDORevisionCache;
@@ -273,7 +272,7 @@ public class CDORevisionManagerImpl extends Lifecycle implements InternalCDORevi
           }
 
           revision = revisionLoader.loadRevisionByVersion(id, branchVersion, referenceChunk);
-          addRevision(revision);
+          revision = (InternalCDORevision)internRevision(revision);
         }
       }
 
@@ -517,7 +516,7 @@ public class CDORevisionManagerImpl extends Lifecycle implements InternalCDORevi
   }
 
   @Override
-  public void addRevision(CDORevision revision)
+  public CDORevision internRevision(CDORevision revision)
   {
     if (revision != null)
     {
@@ -525,18 +524,6 @@ public class CDORevisionManagerImpl extends Lifecycle implements InternalCDORevi
 
       try
       {
-        if (revision instanceof PointerCDORevision)
-        {
-          PointerCDORevision pointer = (PointerCDORevision)revision;
-          CDOBranchVersion target = pointer.getTarget();
-          if (target instanceof InternalCDORevision)
-          {
-            // Replace the target CDORevision by a proper CDOBranchVersion.
-            revision = new PointerCDORevision(pointer.getEClass(), pointer.getID(), pointer.getBranch(), pointer.getRevised(),
-                CDOBranchUtil.copyBranchVersion(target));
-          }
-        }
-
         int oldVersion = revision.getVersion() - 1;
         if (oldVersion >= CDORevision.UNSPECIFIED_VERSION)
         {
@@ -564,13 +551,22 @@ public class CDORevisionManagerImpl extends Lifecycle implements InternalCDORevi
           }
         }
 
-        cache.addRevision(revision);
+        revision = cache.internRevision(revision);
       }
       finally
       {
         releaseAtomicRequestLock(loadAndAddLock);
       }
     }
+
+    return revision;
+  }
+
+  @Deprecated
+  @Override
+  public void addRevision(CDORevision revision)
+  {
+    internRevision(revision);
   }
 
   @Override
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/NOOPRevisionCache.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/NOOPRevisionCache.java
index 7f30198a6e..a8ece823e1 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/NOOPRevisionCache.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/NOOPRevisionCache.java
@@ -87,9 +87,9 @@ public class NOOPRevisionCache extends Lifecycle implements InternalCDORevisionC
   }
 
   @Override
-  public void addRevision(CDORevision revision)
+  public CDORevision internRevision(CDORevision revision)
   {
-    // Do nothing
+    return revision;
   }
 
   @Override