Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance improvement during run of instrumented code #172

Merged
merged 1 commit into from
Aug 22, 2018
Merged

Performance improvement during run of instrumented code #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/coverlet.core/Coverage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ private void CalculateCoverage()
continue;
}

List<Document> documents = result.Documents.Values.ToList();

using (var fs = new FileStream(result.HitsFilePath, FileMode.Open))
using (var sr = new StreamReader(fs))
{
Expand All @@ -173,7 +175,7 @@ private void CalculateCoverage()
continue;

bool isBranch = info[0] == "B";
var document = result.Documents.ElementAt(int.Parse(info[1])).Value;
var document = documents[int.Parse(info[1])];

int start = int.Parse(info[2]);
int hits = int.Parse(info[4]);
Expand Down
18 changes: 4 additions & 14 deletions src/coverlet.core/Instrumentation/Instrumenter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,25 +175,20 @@ private void InstrumentIL(MethodDefinition method)

private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor processor, Instruction instruction, SequencePoint sequencePoint)
{
int documentIndex = 0;
if (!_result.Documents.TryGetValue(sequencePoint.Document.Url, out var document))
{
document = new Document { Path = sequencePoint.Document.Url };
documentIndex = _result.Documents.Count;
document.Index = _result.Documents.Count;
Copy link
Collaborator

@tonerdo tonerdo Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh shit, why didn't I think of this 😄

_result.Documents.Add(document.Path, document);
}
else
{
documentIndex = _result.Documents.Keys.ToList().IndexOf(document.Path);
}

for (int i = sequencePoint.StartLine; i <= sequencePoint.EndLine; i++)
{
if (!document.Lines.ContainsKey(i))
document.Lines.Add(i, new Line { Number = i, Class = method.DeclaringType.FullName, Method = method.FullName });
}

string marker = $"L,{documentIndex},{sequencePoint.StartLine},{sequencePoint.EndLine}";
string marker = $"L,{document.Index},{sequencePoint.StartLine},{sequencePoint.EndLine}";

var pathInstr = Instruction.Create(OpCodes.Ldstr, _result.HitsFilePath);
var markInstr = Instruction.Create(OpCodes.Ldstr, marker);
Expand All @@ -208,17 +203,12 @@ private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor

private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor processor, Instruction instruction, BranchPoint branchPoint)
{
int documentIndex = 0;
if (!_result.Documents.TryGetValue(branchPoint.Document, out var document))
{
document = new Document { Path = branchPoint.Document };
documentIndex = _result.Documents.Count;
document.Index = _result.Documents.Count;
_result.Documents.Add(document.Path, document);
}
else
{
documentIndex = _result.Documents.Keys.ToList().IndexOf(document.Path);
}

var key = (branchPoint.StartLine, (int)branchPoint.Ordinal);
if (!document.Branches.ContainsKey(key))
Expand All @@ -235,7 +225,7 @@ private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor
}
);

string marker = $"B,{documentIndex},{branchPoint.StartLine},{branchPoint.Ordinal}";
string marker = $"B,{document.Index},{branchPoint.StartLine},{branchPoint.Ordinal}";

var pathInstr = Instruction.Create(OpCodes.Ldstr, _result.HitsFilePath);
var markInstr = Instruction.Create(OpCodes.Ldstr, marker);
Expand Down
1 change: 1 addition & 0 deletions src/coverlet.core/Instrumentation/InstrumenterResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public Document()
}

public string Path;
public int Index;

