diff --git a/platform/util/src/com/intellij/util/indexing/impl/ChangeTrackingValueContainer.java b/platform/util/src/com/intellij/util/indexing/impl/ChangeTrackingValueContainer.java index 7202c4f61705..e6edb03afc8b 100644 --- a/platform/util/src/com/intellij/util/indexing/impl/ChangeTrackingValueContainer.java +++ b/platform/util/src/com/intellij/util/indexing/impl/ChangeTrackingValueContainer.java @@ -7,12 +7,13 @@ import com.intellij.util.indexing.ValueContainer; import com.intellij.util.io.DataExternalizer; import com.intellij.util.io.DataInputOutputUtil; import it.unimi.dsi.fastutil.ints.IntOpenHashSet; -import it.unimi.dsi.fastutil.ints.IntSet; import org.jetbrains.annotations.ApiStatus.Internal; import org.jetbrains.annotations.NotNull; import java.io.DataOutput; import java.io.IOException; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.IntConsumer; /** * Container balances between keeping the changes as changes, and merging (applying) them. @@ -24,9 +25,67 @@ import java.io.IOException; */ @Internal public class ChangeTrackingValueContainer extends UpdatableValueContainer { + + protected static class ThreadSafeIntSet { + private final IntOpenHashSet delegate = new IntOpenHashSet(1); + private final ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock(); + + private T withWriteLock(Computable block) { + rwLock.writeLock().lock(); + try { + return block.compute(); + } + finally { + rwLock.writeLock().unlock(); + } + } + + private T withReadLock(Computable block) { + rwLock.readLock().lock(); + try { + return block.compute(); + } + finally { + rwLock.readLock().unlock(); + } + } + + boolean add(int id) { + return withWriteLock(() -> delegate.add(id)); + } + + void forEach(IntConsumer intConsumer) { + withReadLock(() -> { + delegate.forEach(intConsumer); + return null; + }); + } + + boolean isEmpty() { + return withReadLock(() -> delegate.isEmpty()); + } + + int[] toIntArray() { + return withReadLock(() -> delegate.toIntArray()); + } + + public void remove(int id) { + withWriteLock(() -> { + delegate.remove(id); + return null; + }); + } + } + // there is no volatile as we modify under write lock and read under read lock + // TODO RC: myInvalidated can be updated under read lock (through FileBasedIndexImpl.ensureUpToDate), + // which technically means that we modify set under read lock. + // We have a lot of reports that IDE freezes in IntOpenHashSet.add, and it seems that we indeed never exit that method. + // Inside IntOpenHashSet.add there is a while loop. If it never exits, then we either have a problem with internal + // structure consistency, or we have a bug in IntOpenHashSet implementation. Former is more realistic, let's check + // if this is true by making myInvalidated thread-safe. protected ValueContainerImpl myAdded; - protected IntSet myInvalidated; + protected ThreadSafeIntSet myInvalidated; //TODO RC: volatile field(s) here seems suspicious/ambiguous to me. @@ -73,7 +132,7 @@ public class ChangeTrackingValueContainer extends UpdatableValueContainer boolean wasRemovedFromAdded = removeFromAdded(inputId); //RC: It is teasing to short-circuit here if wasRemovedFromAdded=true -- seems like no need to add inputId to invalidatedIds? // Wrong: inputId could be contained in the (not yet loaded) -- inputId still needs to be in invalidatedIds then. - // I.e consider scenario: + // I.e. consider scenario: // 1) container X created: { merged=null, added=[], invalidated=[] } // underlying container (to-be-mergedSnapshot, not yet loaded) = [..., (inputId, value), ... ] // 2) X.addValue(inputId, value) => X{ merged=null, added=[(inputId, value)], invalidated=[] } @@ -88,7 +147,7 @@ public class ChangeTrackingValueContainer extends UpdatableValueContainer private boolean addToRemoved(int inputId) { if (myInvalidated == null) { - myInvalidated = new IntOpenHashSet(1); + myInvalidated = new ThreadSafeIntSet(); } return myInvalidated.add(inputId); } @@ -196,9 +255,9 @@ public class ChangeTrackingValueContainer extends UpdatableValueContainer public void saveDiffTo(@NotNull DataOutput out, @NotNull DataExternalizer externalizer) throws IOException { - IntSet set = myInvalidated; + ThreadSafeIntSet set = myInvalidated; if (set != null && !set.isEmpty()) { - for (int inputId : myInvalidated.toIntArray()) { + for (int inputId : set.toIntArray()) { DataInputOutputUtil.writeINT(out, -inputId); // mark inputId as invalid, to be processed on load in ValueContainerImpl.readFrom } }