Skip to content

Commit c2ba965

Browse files
authored
Merge pull request #322 from ViktorHofer/RevertMMF
Revert "Merge pull request #276 from petli/memory-mapped-hit-counts"
2 parents e620b7e + c71b7d8 commit c2ba965

File tree

7 files changed

+100
-194
lines changed

7 files changed

+100
-194
lines changed

src/coverlet.core/Coverage.cs

+12-38
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
using System;
22
using System.Collections.Generic;
33
using System.IO;
4-
using System.IO.MemoryMappedFiles;
54
using System.Linq;
65

76
using Coverlet.Core.Enums;
@@ -27,15 +26,11 @@ public class Coverage
2726
private bool _useSourceLink;
2827
private List<InstrumenterResult> _results;
2928

30-
private readonly Dictionary<string, MemoryMappedFile> _resultMemoryMaps = new Dictionary<string, MemoryMappedFile>();
31-
3229
public string Identifier
3330
{
3431
get { return _identifier; }
3532
}
3633

37-
internal IEnumerable<InstrumenterResult> Results => _results;
38-
3934
public Coverage(string module, string[] includeFilters, string[] includeDirectories, string[] excludeFilters, string[] excludedSourceFiles, string[] excludeAttributes, string mergeWith, bool useSourceLink)
4035
{
4136
_module = module;
@@ -82,26 +77,6 @@ public void PrepareModules()
8277
}
8378
}
8479
}
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-
}
10580
}
10681

10782
public CoverageResult GetCoverageResult()
@@ -208,6 +183,12 @@ private void CalculateCoverage()
208183
{
209184
foreach (var result in _results)
210185
{
186+
if (!File.Exists(result.HitsFilePath))
187+
{
188+
// File not instrumented, or nothing in it called. Warn about this?
189+
continue;
190+
}
191+
211192
List<Document> documents = result.Documents.Values.ToList();
212193
if (_useSourceLink && result.SourceLink != null)
213194
{
@@ -219,26 +200,20 @@ private void CalculateCoverage()
219200
}
220201
}
221202

222-
// Read hit counts from the memory mapped area, disposing it when done
223-
using (var mmapFile = _resultMemoryMaps[result.HitsResultGuid])
203+
using (var fs = new FileStream(result.HitsFilePath, FileMode.Open))
204+
using (var br = new BinaryReader(fs))
224205
{
225-
var mmapAccessor = mmapFile.CreateViewAccessor();
226-
227-
var unloadStarted = mmapAccessor.ReadInt32(ModuleTrackerTemplate.HitsResultUnloadStarted * sizeof(int));
228-
var unloadFinished = mmapAccessor.ReadInt32(ModuleTrackerTemplate.HitsResultUnloadFinished * sizeof(int));
206+
int hitCandidatesCount = br.ReadInt32();
229207

230-
if (unloadFinished < unloadStarted)
231-
{
232-
throw new Exception($"Hit counts only partially reported for {result.Module}");
233-
}
208+
// TODO: hitCandidatesCount should be verified against result.HitCandidates.Count
234209

235210
var documentsList = result.Documents.Values.ToList();
236211

237-
for (int i = 0; i < result.HitCandidates.Count; ++i)
212+
for (int i = 0; i < hitCandidatesCount; ++i)
238213
{
239214
var hitLocation = result.HitCandidates[i];
240215
var document = documentsList[hitLocation.docIndex];
241-
var hits = mmapAccessor.ReadInt32((i + ModuleTrackerTemplate.HitsResultHeaderSize) * sizeof(int));
216+
int hits = br.ReadInt32();
242217

243218
if (hitLocation.isBranch)
244219
{
@@ -281,7 +256,6 @@ private void CalculateCoverage()
281256
}
282257
}
283258

284-
// There's only a hits file on Linux, but if the file doesn't exist this is just a no-op
285259
InstrumentationHelper.DeleteHitsFile(result.HitsFilePath);
286260
}
287261
}

src/coverlet.core/Instrumentation/Instrumenter.cs

+4-10
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,8 @@ internal class Instrumenter
2525
private readonly string[] _excludedAttributes;
2626
private readonly bool _isCoreLibrary;
2727
private InstrumenterResult _result;
28-
private FieldDefinition _customTrackerHitsFilePath;
2928
private FieldDefinition _customTrackerHitsArray;
30-
private FieldDefinition _customTrackerHitsMemoryMapName;
29+
private FieldDefinition _customTrackerHitsFilePath;
3130
private ILProcessor _customTrackerClassConstructorIl;
3231
private TypeDefinition _customTrackerTypeDef;
3332
private MethodReference _customTrackerRegisterUnloadEventsMethod;
@@ -59,7 +58,6 @@ public InstrumenterResult Instrument()
5958
{
6059
Module = Path.GetFileNameWithoutExtension(_module),
6160
HitsFilePath = hitsFilePath,
62-
HitsResultGuid = Guid.NewGuid().ToString(),
6361
ModulePath = _module
6462
};
6563