public Dictionary<int, Line> Lines { get; private set; }
public Dictionary<(int Line, int Ordinal), Branch> Branches { get; private set; }
Expand Down
5 changes: 3 additions & 2 deletions src/coverlet.tracker/CoverageTracker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace Coverlet.Tracker
public static class CoverageTracker
{
private static List<Dictionary<string, Dictionary<string, int>>> _events;
private static readonly StringHashSuffixComparer _stringHashSuffixComparer = new StringHashSuffixComparer();

[ThreadStatic]
private static Dictionary<string, Dictionary<string, int>> t_events;
Expand All @@ -25,7 +26,7 @@ public static void MarkExecuted(string file, string evt)
{
if (t_events == null)
{
t_events = new Dictionary<string, Dictionary<string, int>>();
t_events = new Dictionary<string, Dictionary<string, int>>(_stringHashSuffixComparer);
Copy link
Collaborator

@tonerdo tonerdo Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, now that I think of it there's really no reason to store the path to the hits file as a key in a dictionary. By design, the hits file for an assembly remains the same for an entire test session. Also, the coverage.tracker assembly is usually loaded by the runtime in different contexts for each assembly that depends on it so there's no chance of multiple instrumented assemblies supplying conflicting information.

I believe this approach was added when we had multiple separate gzipped hits files because some users were experiencing considerable disk usage due to large hits files. That's no longer the case.

In this case we can simply make the hits file a field and assign to it once (if it evaluates to null) and have a standalone Dictionary<string, int> to store the marker information.

Let me know what you think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds correct to me. I'll give a try during the day.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires some risky changes: the same thread can receive events from different modules. In other words while the hits file is the same for a given assembly the same thread may receive markers from different assemblies. I'm going to try some minimal changes to optimize that but perhaps we should go for the safe and simple optimization in the short run, while we work on a more profound one.

Copy link
Collaborator

@tonerdo tonerdo Aug 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's currently possible for the coverlet.tracker assembly to receive markers from different assemblies. Here's an illustration of how I believe things are setup in a multiple assembly scenario.

  1. SomeAssembly.Tests.dll depends on SomeAssembly1.dll and SomeAssembly2.dll which are both instrument-able
  2. SomeAssembly1.dll and SomeAssembly2.dll are instrumented by coverlet and the coverlet.tracker.dll dependency is copied to the same folder the two assemblies occupy.
  3. The test project SomeAssembly.Tests is run and during that process, methods in SomeAssembly1.dll are called which contain instrumentation instructions that call out to coverlet.tracker.dll
  4. During that same process, instrumented methods from SomeAssembly2.dll also call out to coverlet.tracker.dll

What happens in this case is that both SomeAssembly1.dll and SomeAssembly2.dll have coverlet.tracker.dll loaded in different contexts. Whatever changes SomeAssembly1.dll makes to static variables in its own copy of coverlet.tracker.dll doesn't interfere with how SomeAssembly2.dll interacts with its own equivalent copy. SomeAssembly1.dll and SomeAssembly2.dll depend independently on coverlet.tracker.dll the same way two different projects can depend on the same version of Newtonsoft.Json and still not get in each other's way even if their binaries all end up in the same folder. Hence, it's perfectly safe for coverlet.tracker.dll to assume that markers will always come from the same assembly.

See https://github.com/tonerdo/coverlet/blob/1d92ee66a1bb68629bf31022a300e57c36ce535f/src/coverlet.core/CoverageTracker.cs for the original implementation that shipped. It worked perfectly in a multiple assembly scenario, it just wasn't performant or thread safe

Copy link
Contributor Author

@pjanotti pjanotti Aug 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a test to be sure: check https://github.com/tonerdo/coverlet/compare/master...pjanotti:test.multithread?expand=1 It does hit the exception when running the performance test. The thread static is the same in a single app domain (that even doesn't really exist on core) for a given thread, but, the same is true for any static of CoverageTracker in the same app domain.

The loader is doing the right thing: let's say SomeAssembly1.dll is the first to call the marker method, it causes the runtime to load the type CoverageTracker (and perform its static initialization), from that point on any other call, no matter from which module is going to use the already loaded CoverageTracker, unless there are multiple app domains (then you get independent types/statics for each AppDomain, even if they are [ThreadStatic]).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this happen only in a multi-threaded scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, single thread too, you just need different modules on the same thread.

Copy link
Contributor Author

@pjanotti pjanotti Aug 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did an experiment to generate a "tracker type" per module and get rid of the dictionaries, it seems to be going well. It needs more testing to ensure correctness and that it doesn't break some scenarios that are working now. If you want to take a peak before I do some clean-up/polishing/fine tunning take a look here. There is a bit of trade-off and it probably it will use more memory in projects with lower coverage numbers but for the project that I tested here it consistently runs the whole test in ~19 seconds (about 6 times faster than the optimization proposed on this PR).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick peek. Seems like a good crop of changes. Will merge this in now since I'm fine with it, thanks for the change and explanations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tonerdo

lock (_events)
{
_events.Add(t_events);
Expand All @@ -38,7 +39,7 @@ public static void MarkExecuted(string file, string evt)
{
if (!t_events.TryGetValue(file, out var fileEvents))
{
fileEvents = new Dictionary<string, int>();
fileEvents = new Dictionary<string, int>(_stringHashSuffixComparer);
t_events.Add(file, fileEvents);
}

Expand Down
35 changes: 35 additions & 0 deletions src/coverlet.tracker/StringHashSuffixComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
using System.Collections.Generic;

namespace Coverlet.Tracker
{
internal class StringHashSuffixComparer : IEqualityComparer<string>
{
public bool Equals(string x, string y) => string.Equals(x, y);

public int GetHashCode(string s)
{
if (s == null || s.Length == 0)
return 0;

// Hash calculation based on the old implementation of NameTable used in System.Xml
const int SuffixLength = 8;
const int Seed = 1031880390;
int hashCode;
unchecked
{
hashCode = s.Length + Seed;
int i = s.Length > SuffixLength ? s.Length - SuffixLength : 0;
for (; i<s.Length; ++i)
{
hashCode += (hashCode << 7) ^ s[i];
}

hashCode -= hashCode >> 17;
hashCode -= hashCode >> 11;
hashCode -= hashCode >> 5;
}

return hashCode;
}
}
}