Skip to content

Commit

Permalink
Make counters use dedicated thread instead of timer (dotnet/coreclr#2…
Browse files Browse the repository at this point in the history 
…5864)

* Make a dedicated polling thread for EventCounters

* Cleanup

* remove debug prints

* nit

* Fix comment

* addressing issues from code review

* Fix an assertion from getting fired when we have multiple listeners

* Fix a test issue

* code review feedback

* Fix a potential deadlock

* Fix another deadlock

* Allow s_sleepDurationInMilliseconds to be reset to a larger value

* More issues fix

* Let thread wake up from sleep if needed

* Some suggestions for the counters fix.

The resulting behavior should be the same as what is there now (assuming I didn't mess anything up), these are all just code simplifications that hopefully make it easier to review and maintain going forward. In total I think this reduces the change size in CounterGroup.cs by ~30%.
- With the addition of the AutoResetEvent the ManualResetEvent is no longer needed, removed it.
- Removed the various if foo != null checks for the shared state, it is all initialized once when then thread is created and then assumed to be non-null elsewhere.
- Removed a 2nd lock acquisition inside OnTimer
- Replaced an if with Math.Min in PollForValues

* fix test


Commit migrated from dotnet/coreclr@b3de897
  • Loading branch information
@sywhang
sywhang authored Jul 31, 2019
1 parent 88f7610 commit a8c5b84
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 81 deletions.
12 changes: 4 additions & 8 deletions src/coreclr/tests/src/tracing/eventcounter/eventcounter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ protected override void OnEventWritten(EventWrittenEventArgs eventData)
if (payload.Key.Equals("Mean"))
{
int mean = Int32.Parse(payload.Value.ToString());
Console.WriteLine("Adding " + mean + " to known list of means");
means.Add(mean);
}
else if (payload.Key.Equals("DisplayName"))
Expand All @@ -95,17 +96,12 @@ protected override void OnEventWritten(EventWrittenEventArgs eventData)
public bool validateMean()
{
// we expect to see 1 because we wrote only 1s
// we *might* also see 0 because there is a period of time we didn't write stuff and got callback
if (!means.Contains(1))
{
Console.WriteLine("Mean doesn't have a 1");
return false;
}

// we also expect to see 0 because there is a period of time we didn't write stuff and got callback
if (!means.Contains(0))
{
return false;
}

return true;
}
}
Expand Down Expand Up @@ -159,4 +155,4 @@ public static int Main(string[] args)
}
}
}
}
}
148 changes: 81 additions & 67 deletions src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ internal class CounterGroup
{
private readonly EventSource _eventSource;
private readonly List<DiagnosticCounter> _counters;
private static readonly object s_counterGroupLock = new object();

internal CounterGroup(EventSource eventSource)
{
Expand All @@ -31,13 +32,13 @@ internal CounterGroup(EventSource eventSource)

internal void Add(DiagnosticCounter eventCounter)
{
lock (this) // Lock the CounterGroup
lock (s_counterGroupLock) // Lock the CounterGroup
_counters.Add(eventCounter);
}

internal void Remove(DiagnosticCounter eventCounter)
{
lock (this) // Lock the CounterGroup
lock (s_counterGroupLock) // Lock the CounterGroup
_counters.Remove(eventCounter);
}

Expand All @@ -56,17 +57,17 @@ private void OnEventSourceCommand(object? sender, EventCommandEventArgs e)

if (e.Arguments.TryGetValue("EventCounterIntervalSec", out string? valueStr) && float.TryParse(valueStr, out float value))
{
lock (this) // Lock the CounterGroup
lock (s_counterGroupLock) // Lock the CounterGroup
{
EnableTimer(value);
}
}
}
else if (e.Command == EventCommand.Disable)
{
lock (this)
lock(s_counterGroupLock)
{
_pollingIntervalInMilliseconds = 0;
DisableTimer();
}
}
}
Expand All @@ -79,11 +80,10 @@ private void OnEventSourceCommand(object? sender, EventCommandEventArgs e)
// this table provides the mapping from EventSource -> CounterGroup
// which represents this 'attached' information.
private static WeakReference<CounterGroup>[]? s_counterGroups;
private static readonly object s_counterGroupsLock = new object();