@@ -127,8 +125,6 @@ private void InstrumentModule()
127125
_customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsArray));
128126
_customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Ldstr, _result.HitsFilePath));
129127
_customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsFilePath));
130-
_customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Ldstr, _result.HitsResultGuid));
131-
_customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsMemoryMapName));
132128

133129
if (containsAppContext)
134130
{
@@ -174,12 +170,10 @@ private void AddCustomModuleTrackerToModule(ModuleDefinition module)
174170

175171
_customTrackerTypeDef.Fields.Add(fieldClone);
176172

177-
if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsFilePath))
178-
_customTrackerHitsFilePath = fieldClone;
179-
else if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsMemoryMapName))
180-
_customTrackerHitsMemoryMapName = fieldClone;
181-
else if (fieldClone.Name == nameof(ModuleTrackerTemplate.HitsArray))
173+
if (fieldClone.Name == "HitsArray")
182174
_customTrackerHitsArray = fieldClone;
175+
else if (fieldClone.Name == "HitsFilePath")
176+
_customTrackerHitsFilePath = fieldClone;
183177
}
184178

185179
foreach (MethodDefinition methodDef in moduleTrackerTemplate.Methods)

src/coverlet.core/Instrumentation/InstrumenterResult.cs

-1
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ public InstrumenterResult()
4343
public string Module;
4444
public string[] AsyncMachineStateMethod;
4545
public string HitsFilePath;
46-
public string HitsResultGuid;
4746
public string ModulePath;
4847
public string SourceLink;
4948
public Dictionary<string, Document> Documents { get; private set; }

src/coverlet.template/ModuleTrackerTemplate.cs

+35-57
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
using System;
22
using System.Diagnostics.CodeAnalysis;
33
using System.IO;
4-
using System.IO.MemoryMappedFiles;
54
using System.Threading;
65

76
namespace Coverlet.Core.Instrumentation
@@ -17,12 +16,7 @@ namespace Coverlet.Core.Instrumentation
1716
[ExcludeFromCodeCoverage]
1817
public static class ModuleTrackerTemplate
1918
{
20-
public const int HitsResultHeaderSize = 2;
21-
public const int HitsResultUnloadStarted = 0;
22-
public const int HitsResultUnloadFinished = 1;
23-
2419
public static string HitsFilePath;
25-
public static string HitsMemoryMapName;
2620
public static int[] HitsArray;
2721

2822
static ModuleTrackerTemplate()
@@ -63,72 +57,56 @@ public static void UnloadModule(object sender, EventArgs e)
6357

6458
// The same module can be unloaded multiple times in the same process via different app domains.
6559
// Use a global mutex to ensure no concurrent access.
66-
using (var mutex = new Mutex(true, HitsMemoryMapName + "_Mutex", out bool createdNew))
60+
using (var mutex = new Mutex(true, Path.GetFileNameWithoutExtension(HitsFilePath) + "_Mutex", out bool createdNew))
6761
{
6862
if (!createdNew)
6963
mutex.WaitOne();
7064

71-
MemoryMappedFile memoryMap = null;
72-
65+
bool failedToCreateNewHitsFile = false;
7366
try
7467
{
75-
try
68+
using (var fs = new FileStream(HitsFilePath, FileMode.CreateNew))
69+
using (var bw = new BinaryWriter(fs))
7670
{
77-
memoryMap = MemoryMappedFile.OpenExisting(HitsMemoryMapName);
78-
}
79-
catch (PlatformNotSupportedException)
80-
{
81-
memoryMap = MemoryMappedFile.CreateFromFile(HitsFilePath, FileMode.Open, null, (HitsArray.Length + HitsResultHeaderSize) * sizeof(int));
71+
bw.Write(hitsArray.Length);
72+
foreach (int hitCount in hitsArray)
73+
{
74+
bw.Write(hitCount);
75+
}
8276
}
77+
}
78+
catch
79+
{
80+
failedToCreateNewHitsFile = true;
81+
}
8382

84-
// Tally hit counts from all threads in memory mapped area
85-
var accessor = memoryMap.CreateViewAccessor();
86-
using (var buffer = accessor.SafeMemoryMappedViewHandle)
83+
if (failedToCreateNewHitsFile)
84+
{
85+
// Update the number of hits by adding value on disk with the ones on memory.
86+
// This path should be triggered only in the case of multiple AppDomain unloads.
87+
using (var fs = new FileStream(HitsFilePath, FileMode.Open, FileAccess.ReadWrite, FileShare.None))
88+
using (var br = new BinaryReader(fs))
89+
using (var bw = new BinaryWriter(fs))
8790
{
88-
unsafe
91+
int hitsLength = br.ReadInt32();
92+
if (hitsLength != hitsArray.Length)
8993
{
90-
byte* pointer = null;
91-
buffer.AcquirePointer(ref pointer);
92-
try
93-
{
94-
var intPointer = (int*) pointer;
95-
96-
// Signal back to coverage analysis that we've started transferring hit counts.
97-
// Use interlocked here to ensure a memory barrier before the Coverage class reads
98-
// the shared data.
99-
Interlocked.Increment(ref *(intPointer + HitsResultUnloadStarted));
100-
101-
for (var i = 0; i < hitsArray.Length; i++)
102-
{
103-
var count = hitsArray[i];
104-
105-
// By only modifying the memory map pages where there have been hits
106-
// unnecessary allocation of all-zero pages is avoided.
107-
if (count > 0)
108-
{
109-
var hitLocationArrayOffset = intPointer + i + HitsResultHeaderSize;
110-
111-
// No need to use Interlocked here since the mutex ensures only one thread updates
112-
// the shared memory map.
113-
*hitLocationArrayOffset += count;
114-
}
115-
}
94+
throw new InvalidOperationException(
95+
$"{HitsFilePath} has {hitsLength} entries but on memory {nameof(HitsArray)} has {hitsArray.Length}");
96+
}
11697

117-
// Signal back to coverage analysis that all hit counts were successfully tallied.
118-
Interlocked.Increment(ref *(intPointer + HitsResultUnloadFinished));
119-
}
120-
finally
121-
{
122-
buffer.ReleasePointer();
123-
}
98+
for (int i = 0; i < hitsLength; ++i)
99+
{
100+
int oldHitCount = br.ReadInt32();
101+
bw.Seek(-sizeof(int), SeekOrigin.Current);
102+
bw.Write(hitsArray[i] + oldHitCount);
124103
}
125104
}
126105
}
127-
finally
128-
{
129-
mutex.ReleaseMutex();
130-
memoryMap?.Dispose();
131-
}
106+
107+
// 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
108+
// this case is relevant when instrumenting corelib since multiple processes can be running against the same instrumented dll.
109+
mutex.ReleaseMutex();
132110
}
133111
}
134112
}

