Merge pull request #276 from petli/memory-mapped-hit-counts

tonerdo · web-flow · commit 0d285b187a44 · 2019-01-16T22:09:53.000+01:00
Writing hit counts to a shared memory mapped area instead of to a file.
diff --git a/coverlet.sln b/coverlet.sln
diff --git a/src/coverlet.core/Coverage.cs b/src/coverlet.core/Coverage.cs
@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
+using System.IO.MemoryMappedFiles;
 using System.Linq;
 
 using Coverlet.Core.Enums;
@@ -26,11 +27,15 @@ public class Coverage
         private bool _useSourceLink;
         private List<InstrumenterResult> _results;
 
+        private readonly Dictionary<string, MemoryMappedFile> _resultMemoryMaps = new Dictionary<string, MemoryMappedFile>();
+
         public string Identifier
         {
             get { return _identifier; }
         }
 
+        internal IEnumerable<InstrumenterResult> Results => _results;
+
         public Coverage(string module, string[] includeFilters, string[] includeDirectories, string[] excludeFilters, string[] excludedSourceFiles, string[] excludeAttributes, string mergeWith, bool useSourceLink)
         {
             _module = module;
@@ -77,6 +82,26 @@ public void PrepareModules()
                     }
                 }
             }
+
+            foreach (var result in _results)
+            {
+                var size = (result.HitCandidates.Count + ModuleTrackerTemplate.HitsResultHeaderSize) * sizeof(int);
+
+                MemoryMappedFile mmap;
+
+                try
+                {
+                    // Try using a named memory map not backed by a file (currently only supported on Windows)
+                    mmap = MemoryMappedFile.CreateNew(result.HitsResultGuid, size);
+                }
+                catch (PlatformNotSupportedException)
+                {
+                    // Fall back on a file-backed memory map
+                    mmap = MemoryMappedFile.CreateFromFile(result.HitsFilePath, FileMode.CreateNew, null, size);
+                }
+
+                _resultMemoryMaps.Add(result.HitsResultGuid, mmap);
+            }
         }
 
         public CoverageResult GetCoverageResult()
@@ -183,12 +208,6 @@ private void CalculateCoverage()
         {
             foreach (var result in _results)
             {
-                if (!File.Exists(result.HitsFilePath))
-                {
-                    // File not instrumented, or nothing in it called.  Warn about this?
-                    continue;
-                }
-
                 List<Document> documents = result.Documents.Values.ToList();
                 if (_useSourceLink && result.SourceLink != null)
                 {
@@ -200,20 +219,26 @@ private void CalculateCoverage()
                     }
                 }
 
-                using (var fs = new FileStream(result.HitsFilePath, FileMode.Open))
-                using (var br = new BinaryReader(fs))
+                // Read hit counts from the memory mapped area, disposing it when done
+                using (var mmapFile = _resultMemoryMaps[result.HitsResultGuid])
                 {
-                    int hitCandidatesCount = br.ReadInt32();
+                    var mmapAccessor = mmapFile.CreateViewAccessor();
+
+                    var unloadStarted = mmapAccessor.ReadInt32(ModuleTrackerTemplate.HitsResultUnloadStarted * sizeof(int));
+                    var unloadFinished = mmapAccessor.ReadInt32(ModuleTrackerTemplate.HitsResultUnloadFinished * sizeof(int));
 
-                    // TODO: hitCandidatesCount should be verified against result.HitCandidates.Count
+                    if (unloadFinished < unloadStarted)
+                    {
+                        throw new Exception($"Hit counts only partially reported for {result.Module}");
+                    }
 
                     var documentsList = result.Documents.Values.ToList();
 
-                    for (int i = 0; i < hitCandidatesCount; ++i)
+                    for (int i = 0; i < result.HitCandidates.Count; ++i)
                     {
                         var hitLocation = result.HitCandidates[i];
                         var document = documentsList[hitLocation.docIndex];
-                        int hits = br.ReadInt32();
+                        var hits = mmapAccessor.ReadInt32((i + ModuleTrackerTemplate.HitsResultHeaderSize) * sizeof(int));
 
                         if (hitLocation.isBranch)
                         {
@@ -256,6 +281,7 @@ private void CalculateCoverage()
                     }
                 }
 
+                // There's only a hits file on Linux, but if the file doesn't exist this is just a no-op
                 InstrumentationHelper.DeleteHitsFile(result.HitsFilePath);
             }
         }
diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs
@@ -4,7 +4,6 @@
 using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Linq;
-using System.Reflection;
 
 using Coverlet.Core.Attributes;
 using Coverlet.Core.Helpers;
@@ -25,8 +24,9 @@ internal class Instrumenter
         private readonly string[] _excludedFiles;
         private readonly string[] _excludedAttributes;
         private InstrumenterResult _result;
-        private FieldDefinition _customTrackerHitsArray;
         private FieldDefinition _customTrackerHitsFilePath;
+        private FieldDefinition _customTrackerHitsArray;
+        private FieldDefinition _customTrackerHitsMemoryMapName;
         private ILProcessor _customTrackerClassConstructorIl;
         private TypeDefinition _customTrackerTypeDef;
         private MethodReference _customTrackerRegisterUnloadEventsMethod;
@@ -55,6 +55,7 @@ public InstrumenterResult Instrument()
             {
                 Module = Path.GetFileNameWithoutExtension(_module),
                 HitsFilePath = hitsFilePath,
+                HitsResultGuid = Guid.NewGuid().ToString(),
                 ModulePath = _module
             };
 
@@ -118,6 +119,8 @@ private void InstrumentModule()
                     _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsArray));
                     _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Ldstr, _result.HitsFilePath));
                     _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsFilePath));
+                    _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Ldstr, _result.HitsResultGuid));
+                    _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsMemoryMapName));
 
                     if (containsAppContext)
                     {
@@ -163,10 +166,12 @@ private void AddCustomModuleTrackerToModule(ModuleDefinition module)
 
                     _customTrackerTypeDef.Fields.Add(fieldClone);
 
-                    if (fieldClone.Name == "HitsArray")
-                        _customTrackerHitsArray = fieldClone;
-                    else if (fieldClone.Name == "HitsFilePath")
+                    if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsFilePath))
                         _customTrackerHitsFilePath = fieldClone;
+                    else if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsMemoryMapName))
+                        _customTrackerHitsMemoryMapName = fieldClone;
+                    else if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsArray))
+                        _customTrackerHitsArray = fieldClone;
                 }
 
                 foreach (MethodDefinition methodDef in moduleTrackerTemplate.Methods)
diff --git a/src/coverlet.core/Instrumentation/InstrumenterResult.cs b/src/coverlet.core/Instrumentation/InstrumenterResult.cs
@@ -42,6 +42,7 @@ public InstrumenterResult()
 
         public string Module;
         public string HitsFilePath;
+        public string HitsResultGuid;
         public string ModulePath;
         public string SourceLink;
         public Dictionary<string, Document> Documents { get; private set; }
diff --git a/src/coverlet.core/coverlet.core.csproj b/src/coverlet.core/coverlet.core.csproj
@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+﻿<Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
     <OutputType>Library</OutputType>
diff --git a/src/coverlet.template/ModuleTrackerTemplate.cs b/src/coverlet.template/ModuleTrackerTemplate.cs
@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.IO;
+using System.IO.MemoryMappedFiles;
 using System.Threading;
 
 namespace Coverlet.Core.Instrumentation
@@ -11,13 +12,18 @@ namespace Coverlet.Core.Instrumentation
     /// to a single location.
     /// </summary>
     /// <remarks>