private static void EnsureEventSourceIndexAvailable(int eventSourceIndex)
{
Debug.Assert(Monitor.IsEntered(s_counterGroupsLock));
Debug.Assert(Monitor.IsEntered(s_counterGroupLock));
if (CounterGroup.s_counterGroups == null)
{
CounterGroup.s_counterGroups = new WeakReference<CounterGroup>[eventSourceIndex + 1];
Expand All @@ -98,7 +98,7 @@ private static void EnsureEventSourceIndexAvailable(int eventSourceIndex)

internal static CounterGroup GetCounterGroup(EventSource eventSource)
{
lock (s_counterGroupsLock)
lock (s_counterGroupLock)
{
int eventSourceIndex = EventListener.EventSourceIndex(eventSource);
EnsureEventSourceIndexAvailable(eventSourceIndex);
Expand All @@ -120,32 +120,20 @@ internal static CounterGroup GetCounterGroup(EventSource eventSource)

private DateTime _timeStampSinceCollectionStarted;
private int _pollingIntervalInMilliseconds;
private Timer? _pollingTimer;

private void DisposeTimer()
{
Debug.Assert(Monitor.IsEntered(this));
if (_pollingTimer != null)
{
_pollingTimer.Dispose();
_pollingTimer = null;
}
}
private DateTime _nextPollingTimeStamp;

private void EnableTimer(float pollingIntervalInSeconds)
{
Debug.Assert(Monitor.IsEntered(this));
Debug.Assert(Monitor.IsEntered(s_counterGroupLock));
if (pollingIntervalInSeconds <= 0)
{
DisposeTimer();
_pollingIntervalInMilliseconds = 0;
}
else if (_pollingIntervalInMilliseconds == 0 || pollingIntervalInSeconds * 1000 < _pollingIntervalInMilliseconds)
{
Debug.WriteLine("Polling interval changed at " + DateTime.UtcNow.ToString("mm.ss.ffffff"));
_pollingIntervalInMilliseconds = (int)(pollingIntervalInSeconds * 1000);
DisposeTimer();
ResetCounters(); // Reset statistics for counters before we start the thread.

_timeStampSinceCollectionStarted = DateTime.UtcNow;
// Don't capture the current ExecutionContext and its AsyncLocals onto the timer causing them to live forever
bool restoreFlow = false;
Expand All @@ -157,7 +145,25 @@ private void EnableTimer(float pollingIntervalInSeconds)
restoreFlow = true;
}

_pollingTimer = new Timer(s => ((CounterGroup)s!).OnTimer(null), this, _pollingIntervalInMilliseconds, _pollingIntervalInMilliseconds);
_nextPollingTimeStamp = DateTime.UtcNow + new TimeSpan(0, 0, (int)pollingIntervalInSeconds);

// Create the polling thread and init all the shared state if needed
if (s_pollingThread == null)
{
s_pollingThreadSleepEvent = new AutoResetEvent(false);
s_counterGroupEnabledList = new List<CounterGroup>();
s_pollingThread = new Thread(PollForValues) { IsBackground = true };
s_pollingThread.Start();
}

if (!s_counterGroupEnabledList!.Contains(this))
{
s_counterGroupEnabledList.Add(this);
}

// notify the polling thread that the polling interval may have changed and the sleep should
// be recomputed
s_pollingThreadSleepEvent!.Set();
}
finally
{
Expand All @@ -168,9 +174,15 @@ private void EnableTimer(float pollingIntervalInSeconds)
}
}

private void DisableTimer()
{
_pollingIntervalInMilliseconds = 0;
s_counterGroupEnabledList?.Remove(this);
}

private void ResetCounters()
{
lock (this) // Lock the CounterGroup
lock (s_counterGroupLock) // Lock the CounterGroup
{
foreach (var counter in _counters)
{
Expand All @@ -190,63 +202,65 @@ private void ResetCounters()
}
}

private void OnTimer(object? state)
private void OnTimer()
{
Debug.WriteLine("Timer fired at " + DateTime.UtcNow.ToString("mm.ss.ffffff"));
lock (this) // Lock the CounterGroup
Debug.Assert(Monitor.IsEntered(s_counterGroupLock));
if (_eventSource.IsEnabled())
{
if (_eventSource.IsEnabled())
{
DateTime now = DateTime.UtcNow;
TimeSpan elapsed = now - _timeStampSinceCollectionStarted;
DateTime now = DateTime.UtcNow;
TimeSpan elapsed = now - _timeStampSinceCollectionStarted;

foreach (var counter in _counters)
{
counter.WritePayload((float)elapsed.TotalSeconds, _pollingIntervalInMilliseconds);
}
_timeStampSinceCollectionStarted = now;
}
else
foreach (var counter in _counters)
{
DisposeTimer();
counter.WritePayload((float)elapsed.TotalSeconds, _pollingIntervalInMilliseconds);
}
_timeStampSinceCollectionStarted = now;

do
{
_nextPollingTimeStamp += new TimeSpan(0, 0, 0, 0, _pollingIntervalInMilliseconds);
} while (_nextPollingTimeStamp <= now);
}
}

#region PCL timer hack

#if ES_BUILD_PCL
internal delegate void TimerCallback(object state);

internal sealed class Timer : CancellationTokenSource, IDisposable
{
private int _period;
private TimerCallback _callback;
private object _state;

internal Timer(TimerCallback callback, object state, int dueTime, int period)
{
_callback = callback;
_state = state;
_period = period;
Schedule(dueTime);
}
private static Thread? s_pollingThread;
// Used for sleeping for a certain amount of time while allowing the thread to be woken up
private static AutoResetEvent? s_pollingThreadSleepEvent;

private void Schedule(int dueTime)
{
Task.Delay(dueTime, Token).ContinueWith(OnTimer, null, CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.OnlyOnRanToCompletion, TaskScheduler.Default);
}
private static List<CounterGroup>? s_counterGroupEnabledList;

private void OnTimer(Task t, object s)
private static void PollForValues()
{
AutoResetEvent? sleepEvent = null;
while (true)
{
Schedule(_period);
_callback(_state);
int sleepDurationInMilliseconds = Int32.MaxValue;
lock (s_counterGroupLock)
{
sleepEvent = s_pollingThreadSleepEvent;
foreach (CounterGroup counterGroup in s_counterGroupEnabledList!)
{
DateTime now = DateTime.UtcNow;
if (counterGroup._nextPollingTimeStamp < now + new TimeSpan(0, 0, 0, 0, 1))
{
counterGroup.OnTimer();
}

int millisecondsTillNextPoll = (int)((counterGroup._nextPollingTimeStamp - now).TotalMilliseconds);
millisecondsTillNextPoll = Math.Max(1, millisecondsTillNextPoll);
sleepDurationInMilliseconds = Math.Min(sleepDurationInMilliseconds, millisecondsTillNextPoll);
}
}
if (sleepDurationInMilliseconds == Int32.MaxValue)
{
sleepDurationInMilliseconds = -1; // WaitOne uses -1 to mean infinite
}
sleepEvent?.WaitOne(sleepDurationInMilliseconds);
}

public new void Dispose() { base.Cancel(); }
}
#endif
#endregion // PCL timer hack


#endregion // Timer Processing

Expand Down
14 changes: 8 additions & 6 deletions src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventCounter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,14 @@ internal override void WritePayload(float intervalSec, int pollingIntervalMillis

internal void ResetStatistics()
{
Debug.Assert(Monitor.IsEntered(this));
_count = 0;
_sum = 0;
_sumSquared = 0;
_min = double.PositiveInfinity;
_max = double.NegativeInfinity;
lock(this)
{
_count = 0;
_sum = 0;
_sumSquared = 0;
_min = double.PositiveInfinity;
_max = double.NegativeInfinity;
}
}

#endregion // Statistics Calculation
Expand Down

0 comments on commit a8c5b84

Please sign in to comment.