Merge branch 'main-master' into memory-mapped-hit-counts

# Conflicts:
#	src/coverlet.template/ModuleTrackerTemplate.cs

Author: petli

.gitattributes

@@ -0,0 +1 @@
+* text=auto

README.md

@@ -434,12 +434,39 @@ These steps must be followed before you attempt to open the solution in an IDE (
 
 ### Performance testing
 
-There is a performance test for the hit counting instrumentation in the test project `coverlet.core.performancetest`.  Build the project with the msbuild step above and then run:
+There is a simple performance test for the hit counting instrumentation in the test project `coverlet.core.performancetest`.  Build the project with the msbuild step above and then run:
 
     dotnet test /p:CollectCoverage=true test/coverlet.core.performancetest/
 
 The duration of the test can be tweaked by changing the number of iterations in the `[InlineData]` in the `PerformanceTest` class.
 
+For more realistic testing it is recommended to try out any changes to the hit counting code paths on large, realistic projects.  If you don't have any handy https://github.com/dotnet/corefx is an excellent candidate.  [This page](https://github.com/dotnet/corefx/blob/master/Documentation/building/code-coverage.md) describes how to run code coverage tests for both the full solution and for individual projects with coverlet from nuget.  Suitable projects (listed in order of escalating test durations):
+
+* System.Collections.Concurrent.Tests
+* System.Collections.Tests
+* System.Reflection.Metadata.Tests
+* System.Xml.Linq.Events.Tests
+* System.Runtime.Serialization.Formatters.Tests
+
+Change to the directory of the library and run the msbuild code coverage command:
+
+    dotnet msbuild /t:BuildAndTest /p:Coverage=true
+
+Look for a line like this:
+
+    ----- start 18:13:36,59 ===============  To repro directly: =====================================================
+
+It is followed by a `pushd` command into the artefact directory, and a command to run coverlet.  Run these to get to see the coverlet output and especially the test duration. E.g.:
+
+    C:\...\corefx\artifacts\tools\coverlet "System.Collections.Concurrent.Tests.dll" --target ...
+
+To run with a development version of coverlet call `dotnet run` instead of the installed coverlet version, e.g.:
+
+    dotnet run -p C:\...\coverlet\src\coverlet.console\coverlet.console.csproj -c Release -- "System.Collections.Concurrent.Tests.dll" --target ...
+
+
+
+
 
 ## Code of Conduct
 

src/coverlet.core/Instrumentation/Instrumenter.cs

@@ -25,7 +25,7 @@ internal class Instrumenter
         private readonly string[] _excludedAttributes;
         private InstrumenterResult _result;
         private FieldDefinition _customTrackerHitsFilePath;
-        private FieldDefinition _customTrackerHitsArraySize;
+        private FieldDefinition _customTrackerHitsArray;
         private FieldDefinition _customTrackerHitsMemoryMapName;
         private ILProcessor _customTrackerClassConstructorIl;
         private TypeDefinition _customTrackerTypeDef;
@@ -101,7 +101,8 @@ private void InstrumentModule()
                     _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Ldstr, _result.HitsFilePath));
                     _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsFilePath));
                     _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Ldc_I4, _result.HitCandidates.Count));
-                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsArraySize));
+                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Newarr, module.TypeSystem.Int32));
+                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsArray));
                     _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Ldstr, _result.HitsResultGuid));
                     _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsMemoryMapName));
 
@@ -132,8 +133,8 @@ private void AddCustomModuleTrackerToModule(ModuleDefinition module)
                         _customTrackerHitsFilePath = fieldClone;
                     else if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsMemoryMapName))
                         _customTrackerHitsMemoryMapName = fieldClone;
-                    else if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsArraySize))
-                        _customTrackerHitsArraySize = fieldClone;
+                    else if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsArray))
+                        _customTrackerHitsArray = fieldClone;
                 }
 
                 foreach (MethodDefinition methodDef in moduleTrackerTemplate.Methods)

src/coverlet.core/Symbols/CecilSymbolHelper.cs