-    /// As this type is going to be customized for each instrumeted module it doesn't follow typical practices
+    /// As this type is going to be customized for each instrumented module it doesn't follow typical practices
     /// regarding visibility of members, etc.
     /// </remarks>
     [ExcludeFromCodeCoverage]
     public static class ModuleTrackerTemplate
     {
+        public const int HitsResultHeaderSize = 2;
+        public const int HitsResultUnloadStarted = 0;
+        public const int HitsResultUnloadFinished = 1;
+
         public static string HitsFilePath;
+        public static string HitsMemoryMapName;
         public static int[] HitsArray;
 
         static ModuleTrackerTemplate()
@@ -53,56 +59,72 @@ public static void UnloadModule(object sender, EventArgs e)
 
             // 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.
-            using (var mutex = new Mutex(true, Path.GetFileNameWithoutExtension(HitsFilePath) + "_Mutex", out bool createdNew))
+            using (var mutex = new Mutex(true, HitsMemoryMapName + "_Mutex", out bool createdNew))
             {
                 if (!createdNew)
                     mutex.WaitOne();
 
-                bool failedToCreateNewHitsFile = false;
+                MemoryMappedFile memoryMap = null;
+
                 try
                 {
-                    using (var fs = new FileStream(HitsFilePath, FileMode.CreateNew))
-                    using (var bw = new BinaryWriter(fs))
+                    try
                     {
-                        bw.Write(hitsArray.Length);
-                        foreach (int hitCount in hitsArray)
-                        {
-                            bw.Write(hitCount);
-                        }
+                        memoryMap = MemoryMappedFile.OpenExisting(HitsMemoryMapName);
+                    }
+                    catch (PlatformNotSupportedException)
+                    {
+                        memoryMap = MemoryMappedFile.CreateFromFile(HitsFilePath, FileMode.Open, null, (HitsArray.Length + HitsResultHeaderSize) * sizeof(int));
                     }
-                }
-                catch
-                {
-                    failedToCreateNewHitsFile = true;
-                }
 
-                if (failedToCreateNewHitsFile)
-                {
-                    // Update the number of hits by adding value on disk with the ones on memory.
-                    // This path should be triggered only in the case of multiple AppDomain unloads.
-                    using (var fs = new FileStream(HitsFilePath, FileMode.Open, FileAccess.ReadWrite, FileShare.None))
-                    using (var br = new BinaryReader(fs))
-                    using (var bw = new BinaryWriter(fs))
+                    // Tally hit counts from all threads in memory mapped area
+                    var accessor = memoryMap.CreateViewAccessor();
+                    using (var buffer = accessor.SafeMemoryMappedViewHandle)
                     {
-                        int hitsLength = br.ReadInt32();
-                        if (hitsLength != hitsArray.Length)
+                        unsafe
                         {
-                            throw new InvalidOperationException(
-                                $"{HitsFilePath} has {hitsLength} entries but on memory {nameof(HitsArray)} has {hitsArray.Length}");
-                        }
+                            byte* pointer = null;
+                            buffer.AcquirePointer(ref pointer);
+                            try
+                            {
+                                var intPointer = (int*) pointer;
 
-                        for (int i = 0; i < hitsLength; ++i)
-                        {
-                            int oldHitCount = br.ReadInt32();
-                            bw.Seek(-sizeof(int), SeekOrigin.Current);
-                            bw.Write(hitsArray[i] + oldHitCount);
+                                // Signal back to coverage analysis that we've started transferring hit counts.
+                                // Use interlocked here to ensure a memory barrier before the Coverage class reads
+                                // the shared data.
+                                Interlocked.Increment(ref *(intPointer + HitsResultUnloadStarted));
+
+                                for (var i = 0; i < hitsArray.Length; i++)
+                                {
+                                    var count = hitsArray[i];
+
+                                    // By only modifying the memory map pages where there have been hits
+                                    // unnecessary allocation of all-zero pages is avoided.
+                                    if (count > 0)
+                                    {
+                                        var hitLocationArrayOffset = intPointer + i + HitsResultHeaderSize;
+
+                                        // No need to use Interlocked here since the mutex ensures only one thread updates 
+                                        // the shared memory map.
+                                        *hitLocationArrayOffset += count;
+                                    }
+                                }
+
+                                // Signal back to coverage analysis that all hit counts were successfully tallied.
+                                Interlocked.Increment(ref *(intPointer + HitsResultUnloadFinished));
+                            }
+                            finally
+                            {
+                                buffer.ReleasePointer();
+                            }
                         }
                     }
                 }
-
-                // On purpose this is not under a try-finally: it is better to have an exception if there was any error writing the hits file
-                // this case is relevant when instrumenting corelib since multiple processes can be running against the same instrumented dll.
-                mutex.ReleaseMutex();
+                finally
+                {
+                    mutex.ReleaseMutex();
+                    memoryMap?.Dispose();
+                }
             }
         }
     }
diff --git a/src/coverlet.template/coverlet.template.csproj b/src/coverlet.template/coverlet.template.csproj
@@ -1,7 +1,8 @@
-<Project Sdk="Microsoft.NET.Sdk">
+﻿<Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
     <TargetFramework>netstandard2.0</TargetFramework>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
   </PropertyGroup>
 
 </Project>
diff --git a/test/coverlet.core.tests/CoverageTests.cs b/test/coverlet.core.tests/CoverageTests.cs
@@ -6,9 +6,13 @@
 
 using Coverlet.Core;
 using System.Collections.Generic;
