[Issue #1146] reduce lock overhead in MainIndexFactory and SinglePointIndexFactory (#1149)

Now, getSinglePointIndex() and getMainIndex() are lock-free in most cases.

Author: bianhq

pixels-common/src/main/java/io/pixelsdb/pixels/common/index/MainIndexFactory.java

@@ -26,9 +26,9 @@
 import org.apache.logging.log4j.Logger;
 
 import java.io.IOException;
-import java.util.HashMap;
 import java.util.Map;
 import java.util.ServiceLoader;
+import java.util.concurrent.ConcurrentHashMap;
 
 import static java.util.Objects.requireNonNull;
 
@@ -40,7 +40,7 @@
 public class MainIndexFactory
 {
     private static final Logger logger = LogManager.getLogger(MainIndexFactory.class);
-    private final Map<Long, MainIndex> mainIndexImpls = new HashMap<>();
+    private final Map<Long, MainIndex> mainIndexImpls = new ConcurrentHashMap<>();
     private final MainIndex.Scheme enabledScheme;
     /**
      * The providers of the enabled main index schemes.
@@ -114,16 +114,22 @@ public boolean isSchemeEnabled(MainIndex.Scheme scheme)
      * @return the main index instance
      * @throws SinglePointIndexException
      */
-    public synchronized MainIndex getMainIndex(long tableId) throws MainIndexException
+    public MainIndex getMainIndex(long tableId) throws MainIndexException
     {
-        if (mainIndexImpls.containsKey(tableId))
+        MainIndex mainIndex = this.mainIndexImpls.get(tableId);
+        if (mainIndex == null)
         {
-            return mainIndexImpls.get(tableId);
+            synchronized (this)
+            {
+                // double check to avoid redundant creation of mainIndex
+                mainIndex = this.mainIndexImpls.get(tableId);
+                if (mainIndex == null)
+                {
+                    mainIndex = this.mainIndexProvider.createInstance(tableId, this.enabledScheme);
+                    this.mainIndexImpls.put(tableId, mainIndex);
+                }
+            }
         }
-
-        MainIndex mainIndex = this.mainIndexProvider.createInstance(tableId, enabledScheme);
-        mainIndexImpls.put(tableId, mainIndex);
-
         return mainIndex;
     }
 

pixels-common/src/main/java/io/pixelsdb/pixels/common/index/SinglePointIndexFactory.java

@@ -30,6 +30,10 @@
 
 import java.io.IOException;
 import java.util.*;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentSkipListSet;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static java.util.Objects.requireNonNull;
@@ -41,9 +45,10 @@
 public class SinglePointIndexFactory
 {
     private static final Logger logger = LogManager.getLogger(SinglePointIndexFactory.class);
-    private final Map<TableIndex, SinglePointIndex> singlePointIndexImpls = new HashMap<>();
-    private final Set<SinglePointIndex.Scheme> enabledSchemes = new TreeSet<>();
-    private final Map<Long, TableIndex> indexIdToTableIndex = new HashMap<>();
+    private final Map<TableIndex, SinglePointIndex> singlePointIndexImpls = new ConcurrentHashMap<>();
+    private final Set<SinglePointIndex.Scheme> enabledSchemes = new ConcurrentSkipListSet<>();
+    private final Map<Long, TableIndex> indexIdToTableIndex = new ConcurrentHashMap<>();
+    private final Lock lock = new ReentrantLock();
     /**
      * The providers of the enabled single point index schemes.
      */
@@ -128,27 +133,36 @@ public boolean isSchemeEnabled(SinglePointIndex.Scheme scheme)
      * @return the single point index instance
      * @throws SinglePointIndexException
      */
-    public synchronized SinglePointIndex getSinglePointIndex(long tableId, long indexId) throws SinglePointIndexException
+    public SinglePointIndex getSinglePointIndex(long tableId, long indexId) throws SinglePointIndexException
     {
-        TableIndex tableIndex = indexIdToTableIndex.get(indexId);
+        TableIndex tableIndex = this.indexIdToTableIndex.get(indexId);
         if (tableIndex == null)
         {
+            this.lock.lock();
             try
             {
-                io.pixelsdb.pixels.common.metadata.domain.SinglePointIndex singlePointIndex =
-                        MetadataService.Instance().getSinglePointIndex(indexId);
-                if (singlePointIndex == null)
+                // double check to avoid redundant tableIndex creation
+                tableIndex = this.indexIdToTableIndex.get(indexId);
+                if (tableIndex == null)
                 {
-                    throw new SinglePointIndexException("single point index with id " + indexId + " does not exist");
+                    io.pixelsdb.pixels.common.metadata.domain.SinglePointIndex spi =
+                            MetadataService.Instance().getSinglePointIndex(indexId);
+                    if (spi == null)
+                    {
+                        throw new SinglePointIndexException("single point index with id " + indexId + " does not exist");
+                    }
+                    tableIndex = new TableIndex(tableId, indexId, spi.getIndexScheme(), spi.isUnique());
+                    this.indexIdToTableIndex.put(indexId, tableIndex);
                 }
-                tableIndex = new TableIndex(tableId, indexId, singlePointIndex.getIndexScheme(), singlePointIndex.isUnique());
-                indexIdToTableIndex.put(indexId, tableIndex);
             } catch (MetadataException e)
             {
                 throw new SinglePointIndexException("failed to query single point index information from metadata", e);
             }
+            finally
+            {
+                this.lock.unlock();
+            }
         }
-        // 'synchronized' in Java is reentrant,  it is fine to call the other getSinglePointIndex() from here.
         return getSinglePointIndex(tableIndex);
     }
 
@@ -158,53 +172,68 @@ public synchronized SinglePointIndex getSinglePointIndex(long tableId, long inde
      * @return the single point index instance
      * @throws SinglePointIndexException
      */
-    public synchronized SinglePointIndex getSinglePointIndex(TableIndex tableIndex) throws SinglePointIndexException
+    public SinglePointIndex getSinglePointIndex(TableIndex tableIndex) throws SinglePointIndexException
     {
         requireNonNull(tableIndex, "tableIndex is null");
         checkArgument(this.enabledSchemes.contains(tableIndex.scheme), "single point index scheme '" +
                 tableIndex.scheme.toString() + "' is not enabled.");
 
-        if (!indexIdToTableIndex.containsKey(tableIndex.indexId))
-        {
-            indexIdToTableIndex.put(tableIndex.indexId, tableIndex);
-        }
+        this.indexIdToTableIndex.putIfAbsent(tableIndex.indexId, tableIndex);
 
-        if (singlePointIndexImpls.containsKey(tableIndex))
+        SinglePointIndex singlePointIndex = this.singlePointIndexImpls.get(tableIndex);
+        if (singlePointIndex == null)
         {
-            return singlePointIndexImpls.get(tableIndex);
+            this.lock.lock();
+            try
+            {
+                // double check to avoid redundant creation of singlePointIndex
+                singlePointIndex = this.singlePointIndexImpls.get(tableIndex);
+                if (singlePointIndex == null)
+                {
+                    singlePointIndex = this.singlePointIndexProviders.get(tableIndex.getScheme()).createInstance(
+                            tableIndex.tableId, tableIndex.indexId, tableIndex.scheme, tableIndex.isUnique());
+                    this.singlePointIndexImpls.put(tableIndex, singlePointIndex);
+                }
+            }
+            finally
+            {
+                this.lock.unlock();
+            }
         }
-
-        SinglePointIndex singlePointIndex = this.singlePointIndexProviders.get(tableIndex.getScheme())
-                .createInstance(tableIndex.tableId, tableIndex.indexId, tableIndex.scheme, tableIndex.isUnique());
-        singlePointIndexImpls.put(tableIndex, singlePointIndex);
-
         return singlePointIndex;
     }
 
     /**
      * Close all the opened single point index instances.
      * @throws IOException
      */
-    public synchronized void closeAll() throws SinglePointIndexException
+    public void closeAll() throws SinglePointIndexException
     {
-        for (TableIndex tableIndex : singlePointIndexImpls.keySet())
+        this.lock.lock();
+        try
         {
-            try
+            for (TableIndex tableIndex : this.singlePointIndexImpls.keySet())
             {
-                SinglePointIndex removing = singlePointIndexImpls.get(tableIndex);
-                if (removing != null)
+                try
                 {
-                    removing.close();
+                    SinglePointIndex removing = this.singlePointIndexImpls.get(tableIndex);
+                    if (removing != null)
+                    {
+                        removing.close();
+                    }
+                } catch (IOException e)
+                {
+                    throw new SinglePointIndexException(
+                            "failed to close single point index with id " + tableIndex.indexId, e);
                 }
             }
-            catch (IOException e)
-            {
-                throw new SinglePointIndexException(
-                        "failed to close single point index with id " + tableIndex.indexId, e);
-            }
+            this.singlePointIndexImpls.clear();
+            this.indexIdToTableIndex.clear();
+        }
+        finally
+        {
+            this.lock.unlock();
         }
-        singlePointIndexImpls.clear();
-        indexIdToTableIndex.clear();
     }
 
     /**
@@ -214,61 +243,63 @@ public synchronized void closeAll() throws SinglePointIndexException
      * @param closeAndRemove remove the index storage after closing if true
      * @throws SinglePointIndexException
      */
-    public synchronized void closeIndex(long tableId, long indexId, boolean closeAndRemove) throws SinglePointIndexException
+    public void closeIndex(long tableId, long indexId, boolean closeAndRemove) throws SinglePointIndexException
     {
-        TableIndex tableIndex = indexIdToTableIndex.remove(indexId);
-        if (tableIndex != null)
+        this.lock.lock();
+        try
         {
-            SinglePointIndex removed = singlePointIndexImpls.remove(tableIndex);
-            if (removed != null)
+            TableIndex tableIndex = this.indexIdToTableIndex.remove(indexId);
+            if (tableIndex != null)
             {
-                try
+                SinglePointIndex removed = this.singlePointIndexImpls.remove(tableIndex);
+                if (removed != null)
                 {
-                    if (closeAndRemove)
+                    try
                     {
-                        removed.closeAndRemove();
-                    }
-                    else
+                        if (closeAndRemove)
+                        {
+                            removed.closeAndRemove();
+                        } else
+                        {
+                            removed.close();
+                        }
+                    } catch (IOException e)
                     {
-                        removed.close();
+                        throw new SinglePointIndexException(
+                                "failed to close single point index with id " + tableIndex.indexId, e);
                     }
-                }
-                catch (IOException e)
+                } else
                 {
-                    throw new SinglePointIndexException(
-                            "failed to close single point index with id " + tableIndex.indexId, e);
+                    logger.warn("index with id {} once opened but not found", indexId);
                 }
-            }
-            else
-            {
-                logger.warn("index with id {} once opened but not found", indexId);
-            }
-        }
-        else
-        {
-            TableIndex tableIndex1 = new TableIndex(tableId, indexId, null, false);
-            SinglePointIndex removed = singlePointIndexImpls.remove(tableIndex1);
-            if (removed != null)
+            } else
             {
-                try
+                TableIndex tableIndex1 = new TableIndex(tableId, indexId, null, false);
+                SinglePointIndex removed = this.singlePointIndexImpls.remove(tableIndex1);
+                if (removed != null)
                 {
-                    if (closeAndRemove)
+                    try
                     {
-                        removed.closeAndRemove();
-                    }
-                    else
+                        if (closeAndRemove)
+                        {
+                            removed.closeAndRemove();
+                        } else
+                        {
+                            removed.close();
+                        }
+                    } catch (IOException e)
                     {
-                        removed.close();
+                        throw new SinglePointIndexException(
+                                "failed to close single point index with id " + tableIndex.indexId, e);
                     }
+                    logger.warn("index with id {} is found but not opened properly", indexId);
                 }
-                catch (IOException e)
-                {
-                    throw new SinglePointIndexException(
-                            "failed to close single point index with id " + tableIndex.indexId, e);
-                }
-                logger.warn("index with id {} is found but not opened properly", indexId);
             }
         }
+        finally
+        {
+            this.lock.unlock();
+        }
     }
 
     public static class TableIndex