From: Tuukka Lehtonen <tuukka.lehtonen@semantum.fi>
Date: Thu, 12 Apr 2018 21:43:36 +0000 (+0300)
Subject: Fixed ClusterTable cluster hash/importance map book-keeping problem
X-Git-Tag: v1.43.0~136^2~495
X-Git-Url: https://gerrit.simantics.org/r/gitweb?p=simantics%2Fplatform.git;a=commitdiff_plain;h=51f45bdf95551409666a04f92355638f83c018da

Fixed ClusterTable cluster hash/importance map book-keeping problem

The main problem was that importance values should have been started
from 1 and 0 is considered an invalid importance value. This prevents
ClusterTable.replaceCluster from making mistakes in importanceMap.

refs #7724

Change-Id: Ie15a2d11bef2b8bdbcee2f5baba47633faa97826
(cherry picked from commit f59872e129c2d1b84fe322ae120fbeac22a94f88)
---

diff --git a/bundles/org.simantics.db.procore/src/fi/vtt/simantics/procore/internal/ClusterTable.java b/bundles/org.simantics.db.procore/src/fi/vtt/simantics/procore/internal/ClusterTable.java
index 2c65ee8e8..5daf534fe 100644
--- a/bundles/org.simantics.db.procore/src/fi/vtt/simantics/procore/internal/ClusterTable.java
+++ b/bundles/org.simantics.db.procore/src/fi/vtt/simantics/procore/internal/ClusterTable.java
@@ -18,6 +18,7 @@ import java.util.HashMap;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.concurrent.Semaphore;
+import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.Consumer;
 
 import org.simantics.databoard.Bindings;