+using System.Linq;
+using Coverlet.Core.Instrumentation;
+using Coverlet.Core.Tests.Instrumentation;
 
 namespace Coverlet.Core.Tests
 {
+    [Collection(nameof(ModuleTrackerTemplate))]
     public class CoverageTests
     {
         [Fact]
@@ -22,14 +26,21 @@ public void TestCoverage()
             File.Copy(module, Path.Combine(directory.FullName, Path.GetFileName(module)), true);
             File.Copy(pdb, Path.Combine(directory.FullName, Path.GetFileName(pdb)), true);
 
-            // TODO: Find a way to mimick hits
+            // TODO: Mimic hits by calling ModuleTrackerTemplate.RecordHit before Unload
 
             // Since Coverage only instruments dependancies, we need a fake module here
             var testModule = Path.Combine(directory.FullName, "test.module.dll");
 
             var coverage = new Coverage(testModule, Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), string.Empty, false);
             coverage.PrepareModules();
 
+            // The module hit tracker must signal to Coverage that it has done its job, so call it manually
+            var instrumenterResult = coverage.Results.Single();
+            ModuleTrackerTemplate.HitsArray = new int[instrumenterResult.HitCandidates.Count + ModuleTrackerTemplate.HitsResultHeaderSize];
+            ModuleTrackerTemplate.HitsFilePath = instrumenterResult.HitsFilePath;
+            ModuleTrackerTemplate.HitsMemoryMapName = instrumenterResult.HitsResultGuid;
+            ModuleTrackerTemplate.UnloadModule(null, null);
+
             var result = coverage.GetCoverageResult();
 
             Assert.NotEmpty(result.Modules);
diff --git a/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs b/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs
@@ -63,7 +63,6 @@ public void TestDeleteHitsFile()
             Assert.False(File.Exists(tempFile));
         }
 
