Merged PR 722049: Revert file content table to use ConcurrentBigMap

- Revert the use of `ConcurrentDictionary` back to `ConcurrentBigMap` because based on recent benchmarking, `ConcurrentBigMap`, with the typical load of file content table, has better memory characteristics.
- Avoid unnecessary existing check when "cloning" concurrent big map. This gives 1.47x speed up.

|  Method   |     Mean    |    Error    |   StdDev   |      Gen0     |      Gen1    | Allocated |
|------------|-------------|-----------|------------|-------------|-------------|------------|
| CopyOld  | 843.5 ms    |  9.61 ms  |  8.98 ms   | 8000.0000  | 5000.0000 |  48.05 MB |
| CopyNew | 573.4 ms    | 11.40 ms | 11.19 ms | 8000.0000   | 5000.0000 |  48.05 MB |

Related work items: #2072119

Author: narasamdya

Public/Src/Utilities/Native/IO/FileIdAndVolumeId.cs

@@ -24,11 +24,6 @@ namespace BuildXL.Native.IO
     {
         internal static readonly int Size = Marshal.SizeOf<FileIdAndVolumeId>();
 
-        /// <summary>
-        /// Comparer instance.
-        /// </summary>
-        public static readonly Comparer ComparerInstance = new();
-
         /// <summary>
         /// Volume containing the file.
         /// </summary>
@@ -92,17 +87,5 @@ public static FileIdAndVolumeId Deserialize(BinaryReader reader)
         {
             return new FileIdAndVolumeId(reader.ReadUInt64(), FileId.Deserialize(reader));
         }
-
-        /// <summary>
-        /// Default comparer.
-        /// </summary>
-        public class Comparer : IEqualityComparer<FileIdAndVolumeId>
-        {
-            /// <inheritdoc/>
-            public bool Equals(FileIdAndVolumeId x, FileIdAndVolumeId y) => x.Equals(y);
-
-            /// <inheritdoc/>
-            public int GetHashCode([DisallowNull] FileIdAndVolumeId obj) => obj.GetHashCode();
-        }
     }
 }

Public/Src/Utilities/Storage/FileContentTable.cs

@@ -61,7 +61,7 @@ namespace BuildXL.Storage
     /// </remarks>
     public sealed class FileContentTable : IFileChangeTrackingObserver
     {
-        private static readonly FileEnvelope s_fileEnvelope = new(name: "FileContentTable." + ContentHashingUtilities.HashInfo.Name, version: 20);
+        private static readonly FileEnvelope s_fileEnvelope = new(name: "FileContentTable." + ContentHashingUtilities.HashInfo.Name, version: 21);
 
         /// <summary>
         /// Default time-to-live (TTL) for new entries to a <see cref="FileContentTable"/>.
@@ -72,7 +72,7 @@ public sealed class FileContentTable : IFileChangeTrackingObserver
         /// <summary>
         /// These are the (global file ID) -> (USN, hash) mappings recorded or retrieved in this session.
         /// </summary>
-        private readonly ConcurrentDictionary<FileIdAndVolumeId, Entry> m_entries = new(1024, 1024);
+        private readonly ConcurrentBigMap<FileIdAndVolumeId, Entry> m_entries = new();
 
         /// <summary>
         /// In the event a volume has a disabled change journal or not all files have USNs (journal enabled after last write to a
@@ -148,38 +148,12 @@ private FileContentTable(FileContentTable other, LoggingContext loggingContext,
             EntryTimeToLive = entryTimeToLive;
 
             // Copy entries, discarding the stale ones and decrementing TTLs
-            //
-            // Previously, the m_entries' implementation uses ConcurrentBigMap, and it calls ConcurrentBigMap's ConvertUnsafe to cheaply
-            // copy the entries:
-            //
-            //   m_entries = other.m_entries.ConvertUnsafe(e => e.TimeToLive > 0
-            //       ? e.WithTimeToLive((ushort)Math.Min(entryTimeToLive, e.TimeToLive - 1))
-            //       : new Entry(e.Usn, default /* Invalid hash as a marker */, e.Length, e.TimeToLive));
-            //   m_entries
-            //       .Where(kvp => !kvp.Value.Hash.IsValid)
-            //       .Select(kvp => kvp.Key)
-            //       .ToList()
-            //       .ForEach(k => m_entries.RemoveKey(k));
-            //
-            // However, the enumeration implementation of ConcurrentBigMap seems to be buggy and causes
-            // the following exception:
-            // ---> System.IndexOutOfRangeException: Index was outside the bounds of the array.
-            //      at IEnumerator<TItem> BuildXL.Utilities.Collections.ConcurrentBigSet<TItem>+ItemsCollection.GetEnumerator() + MoveNext()
-            //      at List<TResult> System.Linq.Enumerable+WhereSelectEnumerableIterator<TSource, TResult>.ToList()
-            //      at List<TSource> System.Linq.Enumerable.ToList<TSource>(IEnumerable<TSource> source)
-            //      at new BuildXL.Storage.FileContentTable(FileContentTable other, LoggingContext loggingContext, ushort entryTimeToLive) in \.\Public\Src\Utilities\Storage\FileContentTable.cs:line 156
-            m_entries = new ConcurrentDictionary<FileIdAndVolumeId, Entry>(
-                1024,
-                other.m_entries
-                    .Where(kvp => kvp.Value.TimeToLive > 0)
-                    .Select(kvp => new KeyValuePair<FileIdAndVolumeId, Entry>(
-                        kvp.Key,
-                        kvp.Value.WithTimeToLive((ushort)Math.Min(entryTimeToLive, kvp.Value.TimeToLive - 1)))),
-#if NET_FRAMEWORK
-                FileIdAndVolumeId.ComparerInstance); // ConcurrentDictionary does not support null comparer on .NET Framework
-#else
-                null);
-#endif
+            m_entries = ConcurrentBigMap<FileIdAndVolumeId, Entry>.Create(items: other.m_entries
+                .Where(kvp => kvp.Value.TimeToLive > 0)
+                .Select(kvp => new KeyValuePair<FileIdAndVolumeId, Entry>(
+                    kvp.Key,
+                    kvp.Value.WithTimeToLive((ushort)Math.Min(entryTimeToLive, kvp.Value.TimeToLive - 1)))),
+                checkExistingItem: false);
         }
 
         /// <summary>
