Skip to content

Record hits without thread local variables #291

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.

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

Merged
merged 1 commit into from
Jan 15, 2019
Merged
Changes from all commits
Commits
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
51 changes: 8 additions & 43 deletions src/coverlet.template/ModuleTrackerTemplate.cs
Original file line number Diff line number Diff line change
@@ -20,68 +20,33 @@ public static class ModuleTrackerTemplate
public static string HitsFilePath;
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 List<int[]> _threads;
private static bool s_isTracking;

Choose a reason for hiding this comment

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

Late to the party, but is making this non-thread static safe?

Copy link
Contributor Author

@sharwell sharwell Jan 15, 2019

Choose a reason for hiding this comment

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

No. There are only two cases where this flag is used:

  1. The static constructor, which the CLR guarantees will complete on exactly one thread.
  2. The UnloadModule handler, after which point the value will always be true.

By design, there are no more thread static accesses on the RecordHit hot path.


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[HitsArray.Length];
_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;

// Update the global hits array from data from all the threads
lock (_threads)
{
foreach (var threadHits in _threads)
{
for (int i = 0; i < HitsArray.Length; ++i)
HitsArray[i] += threadHits[i];
}

// Prevent any double counting scenario, i.e.: UnloadModule called twice (not sure if this can happen in practice ...)
// Only an issue if DomainUnload and ProcessExit can both happens, perhaps can be removed...
_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.