-
         public static IEnumerable<object[]> GetExcludedFilesReturnsEmptyArgs =>
         new[]
         {
diff --git a/test/coverlet.core.tests/Instrumentation/ModuleTrackerTemplateTests.cs b/test/coverlet.core.tests/Instrumentation/ModuleTrackerTemplateTests.cs

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,7 @@`
`1`	`1`	`using System;`
`2`	`2`	`using System.Collections.Generic;`
`3`	`3`	`using System.IO;`
	`4`	`+using System.IO.MemoryMappedFiles;`
`4`	`5`	`using System.Linq;`
`5`	`6`
`6`	`7`	`using Coverlet.Core.Enums;`
`@@ -26,11 +27,15 @@ public class Coverage`
`26`	`27`	`private bool _useSourceLink;`
`27`	`28`	`private List<InstrumenterResult> _results;`
`28`	`29`
	`30`	`+ private readonly Dictionary<string, MemoryMappedFile> _resultMemoryMaps = new Dictionary<string, MemoryMappedFile>();`
	`31`	`+`
`29`	`32`	`public string Identifier`
`30`	`33`	`{`
`31`	`34`	`get { return _identifier; }`
`32`	`35`	`}`
`33`	`36`
	`37`	`+ internal IEnumerable<InstrumenterResult> Results => _results;`
	`38`	`+`
`34`	`39`	`public Coverage(string module, string[] includeFilters, string[] includeDirectories, string[] excludeFilters, string[] excludedSourceFiles, string[] excludeAttributes, string mergeWith, bool useSourceLink)`
`35`	`40`	`{`
`36`	`41`	`_module = module;`
`@@ -77,6 +82,26 @@ public void PrepareModules()`
`77`	`82`	`}`
`78`	`83`	`}`
`79`	`84`	`}`
	`85`	`+`
	`86`	`+ foreach (var result in _results)`
	`87`	`+ {`
	`88`	`+ var size = (result.HitCandidates.Count + ModuleTrackerTemplate.HitsResultHeaderSize) * sizeof(int);`
	`89`	`+`
	`90`	`+ MemoryMappedFile mmap;`
	`91`	`+`
	`92`	`+ try`
	`93`	`+ {`
	`94`	`+ // Try using a named memory map not backed by a file (currently only supported on Windows)`
	`95`	`+ mmap = MemoryMappedFile.CreateNew(result.HitsResultGuid, size);`
	`96`	`+ }`
	`97`	`+ catch (PlatformNotSupportedException)`
	`98`	`+ {`
	`99`	`+ // Fall back on a file-backed memory map`
	`100`	`+ mmap = MemoryMappedFile.CreateFromFile(result.HitsFilePath, FileMode.CreateNew, null, size);`
	`101`	`+ }`
	`102`	`+`
	`103`	`+ _resultMemoryMaps.Add(result.HitsResultGuid, mmap);`
	`104`	`+ }`
`80`	`105`	`}`
`81`	`106`
`82`	`107`	`public CoverageResult GetCoverageResult()`
`@@ -183,12 +208,6 @@ private void CalculateCoverage()`
`183`	`208`	`{`
`184`	`209`	`foreach (var result in _results)`
`185`	`210`	`{`
`186`		`- if (!File.Exists(result.HitsFilePath))`
`187`		`- {`
`188`		`- // File not instrumented, or nothing in it called. Warn about this?`
`189`		`- continue;`
`190`		`- }`
`191`		`-`
`192`	`211`	`List<Document> documents = result.Documents.Values.ToList();`
`193`	`212`	`if (_useSourceLink && result.SourceLink != null)`
`194`	`213`	`{`
`@@ -200,20 +219,26 @@ private void CalculateCoverage()`
`200`	`219`	`}`
`201`	`220`	`}`
`202`	`221`
`203`		`- using (var fs = new FileStream(result.HitsFilePath, FileMode.Open))`
`204`		`- using (var br = new BinaryReader(fs))`
	`222`	`+ // Read hit counts from the memory mapped area, disposing it when done`
	`223`	`+ using (var mmapFile = _resultMemoryMaps[result.HitsResultGuid])`
`205`	`224`	`{`
`206`		`- int hitCandidatesCount = br.ReadInt32();`
	`225`	`+ var mmapAccessor = mmapFile.CreateViewAccessor();`
	`226`	`+`
	`227`	`+ var unloadStarted = mmapAccessor.ReadInt32(ModuleTrackerTemplate.HitsResultUnloadStarted * sizeof(int));`
	`228`	`+ var unloadFinished = mmapAccessor.ReadInt32(ModuleTrackerTemplate.HitsResultUnloadFinished * sizeof(int));`
`207`	`229`
`208`		`- // TODO: hitCandidatesCount should be verified against result.HitCandidates.Count`
	`230`	`+ if (unloadFinished < unloadStarted)`
	`231`	`+ {`
	`232`	`+ throw new Exception($"Hit counts only partially reported for {result.Module}");`
	`233`	`+ }`
`209`	`234`
`210`	`235`	`var documentsList = result.Documents.Values.ToList();`
`211`	`236`
`212`		`- for (int i = 0; i < hitCandidatesCount; ++i)`
	`237`	`+ for (int i = 0; i < result.HitCandidates.Count; ++i)`
`213`	`238`	`{`
`214`	`239`	`var hitLocation = result.HitCandidates[i];`
`215`	`240`	`var document = documentsList[hitLocation.docIndex];`
`216`		`- int hits = br.ReadInt32();`
	`241`	`+ var hits = mmapAccessor.ReadInt32((i + ModuleTrackerTemplate.HitsResultHeaderSize) * sizeof(int));`
`217`	`242`
`218`	`243`	`if (hitLocation.isBranch)`
`219`	`244`	`{`
`@@ -256,6 +281,7 @@ private void CalculateCoverage()`
`256`	`281`	`}`
`257`	`282`	`}`
`258`	`283`
	`284`	`+ // There's only a hits file on Linux, but if the file doesn't exist this is just a no-op`
`259`	`285`	`InstrumentationHelper.DeleteHitsFile(result.HitsFilePath);`
`260`	`286`	`}`
`261`	`287`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-<Project Sdk="Microsoft.NET.Sdk">`
	`1`	`+<Project Sdk="Microsoft.NET.Sdk">`
`2`	`2`
`3`	`3`	`<PropertyGroup>`
`4`	`4`	`<OutputType>Library</OutputType>`
Original file line number	Diff line number	Diff line change
`@@ -63,7 +63,6 @@ public void TestDeleteHitsFile()`
`63`	`63`	`Assert.False(File.Exists(tempFile));`
`64`	`64`	`}`
`65`	`65`
`66`		`-`
`67`	`66`	`public static IEnumerable<object[]> GetExcludedFilesReturnsEmptyArgs =>`
`68`	`67`	`new[]`
`69`	`68`	`{`