@@ -475,11 +449,12 @@ public VersionedFileIdentity RecordContentHash(
                 // So, concurrently creating and recording (or even just recording) different links is possible, and
                 // keeping the last stored entry (rather than highest-USN entry) can introduce false positives.
                 var fileIdAndVolumeId = new FileIdAndVolumeId(strongIdentity.Value.VolumeSerialNumber, strongIdentity.Value.FileId);
-                
+
                 m_entries.AddOrUpdate(
                     fileIdAndVolumeId,
+                    (@this: this, path, newEntry),
                     addValueFactory: static (key, state) => state.newEntry,
-                    updateValueFactory: static (key, existingEntry, state) =>
+                    updateValueFactory: static (key, state, existingEntry) =>
                     {
                         FileContentTable @this = state.@this;
                         Entry newEntry = state.newEntry;
@@ -510,8 +485,7 @@ public VersionedFileIdentity RecordContentHash(
                         }
 
                         return newEntry;
-                    },
-                    factoryArgument: (@this: this, path, newEntry));
+                    });
 
                 if (ETWLogger.Log.IsEnabled(BuildXL.Tracing.Diagnostics.EventLevel.Verbose, Keywords.Diagnostics))
                 {

Public/Src/Utilities/UnitTests/Collections/ConcurrentBigMapTests.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System;
+using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 using System.Threading;
@@ -166,6 +167,45 @@ private static void TestOperationsHelper(bool parallel)
                 }, parallel);
         }
 