src/coverlet.template/coverlet.template.csproj

-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
<PropertyGroup>
44
<TargetFramework>netstandard2.0</TargetFramework>
5-
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
65
</PropertyGroup>
76

87
</Project>

test/coverlet.core.tests/CoverageTests.cs

+1-12
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,9 @@
66

77
using Coverlet.Core;
88
using System.Collections.Generic;
9-
using System.Linq;
10-
using Coverlet.Core.Instrumentation;
11-
using Coverlet.Core.Tests.Instrumentation;
129

1310
namespace Coverlet.Core.Tests
1411
{
15-
[Collection(nameof(ModuleTrackerTemplate))]
1612
public class CoverageTests
1713
{
1814
[Fact]
@@ -26,21 +22,14 @@ public void TestCoverage()
2622
File.Copy(module, Path.Combine(directory.FullName, Path.GetFileName(module)), true);
2723
File.Copy(pdb, Path.Combine(directory.FullName, Path.GetFileName(pdb)), true);
2824

29-
// TODO: Mimic hits by calling ModuleTrackerTemplate.RecordHit before Unload
25+
// TODO: Find a way to mimick hits
3026

3127
// Since Coverage only instruments dependancies, we need a fake module here
3228
var testModule = Path.Combine(directory.FullName, "test.module.dll");
3329

3430
var coverage = new Coverage(testModule, Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), string.Empty, false);
3531
coverage.PrepareModules();
3632

37-
// The module hit tracker must signal to Coverage that it has done its job, so call it manually
38-
var instrumenterResult = coverage.Results.Single();
39-
ModuleTrackerTemplate.HitsArray = new int[instrumenterResult.HitCandidates.Count + ModuleTrackerTemplate.HitsResultHeaderSize];
40-
ModuleTrackerTemplate.HitsFilePath = instrumenterResult.HitsFilePath;
41-
ModuleTrackerTemplate.HitsMemoryMapName = instrumenterResult.HitsResultGuid;
42-
ModuleTrackerTemplate.UnloadModule(null, null);
43-
4433
var result = coverage.GetCoverageResult();
4534

4635
Assert.NotEmpty(result.Modules);

0 commit comments

Comments
 (0)