From c71b7d8e6d427548d86dc5008d5d30fd26b24c9c Mon Sep 17 00:00:00 2001 From: Viktor Hofer Date: Thu, 24 Jan 2019 20:20:56 +0100 Subject: [PATCH] Revert "Merge pull request #276 from petli/memory-mapped-hit-counts" This reverts commit 0d285b187a445bbbe50a451c116c003dd509134b, reversing changes made to 15a4e62cf0e800f59678f7973ac0d12fe24f0eeb. --- src/coverlet.core/Coverage.cs | 50 ++----- .../Instrumentation/Instrumenter.cs | 14 +- .../Instrumentation/InstrumenterResult.cs | 1 - .../ModuleTrackerTemplate.cs | 92 +++++-------- .../coverlet.template.csproj | 1 - test/coverlet.core.tests/CoverageTests.cs | 13 +- .../ModuleTrackerTemplateTests.cs | 123 +++++++----------- 7 files changed, 100 insertions(+), 194 deletions(-) diff --git a/src/coverlet.core/Coverage.cs b/src/coverlet.core/Coverage.cs index 1d229b5b6..cd1cf6af1 100644 --- a/src/coverlet.core/Coverage.cs +++ b/src/coverlet.core/Coverage.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Generic; using System.IO; -using System.IO.MemoryMappedFiles; using System.Linq; using Coverlet.Core.Enums; @@ -27,15 +26,11 @@ public class Coverage private bool _useSourceLink; private List _results; - private readonly Dictionary _resultMemoryMaps = new Dictionary(); - public string Identifier { get { return _identifier; } } - internal IEnumerable Results => _results; - public Coverage(string module, string[] includeFilters, string[] includeDirectories, string[] excludeFilters, string[] excludedSourceFiles, string[] excludeAttributes, string mergeWith, bool useSourceLink) { _module = module; @@ -82,26 +77,6 @@ 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() @@ -208,6 +183,12 @@ 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 documents = result.Documents.Values.ToList(); if (_useSourceLink && result.SourceLink != null) { @@ -219,26 +200,20 @@ private void CalculateCoverage() } } - // Read hit counts from the memory mapped area, disposing it when done - using (var mmapFile = _resultMemoryMaps[result.HitsResultGuid]) + using (var fs = new FileStream(result.HitsFilePath, FileMode.Open)) + using (var br = new BinaryReader(fs)) { - var mmapAccessor = mmapFile.CreateViewAccessor(); - - var unloadStarted = mmapAccessor.ReadInt32(ModuleTrackerTemplate.HitsResultUnloadStarted * sizeof(int)); - var unloadFinished = mmapAccessor.ReadInt32(ModuleTrackerTemplate.HitsResultUnloadFinished * sizeof(int)); + int hitCandidatesCount = br.ReadInt32(); - if (unloadFinished < unloadStarted) - { - throw new Exception($"Hit counts only partially reported for {result.Module}"); - } + // TODO: hitCandidatesCount should be verified against result.HitCandidates.Count var documentsList = result.Documents.Values.ToList(); - for (int i = 0; i < result.HitCandidates.Count; ++i) + for (int i = 0; i < hitCandidatesCount; ++i) { var hitLocation = result.HitCandidates[i]; var document = documentsList[hitLocation.docIndex]; - var hits = mmapAccessor.ReadInt32((i + ModuleTrackerTemplate.HitsResultHeaderSize) * sizeof(int)); + int hits = br.ReadInt32(); if (hitLocation.isBranch) { @@ -281,7 +256,6 @@ 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 index ee5f8c5fd..82ed6f9db 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -25,9 +25,8 @@ internal class Instrumenter private readonly string[] _excludedAttributes; private readonly bool _isCoreLibrary; private InstrumenterResult _result; - private FieldDefinition _customTrackerHitsFilePath; private FieldDefinition _customTrackerHitsArray; - private FieldDefinition _customTrackerHitsMemoryMapName; + private FieldDefinition _customTrackerHitsFilePath; private ILProcessor _customTrackerClassConstructorIl; private TypeDefinition _customTrackerTypeDef; private MethodReference _customTrackerRegisterUnloadEventsMethod; @@ -59,7 +58,6 @@ public InstrumenterResult Instrument() { Module = Path.GetFileNameWithoutExtension(_module), HitsFilePath = hitsFilePath, - HitsResultGuid = Guid.NewGuid().ToString(), ModulePath = _module }; @@ -127,8 +125,6 @@ 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) { @@ -174,12 +170,10 @@ private void AddCustomModuleTrackerToModule(ModuleDefinition module) _customTrackerTypeDef.Fields.Add(fieldClone); - if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsFilePath)) - _customTrackerHitsFilePath = fieldClone; - else if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsMemoryMapName)) - _customTrackerHitsMemoryMapName = fieldClone; - else if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsArray)) + if (fieldClone.Name == "HitsArray") _customTrackerHitsArray = fieldClone; + else if (fieldClone.Name == "HitsFilePath") + _customTrackerHitsFilePath = fieldClone; } foreach (MethodDefinition methodDef in moduleTrackerTemplate.Methods) diff --git a/src/coverlet.core/Instrumentation/InstrumenterResult.cs b/src/coverlet.core/Instrumentation/InstrumenterResult.cs index 71cb44d29..5c3d374e2 100644 --- a/src/coverlet.core/Instrumentation/InstrumenterResult.cs +++ b/src/coverlet.core/Instrumentation/InstrumenterResult.cs @@ -43,7 +43,6 @@ public InstrumenterResult() public string Module; public string[] AsyncMachineStateMethod; public string HitsFilePath; - public string HitsResultGuid; public string ModulePath; public string SourceLink; public Dictionary Documents { get; private set; } diff --git a/src/coverlet.template/ModuleTrackerTemplate.cs b/src/coverlet.template/ModuleTrackerTemplate.cs index 57ffe0dc8..2bb959971 100644 --- a/src/coverlet.template/ModuleTrackerTemplate.cs +++ b/src/coverlet.template/ModuleTrackerTemplate.cs @@ -1,7 +1,6 @@ using System; using System.Diagnostics.CodeAnalysis; using System.IO; -using System.IO.MemoryMappedFiles; using System.Threading; namespace Coverlet.Core.Instrumentation @@ -17,12 +16,7 @@ namespace Coverlet.Core.Instrumentation [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() @@ -63,72 +57,56 @@ 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, HitsMemoryMapName + "_Mutex", out bool createdNew)) + using (var mutex = new Mutex(true, Path.GetFileNameWithoutExtension(HitsFilePath) + "_Mutex", out bool createdNew)) { if (!createdNew) mutex.WaitOne(); - MemoryMappedFile memoryMap = null; - + bool failedToCreateNewHitsFile = false; try { - try + using (var fs = new FileStream(HitsFilePath, FileMode.CreateNew)) + using (var bw = new BinaryWriter(fs)) { - memoryMap = MemoryMappedFile.OpenExisting(HitsMemoryMapName); - } - catch (PlatformNotSupportedException) - { - memoryMap = MemoryMappedFile.CreateFromFile(HitsFilePath, FileMode.Open, null, (HitsArray.Length + HitsResultHeaderSize) * sizeof(int)); + bw.Write(hitsArray.Length); + foreach (int hitCount in hitsArray) + { + bw.Write(hitCount); + } } + } + catch + { + failedToCreateNewHitsFile = true; + } - // Tally hit counts from all threads in memory mapped area - var accessor = memoryMap.CreateViewAccessor(); - using (var buffer = accessor.SafeMemoryMappedViewHandle) + 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)) { - unsafe + int hitsLength = br.ReadInt32(); + if (hitsLength != hitsArray.Length) { - byte* pointer = null; - buffer.AcquirePointer(ref pointer); - try - { - var intPointer = (int*) pointer; - - // 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; - } - } + throw new InvalidOperationException( + $"{HitsFilePath} has {hitsLength} entries but on memory {nameof(HitsArray)} has {hitsArray.Length}"); + } - // Signal back to coverage analysis that all hit counts were successfully tallied. - Interlocked.Increment(ref *(intPointer + HitsResultUnloadFinished)); - } - finally - { - buffer.ReleasePointer(); - } + for (int i = 0; i < hitsLength; ++i) + { + int oldHitCount = br.ReadInt32(); + bw.Seek(-sizeof(int), SeekOrigin.Current); + bw.Write(hitsArray[i] + oldHitCount); } } } - finally - { - mutex.ReleaseMutex(); - memoryMap?.Dispose(); - } + + // 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(); } } } diff --git a/src/coverlet.template/coverlet.template.csproj b/src/coverlet.template/coverlet.template.csproj index 7f22bfd6b..dbdcea46b 100644 --- a/src/coverlet.template/coverlet.template.csproj +++ b/src/coverlet.template/coverlet.template.csproj @@ -2,7 +2,6 @@ netstandard2.0 - true diff --git a/test/coverlet.core.tests/CoverageTests.cs b/test/coverlet.core.tests/CoverageTests.cs index eed0eb55a..5aba981fa 100644 --- a/test/coverlet.core.tests/CoverageTests.cs +++ b/test/coverlet.core.tests/CoverageTests.cs @@ -6,13 +6,9 @@ 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] @@ -26,7 +22,7 @@ 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: Mimic hits by calling ModuleTrackerTemplate.RecordHit before Unload + // TODO: Find a way to mimick hits // Since Coverage only instruments dependancies, we need a fake module here var testModule = Path.Combine(directory.FullName, "test.module.dll"); @@ -34,13 +30,6 @@ public void TestCoverage() var coverage = new Coverage(testModule, Array.Empty(), Array.Empty(), Array.Empty(), Array.Empty(), Array.Empty(), 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/Instrumentation/ModuleTrackerTemplateTests.cs b/test/coverlet.core.tests/Instrumentation/ModuleTrackerTemplateTests.cs index 27ad1e12e..d77532b7a 100644 --- a/test/coverlet.core.tests/Instrumentation/ModuleTrackerTemplateTests.cs +++ b/test/coverlet.core.tests/Instrumentation/ModuleTrackerTemplateTests.cs @@ -1,9 +1,6 @@ using Coverlet.Core.Instrumentation; -using Coverlet.Core.Helpers; using System; -using System.Collections.Generic; using System.IO; -using System.IO.MemoryMappedFiles; using System.Threading; using System.Threading.Tasks; using Xunit; @@ -12,6 +9,11 @@ namespace Coverlet.Core.Tests.Instrumentation { public class ModuleTrackerTemplateTestsFixture : IDisposable { + public ModuleTrackerTemplateTestsFixture() + { + ModuleTrackerTemplate.HitsFilePath = Path.Combine(Path.GetTempPath(), nameof(ModuleTrackerTemplateTests)); + } + public void Dispose() { AppDomain.CurrentDomain.ProcessExit -= ModuleTrackerTemplate.UnloadModule; @@ -19,65 +21,50 @@ public void Dispose() } } - [CollectionDefinition(nameof(ModuleTrackerTemplate))] - public class ModuleTrackerTemplateCollection : ICollectionFixture - { - - } - - [Collection(nameof(ModuleTrackerTemplate))] - public class ModuleTrackerTemplateTests : IDisposable + public class ModuleTrackerTemplateTests : IClassFixture, IDisposable { - private readonly MemoryMappedFile _mmap; public ModuleTrackerTemplateTests() { - 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.HitsArray.Length + ModuleTrackerTemplate.HitsResultHeaderSize) * sizeof(int); - - try - { - _mmap = MemoryMappedFile.CreateNew(ModuleTrackerTemplate.HitsMemoryMapName, size); - } - catch (PlatformNotSupportedException) - { - _mmap = MemoryMappedFile.CreateFromFile(ModuleTrackerTemplate.HitsFilePath, FileMode.CreateNew, null, size); - } + File.Delete(ModuleTrackerTemplate.HitsFilePath); } public void Dispose() { - var hitsFilePath = ModuleTrackerTemplate.HitsFilePath; - _mmap.Dispose(); - InstrumentationHelper.DeleteHitsFile(hitsFilePath); + File.Delete(ModuleTrackerTemplate.HitsFilePath); } [Fact] public void HitsFileCorrectlyWritten() { - RecordHits(1, 2, 0, 3); + ModuleTrackerTemplate.HitsArray = new[] { 1, 2, 0, 3 }; ModuleTrackerTemplate.UnloadModule(null, null); var expectedHitsArray = new[] { 1, 2, 0, 3 }; - Assert.Equal(expectedHitsArray, ReadHits()); + Assert.Equal(expectedHitsArray, ReadHitsFile()); } [Fact] + public void HitsFileWithDifferentNumberOfEntriesCausesExceptionOnUnload() + { + WriteHitsFile(new[] { 1, 2, 3 }); + ModuleTrackerTemplate.HitsArray = new[] { 1 }; + Assert.Throws(() => ModuleTrackerTemplate.UnloadModule(null, null)); + } + + [Fact(Skip="Failed CI Job: https://ci.appveyor.com/project/tonerdo/coverlet/builds/21145989/job/9gx5jnjs502vy1fv")] public void HitsOnMultipleThreadsCorrectlyCounted() { - for (int i = 0; i < 4; ++i) + ModuleTrackerTemplate.HitsArray = new[] { 0, 0, 0, 0 }; + for (int i = 0; i < ModuleTrackerTemplate.HitsArray.Length; ++i) { var t = new Thread(HitIndex); t.Start(i); - t.Join(); } ModuleTrackerTemplate.UnloadModule(null, null); var expectedHitsArray = new[] { 4, 3, 2, 1 }; - Assert.Equal(expectedHitsArray, ReadHits()); + Assert.Equal(expectedHitsArray, ReadHitsFile()); void HitIndex(object index) { @@ -92,81 +79,67 @@ void HitIndex(object index) [Fact] public void MultipleSequentialUnloadsHaveCorrectTotalData() { - RecordHits(0, 3, 2, 1); + ModuleTrackerTemplate.HitsArray = new[] { 0, 3, 2, 1 }; ModuleTrackerTemplate.UnloadModule(null, null); - RecordHits(0, 1, 2, 3); + ModuleTrackerTemplate.HitsArray = new[] { 0, 1, 2, 3 }; ModuleTrackerTemplate.UnloadModule(null, null); var expectedHitsArray = new[] { 0, 4, 4, 4 }; - Assert.Equal(expectedHitsArray, ReadHits(2)); + Assert.Equal(expectedHitsArray, ReadHitsFile()); } [Fact] public async void MutexBlocksMultipleWriters() { using (var mutex = new Mutex( - true, Path.GetFileNameWithoutExtension(ModuleTrackerTemplate.HitsMemoryMapName) + "_Mutex", out bool createdNew)) + true, Path.GetFileNameWithoutExtension(ModuleTrackerTemplate.HitsFilePath) + "_Mutex", out bool createdNew)) { Assert.True(createdNew); - RecordHits(0, 1, 2, 3); + ModuleTrackerTemplate.HitsArray = new[] { 0, 1, 2, 3 }; var unloadTask = Task.Run(() => ModuleTrackerTemplate.UnloadModule(null, null)); Assert.False(unloadTask.Wait(5)); - var expectedHitsArray = new[] { 0, 0, 0, 0 }; - Assert.Equal(expectedHitsArray, ReadHits(0)); + WriteHitsFile(new[] { 0, 3, 2, 1 }); + + Assert.False(unloadTask.Wait(5)); mutex.ReleaseMutex(); await unloadTask; - expectedHitsArray = new[] { 0, 1, 2, 3 }; - Assert.Equal(expectedHitsArray, ReadHits()); + var expectedHitsArray = new[] { 0, 4, 4, 4 }; + Assert.Equal(expectedHitsArray, ReadHitsFile()); } } - private void RecordHits(params int[] hitCounts) + private void WriteHitsFile(int[] hitsArray) { - // Since the hit array is held in a thread local member that is - // then dropped by UnloadModule the hit counting must be done - // in a new thread for each test - - Assert.Equal(ModuleTrackerTemplate.HitsArray.Length, hitCounts.Length + ModuleTrackerTemplate.HitsResultHeaderSize); - - var thread = new Thread(() => + using (var fs = new FileStream(ModuleTrackerTemplate.HitsFilePath, FileMode.Create)) + using (var bw = new BinaryWriter(fs)) { - for (var i = 0; i < hitCounts.Length; i++) + bw.Write(hitsArray.Length); + foreach (int hitCount in hitsArray) { - var count = hitCounts[i]; - while (count-- > 0) - { - ModuleTrackerTemplate.RecordHit(i); - } + bw.Write(hitCount); } - }); - thread.Start(); - thread.Join(); + } } - private int[] ReadHits(int expectedUnloads = 1) + private int[] ReadHitsFile() { - var mmapAccessor = _mmap.CreateViewAccessor(); - - var unloadStarted = mmapAccessor.ReadInt32(ModuleTrackerTemplate.HitsResultUnloadStarted * sizeof(int)); - var unloadFinished = mmapAccessor.ReadInt32(ModuleTrackerTemplate.HitsResultUnloadFinished * sizeof(int)); - - Assert.Equal(expectedUnloads, unloadStarted); - Assert.Equal(expectedUnloads, unloadFinished); - - var hits = new List(); - - for (int i = ModuleTrackerTemplate.HitsResultHeaderSize; i < ModuleTrackerTemplate.HitsArray.Length; ++i) + using (var fs = new FileStream(ModuleTrackerTemplate.HitsFilePath, FileMode.Open)) + using (var br = new BinaryReader(fs)) { - hits.Add(mmapAccessor.ReadInt32(i * sizeof(int))); - } + var hitsArray = new int[br.ReadInt32()]; + for (int i = 0; i < hitsArray.Length; ++i) + { + hitsArray[i] = br.ReadInt32(); + } - return hits.ToArray(); + return hitsArray; + } } } }