+        [Fact]
+        public void TestCopy()
+        {
+            const int Length = 10_000;
+            var map = ConcurrentBigMap<int, int>.Create(items: Enumerable.Range(0, Length).Select(i => new KeyValuePair<int, int>(i, i)), checkExistingItem: true);
+            var copy = ConcurrentBigMap<int, int>.Create(items: map, checkExistingItem: false);
+            Enumerable.Range(0, Length).ToList().ForEach(i =>
+            {
+                XAssert.AreEqual(map.Count, copy.Count);
+                XAssert.AreEqual(map.TryGetValue(i, out var mapValue), copy.TryGetValue(i, out var copyValue));
+                XAssert.AreEqual(mapValue, copyValue);
+            });
+        }
+
+        [Fact]
+        public void TestRemoveWhileEnumerateSingleThread()
+        {
+            const int Length = 10_000;
+            var map = ConcurrentBigMap<int, int>.Create(items: Enumerable.Range(0, Length).Select(i => new KeyValuePair<int, int>(i, i)), checkExistingItem: true);
+            var copy = ConcurrentBigMap<int, int>.Create(items: map, checkExistingItem: false);
+            
+            map.TryRemove(77, out _);
+            copy.TryRemove(77, out _);
+            copy.TryAdd(77, 12345);
+
+            foreach (var kvp in copy.Where(kvp => kvp.Key % 2 == 1))
+            {
+                copy.TryRemove(kvp.Key, out _);
+            }
+
+            Enumerable.Range(0, Length).ToList().ForEach(i =>
+            {
+                if (i % 2 == 1)
+                {
+                    XAssert.IsFalse(copy.ContainsKey(i));
+                }
+            });
+        }
+
         private static void For(int count, Action<int> action, bool parallel)
         {
             if (parallel)

Public/Src/Utilities/Utilities.Core/Collections/ConcurrentBigMap.cs

@@ -76,17 +76,20 @@ public ConcurrentBigMap(
         /// <param name="items">an enumeration of unique (!)  items to insert</param>
         /// <param name="keyComparer">the comparer for keys</param>
         /// <param name="valueComparer">the comparer for values used in compare exchange operations</param>
+        /// <param name="checkExistingItem">if true, checks if the item already exists in the backing set</param>
         public static ConcurrentBigMap<TKey, TValue> Create(
             int concurrencyLevel = ConcurrentBigSet<KeyValuePair<TKey, TValue>>.DefaultConcurrencyLevel,
             int capacity = ConcurrentBigSet<KeyValuePair<TKey, TValue>>.DefaultCapacity,
             int ratio = ConcurrentBigSet<KeyValuePair<TKey, TValue>>.DefaultBucketToItemsRatio,
             IEnumerable<KeyValuePair<TKey, TValue>> items = null,
-            IEqualityComparer<TKey> keyComparer = null, IEqualityComparer<TValue> valueComparer = null)
+            IEqualityComparer<TKey> keyComparer = null,
+            IEqualityComparer<TValue> valueComparer = null,
+            bool checkExistingItem = true)
         {
             var result = new ConcurrentBigMap<TKey, TValue>(concurrencyLevel: concurrencyLevel, capacity: capacity, ratio: ratio, keyComparer: keyComparer, valueComparer: valueComparer);
             if (items != null)
             {
-                result.BackingSet.UnsafeAddItems(items.Select(item => result.CreateKeyValuePendingItem(item.Key, item.Value)));
+                result.BackingSet.UnsafeAddItems(items.Select(item => result.CreateKeyValuePendingItem(item.Key, item.Value)), checkExistingItem: checkExistingItem);
             }
 
             return result;

Public/Src/Utilities/Utilities.Core/Collections/ConcurrentBigSet.cs

@@ -538,7 +538,7 @@ public GetAddOrUpdateResult Remove(TItem item, IEqualityComparer<TItem> comparer
         /// <summary>
         /// This method assumes no concurrent accesses, and that the new item isn't already in the list
         /// </summary>
-        internal void UnsafeAddItems<TPendingItem>(IEnumerable<TPendingItem> pendingItems)
+        internal void UnsafeAddItems<TPendingItem>(IEnumerable<TPendingItem> pendingItems, bool checkExistingItem)
             where TPendingItem : IPendingSetItem<TItem>
         {
             Accessors accessors = m_accessors;
@@ -548,19 +548,30 @@ internal void UnsafeAddItems<TPendingItem>(IEnumerable<TPendingItem> pendingItem
 
                 int lockNo = 0;
 
+                int bucketNo;
+                int headNodeIndex;
+
                 // Number of nodes searched to find the item or the total number of nodes if the item is not found
-                var result = FindItem(
-                    pendingItem,
-                    bucketHashCode,
-                    out int bucketNo,
-                    out int headNodeIndex,
-                    ref accessors,
-                    out int findCount,
-                    out int priorNodeIndex);
-                Contract.Assert(!result.IsFound);
+                if (checkExistingItem)
+                {
+                    var result = FindItem(
+                        pendingItem,
+                        bucketHashCode,
+                        out bucketNo,
+                        out headNodeIndex,
+                        ref accessors,
+                        out int findCount,
+                        out int priorNodeIndex);
+                    Contract.Assert(!result.IsFound);
+                }
+                else
+                {
+                    bucketNo = m_buckets.GetBucketNo(bucketHashCode);
+                    headNodeIndex = m_buckets[bucketNo];
+                }
 
                 // now make an item from the lookup value
-                TItem item = pendingItem.CreateOrUpdateItem(result.Item, false, out bool remove);
+                TItem item = pendingItem.CreateOrUpdateItem(default, false, out bool remove);
                 Contract.Assert(!remove, "Remove is only allowed when performing update operation");
                 SetBucketHeadNode(bucketNo, lockNo, bucketHashCode, headNodeIndex, item, ref accessors);
                 int countAfterAdd = Interlocked.Increment(ref m_count);