@@ -20,9 +20,9 @@ public static class CecilSymbolHelper
         private const int StepOverLineCode = 0xFEEFEE;
         private static readonly Regex IsMovenext = new Regex(@"\<[^\s>]+\>\w__\w(\w)?::MoveNext\(\)$", RegexOptions.Compiled | RegexOptions.ExplicitCapture);
 
-        public static bool IsMoveNext(string fullName)
-        {
-            return IsMovenext.IsMatch(fullName);
+        public static bool IsMoveNext(string fullName)
+        {
+            return IsMovenext.IsMatch(fullName);
         }
 
         public static List<BranchPoint> GetBranchPoints(MethodDefinition methodDefinition)

src/coverlet.template/ModuleTrackerTemplate.cs

@@ -24,67 +24,35 @@ public static class ModuleTrackerTemplate
 
         public static string HitsFilePath;
         public static string HitsMemoryMapName;
-        public static int HitsArraySize;
+        public static int[] HitsArray;
 
-        // Special case when instrumenting CoreLib, the thread static below prevents infinite loop in CoreLib
+        // Special case when instrumenting CoreLib, the static below prevents infinite loop in CoreLib
         // while allowing the tracker template to call any of its types and functions.
-        [ThreadStatic]
-        private static bool t_isTracking;
-
-        [ThreadStatic]
-        private static int[] t_threadHits;
-
-        private static readonly List<int[]> _threads;
+        private static bool s_isTracking;
 
         static ModuleTrackerTemplate()
         {
-            t_isTracking = true;
-            _threads = new List<int[]>(2 * Environment.ProcessorCount);
-
+            s_isTracking = true;
             AppDomain.CurrentDomain.ProcessExit += new EventHandler(UnloadModule);
             AppDomain.CurrentDomain.DomainUnload += new EventHandler(UnloadModule);
-            t_isTracking = false;
+            s_isTracking = false;
+
             // At the end of the instrumentation of a module, the instrumenter needs to add code here
             // to initialize the static fields according to the values derived from the instrumentation of
             // the module.
         }
 
         public static void RecordHit(int hitLocationIndex)
         {
-            if (t_isTracking)
+            if (s_isTracking)
                 return;
 
-            if (t_threadHits == null)
-            {
-                t_isTracking = true;
-
-                lock (_threads)
-                {
-                    if (t_threadHits == null)
-                    {
-                        t_threadHits = new int[HitsArraySize];
-                        _threads.Add(t_threadHits);
-                    }
-                }
-
-                t_isTracking = false;
-            }
-
-            ++t_threadHits[hitLocationIndex];
+            Interlocked.Increment(ref HitsArray[hitLocationIndex]);
         }
 
         public static void UnloadModule(object sender, EventArgs e)
         {
-            t_isTracking = true;
-
-            int[][] threads;
-            lock (_threads)
-            {
-                threads = _threads.ToArray();
-
-                // Don't double-count if UnloadModule is called more than once
-                _threads.Clear();
-            }
+            s_isTracking = true;
 
             // The same module can be unloaded multiple times in the same process via different app domains.
             // Use a global mutex to ensure no concurrent access.
@@ -103,7 +71,7 @@ public static void UnloadModule(object sender, EventArgs e)
                     }
                     catch (PlatformNotSupportedException)
                     {
-                        memoryMap = MemoryMappedFile.CreateFromFile(HitsFilePath, FileMode.Open, null, (HitsArraySize + HitsResultHeaderSize) * sizeof(int));
+                        memoryMap = MemoryMappedFile.CreateFromFile(HitsFilePath, FileMode.Open, null, (HitsArray.Length + HitsResultHeaderSize) * sizeof(int));
                     }
 
                     // Tally hit counts from all threads in memory mapped area
@@ -123,18 +91,12 @@ public static void UnloadModule(object sender, EventArgs e)
                                 // the shared data.
                                 Interlocked.Increment(ref *(intPointer + HitsResultUnloadStarted));
 
-                                for (var i = 0; i < HitsArraySize; i++)
+                                for (var i = 0; i < HitsArray.Length; i++)
                                 {
-                                    var count = 0;
-
-                                    foreach (var threadHits in threads)
-                                    {
-                                        count += threadHits[i];
-                                    }
+                                    var count = HitsArray[i];
 
                                     if (count > 0)
                                     {
-                                        // There's a header of one int before the hit counts
                                         var hitLocationArrayOffset = intPointer + i + HitsResultHeaderSize;
 
                                         // No need to use Interlocked here since the mutex ensures only one thread updates 
@@ -155,12 +117,15 @@ public static void UnloadModule(object sender, EventArgs e)
                 }
                 finally
                 {
+                    // Avoid double counting if unloaded multiple times (if that can happen?)
+                    HitsArray = new int[HitsArray.Length];
+                    
                     mutex.ReleaseMutex();
                     memoryMap?.Dispose();
                 }
             }
 
-            t_isTracking = false;
+            s_isTracking = false;
         }
     }
 }

