[vfs][refactoring] merge per-record and hierarchy locks in FSRecordsImpl into one lock

GitOrigin-RevId: 64b96629b6c8058191d74d0c049cec675788f39b

platform/platform-impl/src/com/intellij/openapi/vfs/newvfs/persistent/FSRecordsImpl.java

@@ -336,7 +336,6 @@ public final class FSRecordsImpl implements Closeable {
 
   /** Lock to protect individual file-records updates */
   private final FileRecordLock fileRecordLock = new FileRecordLock();
-  private final PerFileIdLock fileHierarchyLock = new PerFileIdLock();
 
   //TODO RC: why to have it both here, and also in PersistentFSConnection? Mb one place is enough?
   private volatile boolean closed = false;
@@ -720,7 +719,7 @@ public final class FSRecordsImpl implements Closeable {
 
     checkNotClosed();
 
-    fileHierarchyLock.lock(parentId);
+    fileRecordLock.lockForHierarchyUpdate(parentId);
     try {
       ListResult children = list(parentId);
       ListResult modifiedChildren = childrenConvertor.apply(children);
@@ -743,7 +742,7 @@ public final class FSRecordsImpl implements Closeable {
       throw handleError(e);
     }
     finally {
-      fileHierarchyLock.unlock(parentId);
+      fileRecordLock.unlockForHierarchyUpdate(parentId);
     }
   }
 
@@ -760,9 +759,9 @@ public final class FSRecordsImpl implements Closeable {
 
     int minId = Math.min(fromParentId, toParentId);
     int maxId = Math.max(fromParentId, toParentId);
-    fileHierarchyLock.lock(minId);
+    fileRecordLock.lockForHierarchyUpdate(minId);
     try {
-      fileHierarchyLock.lock(maxId);
+      fileRecordLock.lockForHierarchyUpdate(maxId);
       try {
         try {
           ListResult childrenToMove = list(fromParentId);
@@ -789,11 +788,11 @@ public final class FSRecordsImpl implements Closeable {
         }
       }
       finally {
-        fileHierarchyLock.unlock(maxId);
+        fileRecordLock.unlockForHierarchyUpdate(maxId);
       }
     }
     finally {
-      fileHierarchyLock.unlock(minId);
+      fileRecordLock.unlockForHierarchyUpdate(minId);
     }
   }
 
@@ -817,9 +816,9 @@ public final class FSRecordsImpl implements Closeable {
 
     int minId = Math.min(fromParentId, toParentId);
     int maxId = Math.max(fromParentId, toParentId);
-    fileHierarchyLock.lock(minId);
+    fileRecordLock.lockForHierarchyUpdate(minId);
     try {
-      fileHierarchyLock.lock(maxId);
+      fileRecordLock.lockForHierarchyUpdate(maxId);
       try {
         try {
           ListResult firstParentChildren = list(fromParentId);
@@ -872,11 +871,11 @@ public final class FSRecordsImpl implements Closeable {
         }
       }
       finally {
-        fileHierarchyLock.unlock(maxId);
+        fileRecordLock.unlockForHierarchyUpdate(maxId);
       }
     }
     finally {
-      fileHierarchyLock.unlock(minId);
+      fileRecordLock.unlockForHierarchyUpdate(minId);
     }
   }
 

platform/platform-impl/src/com/intellij/openapi/vfs/newvfs/persistent/FileRecordLock.java

@@ -1,43 +1,124 @@
 // Copyright 2000-2024 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
 package com.intellij.openapi.vfs.newvfs.persistent;
 
-import org.jetbrains.annotations.ApiStatus;
+import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
+import it.unimi.dsi.fastutil.ints.IntSet;
 
+import java.util.concurrent.locks.LockSupport;
 import java.util.concurrent.locks.StampedLock;
 
+/**
+ * Lock used to protect file-record accesses in {@link FSRecordsImpl}
+ * <p>
+ * Basically, it is a segmented read-write lock ({@link StampedLock}), with an additional 'lock for hierarchy update'
+ * locking mode ({@link #lockForHierarchyUpdate(int)}).
+ * This is not a generally applicable lock by any means: it is very much tailored for the specific needs of {@link FSRecordsImpl}.
+ * <p>
+ * The lock is <b>NOT reentrant</b> (because {@link StampedLock} is not reentrant), and an attempt to lock already locked fileId
+ * down the stack <b>leads to deadlock</b> -- so one needs to be quite careful to use this lock.
+ * 'Lock for hierarchy update' mode is also not reentrant.
+ *
+ * @see StampedLock
+ */
 class FileRecordLock {
   private static final int SEGMENTS_COUNT = 16;
   private static final int SEGMENTS_MASK = 0b1111;
 
-  private final StampedLock[] segmentedLock = new StampedLock[SEGMENTS_COUNT];
+  private final Segment[] segments = new Segment[SEGMENTS_COUNT];
 
   {
     for (int i = 0; i < SEGMENTS_COUNT; i++) {
-      segmentedLock[i] = new StampedLock();
+      segments[i] = new Segment();
     }
   }
 
   public long lockForWrite(int fileId) {
-    StampedLock lock = lockFor(fileId);
+    StampedLock lock = segmentFor(fileId);
     return lock.writeLock();
   }
 
-  public void unlockForWrite(int fileId, long stamp) {
-    StampedLock lock = lockFor(fileId);
+  public void unlockForWrite(int fileId,
+                             long stamp) {
+    StampedLock lock = segmentFor(fileId);
     lock.unlockWrite(stamp);
   }
 
   public long lockForRead(int fileId) {
-    StampedLock lock = lockFor(fileId);
+    StampedLock lock = segmentFor(fileId);
     return lock.readLock();
   }
 
   public void unlockForRead(int fileId, long stamp) {
-    StampedLock lock = lockFor(fileId);
+    StampedLock lock = segmentFor(fileId);
     lock.unlockRead(stamp);
   }
 
-  public StampedLock lockFor(int fileId){
-    return segmentedLock[fileId & SEGMENTS_MASK];
+  public StampedLock lockFor(int fileId) {
+    return segmentFor(fileId);
+  }
+
+  /**
+   * Locks fileId for "hierarchy update": any attempt to lock same fileId for hierarchy update will be blocked until fileId is
+   * released with {@link #unlockForHierarchyUpdate(int)} call.
+   * <p>
+   * 'Hierarchy update' locking mode is independent of regular read/write locking: i.e. fileId locked for hierarchy update is not
+   * locked for read or write, and could be locked for read/write independently.
+   * <p>
+   * Hierarchy update lock is NOT reentrant: an attempt to lock the same fileId for hierarchy update down the stack in the same
+   * thread lead to deadlock.
+   */
+  public void lockForHierarchyUpdate(int fileId) {
+    segmentFor(fileId).lockHierarchy(fileId);
+  }
+
+  public void unlockForHierarchyUpdate(int fileId) {
+    segmentFor(fileId).unlockHierarchy(fileId);
+  }
+
+
+  private Segment segmentFor(int fileId) {
+    return segments[fileId & SEGMENTS_MASK];
+  }
+
+  private static class Segment extends StampedLock {
+
+    /** Set of fileId for which hierarchy updates are now ongoing, so those id are 'locked' for hierarchy updates now */
+    private final IntSet hierarchyUpdatesInProcess = new IntOpenHashSet();
+
+    public void lockHierarchy(int id) {
+      for (int turn = 0; ; turn++) {
+        long lockStamp = writeLock();
+        try {
+          if (!hierarchyUpdatesInProcess.contains(id)) {
+            hierarchyUpdatesInProcess.add(id);
+            return;
+          }
+
+          //use active spinning, since stamped lock doesn't support Condition to await()/signal() on:
+          if (turn < 64) {
+            Thread.onSpinWait();
+          }
+          else {
+            LockSupport.parkNanos(1000);
+          }
+        }
+        finally {
+          unlockWrite(lockStamp);
+        }
+      }
+    }
+
+    public void unlockHierarchy(int id) {
+      long lockStamp = writeLock();
+      try {
+        boolean actuallyRemoved = hierarchyUpdatesInProcess.remove(id);
+        if (!actuallyRemoved) {
+          throw new IllegalStateException("Trying to unlock(" + id + ") which is not currently locked " + hierarchyUpdatesInProcess);
+        }
+      }
+      finally {
+        unlockWrite(lockStamp);
+      }
+    }
   }
 }

platform/platform-impl/src/com/intellij/openapi/vfs/newvfs/persistent/PerFileIdLock.java

@@ -1,92 +0,0 @@
-// Copyright 2000-2024 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
-package com.intellij.openapi.vfs.newvfs.persistent;
-
-import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
-import it.unimi.dsi.fastutil.ints.IntSet;
-
-import java.util.concurrent.locks.Condition;
-import java.util.concurrent.locks.ReentrantLock;
-
-/**
- * Lock for protecting access to file records (identified by integer fileId)
- * Attempt to lock ID that is already locked made the current thread waiting until that ID is unlocked.
- * <p>
- * This lock is used in VFS to protect hierarchy updates -- i.e. add/remove/update children
- * <p>
- * Lock is NOT re-entrant: an attempt to lock same fileId in the thread that already locked the fileId -- leads to deadlock.
- */
-final class PerFileIdLock {
-
-  //TODO RC: Currently we use PerFileIdLock for updating file hierarchy -- which is relatively long process, since it
-  //         involves requests to underlying FS, IO, and children modification. Which is why 'ReentrantLock with fileId
-  //         list per segment' approach was used -- it allows to keep particular fileId locked for some time, without
-  //         locking other fileIds, even those falling into the same segment. The downside is that it is relatively expensive,
-  //         and not reentrant -- both limiting its applicability as general file-record locking method.
-
-  //Ideally, each fileId should have its own lock, but this is too expensive, so we use segmented lock
-
-  private final SegmentLock[] segments;
-
-  PerFileIdLock() {
-    this(16);
-  }
-
-  PerFileIdLock(int segmentsCount) {
-    segments = new SegmentLock[segmentsCount];
-    for (int i = 0; i < segments.length; i++) {
-      segments[i] = new SegmentLock();
-    }
-  }
-
-  public void lock(int id) {
-    int index = toIndex(id);
-    segments[index].lock(id);
-  }
-
-  public void unlock(int id) {
-    int index = toIndex(id);
-    segments[index].unlock(id);
-  }
-
-  private int toIndex(int id) {
-    return id % segments.length;
-  }
-
-  private static class SegmentLock {
-    private final ReentrantLock lock = new ReentrantLock();
-    private final Condition unlockCondition = lock.newCondition();
-    private final IntSet lockedIds = new IntOpenHashSet();
-
-    public void lock(int id) {
-      lock.lock();
-      try {
-        while (lockedIds.contains(id)) {
-          unlockCondition.awaitUninterruptibly();
-        }
-        lockedIds.add(id);
-      }
-      finally {
-        lock.unlock();
-      }
-    }
-
-    public void unlock(int id) {
-      lock.lock();
-      try {
-        boolean actuallyRemoved = lockedIds.remove(id);
-        if (actuallyRemoved) {
-          //This wakes up all threads waiting -- i.e. lockedIds.size() -- we assume it is usually just a few of them.
-          // But there could be pathological scenarios there a lot of threads waiting: and each thread will need to
-          // re-acquire lock, check lockedIds.contains() -- and all threads but one return to waiting after the check.
-          unlockCondition.signalAll();
-        }
-        else {
-          throw new IllegalStateException("Trying to unlock(" + id + ") which is not currently locked " + lockedIds);
-        }
-      }
-      finally {
-        lock.unlock();
-      }
-    }
-  }
-}

platform/platform-tests/testSrc/com/intellij/openapi/vfs/newvfs/persistent/PersistentFsTest.java

@@ -3,7 +3,6 @@ package com.intellij.openapi.vfs.newvfs.persistent;
 
 import com.intellij.CacheSwitcher;
 import com.intellij.ide.plugins.DynamicPluginsTestUtil;
-import com.intellij.idea.IJIgnore;
 import com.intellij.openapi.Disposable;
 import com.intellij.openapi.application.Application;
 import com.intellij.openapi.application.ApplicationManager;
@@ -35,7 +34,6 @@ import com.intellij.testFramework.*;
 import com.intellij.testFramework.fixtures.BareTestFixtureTestCase;
 import com.intellij.testFramework.rules.TempDirectory;
 import com.intellij.testFramework.utils.vfs.CheckVFSHealthRule;
-import com.intellij.testFramework.utils.vfs.SkipVFSHealthCheck;
 import com.intellij.util.ArrayUtil;
 import com.intellij.util.PathUtil;
 import com.intellij.util.containers.ContainerUtil;
@@ -667,7 +665,6 @@ public class PersistentFsTest extends BareTestFixtureTestCase {
   }
 
   @Test
-  @SkipVFSHealthCheck
   public void testConcurrentListAllDoesntCauseDuplicateFileIds() throws Exception {
     PersistentFSImpl pfs = (PersistentFSImpl)PersistentFS.getInstance();
     Application application = ApplicationManager.getApplication();
@@ -980,7 +977,7 @@ public class PersistentFsTest extends BareTestFixtureTestCase {
     events.clear();
   }
 
-  @IJIgnore(issue = "IJPL-149673")
+  //@IJIgnore(issue = "IJPL-149673")
   @Test
   public void testChildMove() throws IOException {
     final File firstDirIoFile = tempDirectory.newDirectory("dir1");