@@ -65,7 +66,7 @@ public final class ClusterTable implements IClusterTable {
     int maximumBytes = 128 * 1024 * 1024;
     int limit = (int)(0.8*(double)maximumBytes);
 
-    long timeCounter = 0;
+    private final AtomicLong timeCounter = new AtomicLong(1);
 
     final private SessionImplSocket sessionImpl;
     final private ArrayList<ClusterI> writeOnlyClusters = new ArrayList<ClusterI>();
@@ -93,6 +94,10 @@ public final class ClusterTable implements IClusterTable {
         public long getClusterId() {
             return clusterId;
         }
+        @Override
+        public String toString() {
+            return "CID " + clusterId;
+        }
     }
     private class Clusters {
         // This makes sure that non-null values from hashMap.get can be trusted (no unsynchronized rehashes occur)
@@ -271,27 +276,35 @@ public final class ClusterTable implements IClusterTable {
             writeOnlyClusters.add(proxy);
             return clusters.create(proxy);
         } else {
+            //printMaps("makeCluster");
             ClusterImpl cluster = ClusterImpl.make(clusterUID, clusterKey, sessionImpl.clusterTranslator);
             clusters.create(cluster);
             if (!cluster.isLoaded())
                 Logger.defaultLogError(new Exception("Bug in ClusterTable.makeCluster(long, boolean), cluster not loaded"));
             importanceMap.put(cluster.getImportance(), new ImportanceEntry(cluster));
+            if (VALIDATE_SIZE)
+                validateSize("makeCluster");
             if(collectorPolicy != null) collectorPolicy.added(cluster);
             return cluster;
         }
     }
 
     synchronized void replaceCluster(ClusterI cluster) {
+        //printMaps("replaceCluster");
         checkCollect();
         int clusterKey = cluster.getClusterKey();
         ClusterI existing = clusterArray[clusterKey];
         if (existing.hasVirtual())
             cluster.markVirtual();
 
-        importanceMap.remove(existing.getImportance());
+        long existingImportance = existing.getImportance();
+        if (existingImportance != 0)
+            importanceMap.remove(existingImportance);
         if (collectorPolicy != null)
             collectorPolicy.removed((ClusterImpl)existing);
 
+        //System.out.println("ClusterTable.replace(" + existing + " (I=" + existing.getImportance() + ") => " + cluster + " (I=" + cluster.getImportance() + ")");
+
         clusters.replace((ClusterImpl)cluster);
         if (!cluster.isLoaded())
             Logger.defaultLogError(new Exception("Bug in ClusterTable.replaceCluster(ClusterI), cluster not loaded"));
@@ -306,7 +319,8 @@ public final class ClusterTable implements IClusterTable {
             // This will update sizeInBytes through adjustCachedSize
             cluster.getCachedSize();
         }
-        validateSize();
+        if (VALIDATE_SIZE)
+            validateSize("replaceCluster");
     }
 
     synchronized void release(CollectorCluster cluster) {
@@ -322,6 +336,7 @@ public final class ClusterTable implements IClusterTable {
             return;
         if(!clusterImpl.isLoaded() || clusterImpl.isEmpty())
             return;
+        //printMaps("release");
         clusters.freeProxy(clusterImpl);
         importanceMap.remove(clusterImpl.getImportance());
         if (collectorPolicy != null)
@@ -331,6 +346,8 @@ public final class ClusterTable implements IClusterTable {
         if (!dirtySizeInBytes) {
             adjustCachedSize(-clusterImpl.getCachedSize(), clusterImpl);
         }
+        if (VALIDATE_SIZE)
+            validateSize("release");
     }
 
     synchronized void compact(long id) {
@@ -687,6 +704,7 @@ public final class ClusterTable implements IClusterTable {
             ClientChangesImpl cs = new ClientChangesImpl(session);
             if (session.clientChanges == null)
                 session.clientChanges = cs;
+            //printMaps("refresh");
             for (int i=0; i<clusterUID.length; ++i) {
                 try {
                     if (DebugPolicy.REPORT_CLUSTER_EVENTS)
@@ -739,6 +757,8 @@ public final class ClusterTable implements IClusterTable {
                     Logger.defaultLogError("Failed to load cluster in refresh.", t);
                 }
             }
+            if (VALIDATE_SIZE)
+                validateSize("refresh");
             // Fake update of cluster changes.
             QueryProcessor queryProcessor = session.getQueryProvider2();
             WriteGraphImpl writer = WriteGraphImpl.create(queryProcessor, session.writeSupport, null);
@@ -799,6 +819,8 @@ public final class ClusterTable implements IClusterTable {
         if (c.isWriteOnly())
             return;
 
+        //printMaps("refreshImportance");
+
         importanceMap.remove(c.getImportance());
         if(collectorPolicy != null) collectorPolicy.removed(c);
 
@@ -810,6 +832,8 @@ public final class ClusterTable implements IClusterTable {
 
         importanceMap.put(c.getImportance(), new ImportanceEntry(c));
         if(collectorPolicy != null) collectorPolicy.added(c);
+        if (VALIDATE_SIZE)
+            validateSize("refreshImportance");
 
     }
 
@@ -1128,7 +1152,7 @@ public final class ClusterTable implements IClusterTable {
     }
 
     public long timeCounter() {
-        return timeCounter++;
+        return timeCounter.getAndIncrement();
     }
 
     public boolean hasVirtual(int index) {
@@ -1176,12 +1200,15 @@ public final class ClusterTable implements IClusterTable {
         }
     }
 
-    private void validateSize() {
+    private void validateSize(String place) {
         if (!VALIDATE_SIZE)
             return;
 
-//        System.out.println("validating cached cluster sizes: " + sizeInBytes + ", hashMap.size="
-//                + clusters.hashMap.size() + ", importanceMap.size=" + importanceMap.size());
+        int ims = importanceMap.size();
+        int ihms = countImportantClusters();
+
+//        System.out.format("[%s] Validating ClusterTable: byteSize=%d, hashMap=%d/%d, importanceMap=%d%n",
+//                place, sizeInBytes, ihms, clusters.hashMap.size(), ims);
 
         int i = clusterArray.length;
         long size = 0;
@@ -1196,19 +1223,43 @@ public final class ClusterTable implements IClusterTable {
                 System.out.println("BUG: CACHED CLUSTER SIZE DIFFERS FROM CALCULATED: " + sizeInBytes + " != " + size + ", delta = " + (sizeInBytes - size));
             //else System.out.println("\"BUG?\": SIZES DIFFER: " + sizeInBytes + " != " + size + ", delta = " + (sizeInBytes - size));
         }
+        if (ims != ihms) {
+            System.out.println("BUG2: hashmap and importanceMap sizes differ: " + ihms + " != " + ims + ", delta=" + (ihms - ims));
+            printMaps("validateSize");
+        }
+        //System.out.println("[" + place + "] VALIDATED");
+    }
 
-        int ims = importanceMap.size();
-        int[] hms = {0};
+    private void printMaps(String place) {
+        int ihms = countImportantClusters();
+        System.out.println("## printMaps(" + place + ") - " + importanceMap.size() + " - (" + ihms + "/" + clusters.hashMap.size() + ")");
+        System.out.println("importanceMap (" + importanceMap.size() + "):");
+        importanceMap.forEach((importance, ie) -> {
+            System.out.format("\t%d: %s%n", importance, ie.toString());
+        });
+        System.out.println("clusters.hashMap (" + ihms + "/" + clusters.hashMap.size() + "):");
         clusters.hashMap.forEachEntry((cid, c) -> {
-            if (c != null && !c.isWriteOnly() && !c.isEmpty() && c.isLoaded()) {
-                //System.out.println(cid + ": " + c);
-                hms[0]++;
-            }
+            boolean important = importantCluster(c);
+            boolean wo = c != null && c.isWriteOnly();
+            boolean empty = c != null && c.isEmpty();
+            boolean loaded = c != null && c.isLoaded();
+            System.out.format("\t%s: %d - %s (writeOnly=%b, empty=%b, loaded=%b)%n", important ? " I" : "NI", cid, c, wo, empty, loaded);
             return true;
         });
-        if (Math.abs(ims-hms[0]) > 0) {
-            System.out.println("BUG2: hashmap and importanceMap sizes differ: " + hms[0] + " != " + ims + ", delta=" + (hms[0] - ims));
-        }
+    }
+
+    private int countImportantClusters() {
+        int[] result = { 0 };
+        clusters.hashMap.forEachEntry((cid, c) -> {
+            if (importantCluster(c))
+                result[0]++;
+            return true;
+        });
+        return result[0];
+    }
+
+    private static boolean importantCluster(ClusterImpl c) {
+        return c != null && !c.isWriteOnly() && c.isLoaded();// && !c.isEmpty();
     }
 
 }
diff --git a/bundles/org.simantics.db.procore/src/org/simantics/db/procore/cluster/ClusterSmall.java b/bundles/org.simantics.db.procore/src/org/simantics/db/procore/cluster/ClusterSmall.java
index 1b7dc0d32..1a8af5143 100644
--- a/bundles/org.simantics.db.procore/src/org/simantics/db/procore/cluster/ClusterSmall.java
+++ b/bundles/org.simantics.db.procore/src/org/simantics/db/procore/cluster/ClusterSmall.java
@@ -1069,14 +1069,14 @@ final public class ClusterSmall extends ClusterImpl {
     public String toString() {
         if (deleted) return "ClusterSmall[" + getClusterId() + " - has been deleted or hasn't been created.]";
         try {
-            final TIntHashSet set = new TIntHashSet();
-            TIntShortHashMap map = foreignTable.getResourceHashMap();
-            map.forEachKey(new TIntProcedure() {
-                @Override
-                public boolean execute(int value) {
-                    set.add(value & 0xfffff000);
-                    return true;
-                }
+            ForeignTableSmall ft = foreignTable;
+            if (ft == null)
+                return "ClusterSmall[" + getClusterId() + " - " + getNumberOfResources() + "]";
+            TIntShortHashMap map = ft.getResourceHashMap();
+            TIntHashSet set = new TIntHashSet();
+            map.forEachKey(value -> {
+                set.add(value & 0xfffff000);
+                return true;
             });
             return "ClusterSmall[" + getClusterId() + " - " + getNumberOfResources() + " - " + foreignTable.getResourceHashMap().size() + " - " + set.size() + "]";
         } catch (Throwable e) {