test/coverlet.core.tests/CoverageTests.cs

@@ -36,7 +36,7 @@ public void TestCoverage()
 
             // The module hit tracker must signal to Coverage that it has done its job, so call it manually
             var instrumenterResult = coverage.Results.Single();
-            ModuleTrackerTemplate.HitsArraySize = instrumenterResult.HitCandidates.Count;
+            ModuleTrackerTemplate.HitsArray = new int[instrumenterResult.HitCandidates.Count + ModuleTrackerTemplate.HitsResultHeaderSize];
             ModuleTrackerTemplate.HitsFilePath = instrumenterResult.HitsFilePath;
             ModuleTrackerTemplate.HitsMemoryMapName = instrumenterResult.HitsResultGuid;
             ModuleTrackerTemplate.UnloadModule(null, null);

test/coverlet.core.tests/Instrumentation/ModuleTrackerTemplateTests.cs

@@ -32,11 +32,11 @@ public class ModuleTrackerTemplateTests : IDisposable
 
         public ModuleTrackerTemplateTests()
         {
-            ModuleTrackerTemplate.HitsArraySize = 4;
+            ModuleTrackerTemplate.HitsArray = new int[4 + ModuleTrackerTemplate.HitsResultHeaderSize];
             ModuleTrackerTemplate.HitsMemoryMapName = Guid.NewGuid().ToString();
             ModuleTrackerTemplate.HitsFilePath = Path.Combine(Path.GetTempPath(), $"coverlet.test_{ModuleTrackerTemplate.HitsMemoryMapName}");
 
-            var size = (ModuleTrackerTemplate.HitsArraySize + ModuleTrackerTemplate.HitsResultHeaderSize) * sizeof(int);
+            var size = (ModuleTrackerTemplate.HitsArray.Length + ModuleTrackerTemplate.HitsResultHeaderSize) * sizeof(int);
 
             try
             {
@@ -68,8 +68,7 @@ public void HitsFileCorrectlyWritten()
         [Fact]
         public void HitsOnMultipleThreadsCorrectlyCounted()
         {
-            ModuleTrackerTemplate.HitsArraySize = 4;
-            for (int i = 0; i < ModuleTrackerTemplate.HitsArraySize; ++i)
+            for (int i = 0; i < 4; ++i)
             {
                 var t = new Thread(HitIndex);
                 t.Start(i);
@@ -133,7 +132,7 @@ private void RecordHits(params int[] hitCounts)
             // then dropped by UnloadModule the hit counting must be done
             // in a new thread for each test
 
-            Assert.Equal(ModuleTrackerTemplate.HitsArraySize, hitCounts.Length);
+            Assert.Equal(ModuleTrackerTemplate.HitsArray.Length, hitCounts.Length + ModuleTrackerTemplate.HitsResultHeaderSize);
 
             var thread = new Thread(() =>
             {
@@ -162,9 +161,9 @@ private int[] ReadHits(int expectedUnloads = 1)
 
             var hits = new List<int>();
 
-            for (int i = 0; i < ModuleTrackerTemplate.HitsArraySize; ++i)
+            for (int i = ModuleTrackerTemplate.HitsResultHeaderSize; i < ModuleTrackerTemplate.HitsArray.Length; ++i)
             {
-                hits.Add(mmapAccessor.ReadInt32((i + ModuleTrackerTemplate.HitsResultHeaderSize) * sizeof(int)));
+                hits.Add(mmapAccessor.ReadInt32(i * sizeof(int)));
             }
 
             return hits.ToArray();