1
\$\begingroup\$

I have created a class which handles (what i call) idle chores. These chores need to be executed every x seconds (x is random every time but within a certain range defined in the settings)

With this code review I'm hoping to find inefficiencies in the code, I need to optimise this because the performance is somewhat lacking when it goes over 300 chores a second.

The 'main' method is ExecuteIdleChores() this is where everything is executed and new times (for when it needs to be executed again) are calculated.

The code:

/// <summary>
/// Contains the information about the simulation.
/// </summary>
public class Simulation : IDisposable
{
    #region Fields
    private static readonly ILog mLogger = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);

    private Dictionary<Guid, IdleChore> mIdleChores = new Dictionary<Guid, IdleChore>();
    private FastPriorityQueue<IdleSimulationArguments> mIdleChoreQueue = new FastPriorityQueue<IdleSimulationArguments>(2048);
    private bool mIdleChoresRunning = true;
    private bool mDisposing = false;

    //private ConcurrentDictionary<Guid, IdleSimulationArguments> mCommandsForModule = new ConcurrentDictionary<Guid, IdleSimulationArguments>();
    private Task mReportReceivedMessagesTask = null;
    #endregion

    #region Constructors
    /// <summary>
    /// Initializes a new instance of the <see cref="Simulation"/> class.
    /// </summary>
    public Simulation()
    {
        mReportReceivedMessagesTask = Task.Factory.StartNew(() => ExecuteIdleChores(), TaskCreationOptions.LongRunning);
    }
    #endregion

    #region Properties
    /// <summary>
    /// Gets the idle tasks.
    /// </summary>
    /// <value>The idle tasks.</value>
    public Dictionary<Guid, IdleChore> IdleChores
    {
        get
        {
            return mIdleChores;
        }
    }
    #endregion

    /// <summary>
    /// Register a simulation context (Module) for the idle chores.
    /// </summary>
    /// <param name="context">The context needed to run the chore.</param>
    /// <param name="address">The address of the module.</param>
    public void RegisterModuleForIdleChores(SimulationContext context, string address)
    {
        if (mIdleChoreQueue.Any(x => x.Address == address) == false)
        {
            foreach (var idleChore in this.IdleChores)
            {
                var arguments = new IdleSimulationArguments();
                arguments.AddTime = Environment.TickCount;
                arguments.Context = context;
                arguments.IdleChoreIdentification = idleChore.Key;
                arguments.WaitTime = idleChore.Value.IdleMiliSeconds;
                arguments.Address = address;
                mIdleChoreQueue.Enqueue(arguments, arguments.AddTime + arguments.WaitTime);
            }
        }
    }

    /// <summary>
    /// Deregister the module from this simulation.
    /// </summary>
    /// <param name="address">The address of the module.</param>
    public void DeregisterModuleForIdleChores(string address)
    {
        var argumentsToRevove = mIdleChoreQueue.Where(x => x.Address == address);
        foreach (var argument in argumentsToRevove)
        {
            mIdleChoreQueue.Remove(argument);
        }
    }

    /// <summary>
    /// Dergeister all the modules for this simulation.
    /// </summary>
    public void DergeisterAllModules()
    {
        mIdleChoreQueue.Clear();
    }

    /// <summary>
    /// Reports the received messages from the queue.
    /// </summary>
    private void ExecuteIdleChores()
    {
        while (mDisposing == false)
        {
            if (this.mIdleChoresRunning == true)
            {
                try
                {
                    if ((mIdleChoreQueue.First != null) && (Timing.IsTimeOutByTicksNow(mIdleChoreQueue.First.AddTime, mIdleChoreQueue.First.WaitTime)))
                    {
                        var chore = mIdleChores[mIdleChoreQueue.First.IdleChoreIdentification];
                        mIdleChoreQueue.First.AddTime = Environment.TickCount;
                        mIdleChoreQueue.First.WaitTime = chore.IdleMiliSeconds;

                        Task.Factory.StartNew(() =>
                        {
                            chore.Execute(mIdleChoreQueue.First.Context);
                        });

                        mIdleChoreQueue.UpdatePriority(mIdleChoreQueue.First, mIdleChoreQueue.First.AddTime + mIdleChoreQueue.First.WaitTime);
                    }
                    else
                    {
                        Thread.Sleep(10);
                    }
                }
                catch (Exception exception)
                {
                    mLogger.Error("ExecuteIdleChores() EXCEPTION", exception);
                }
            }
            else
            {
                Thread.Sleep(100);
            }
        }
    }

    #region Implementation of IDisposable interface
    /// <summary>
    /// Disposes resources.
    /// </summary>
    /// <param name="disposing">True to dispose managed resources.</param>
    protected virtual void Dispose(bool disposing)
    {
        // Dispose managed resources.
        if ((disposing == true) && (mDisposing == false))
        {
            mDisposing = true;
            mIdleChoresRunning = false;
            if (mReportReceivedMessagesTask != null)
            {
                mReportReceivedMessagesTask.Wait();
                mReportReceivedMessagesTask.Dispose();
                mReportReceivedMessagesTask = null;
            }
        }
    }

    /// <summary>
    /// Disposes resources.
    /// </summary>
    public void Dispose()
    {
        this.Dispose(true);
        GC.SuppressFinalize(this);
    }
    #endregion
}

In case someone is interested the queue I'm using is from here, I used to just put it in a List and use the linq .where() statement to get a list of all the chores that have expired and need to be executed (like this: list.Where(x => Timing.IsTimeOutByTicksNow(x.AddTime, x.WaitTime)) but I thought the .where made it quite slow.

an over simplified version of an idle chore (which can be defined in xml and read into C# code) that would send the 'Message' to some server every 1 to 2 seconds (a random milisecond value is calculated between the min and max values which becomse the waittime in the code)

<IdleChore  MinimumIdleTime="00:00:01" MaximumIdleTime="00:00:02">
    <OptionMessage Message="Some message to send" />
</IdleChore>
\$\endgroup\$
  • 1
    \$\begingroup\$ Well, when main issue is performance...a profiling session is absolutely a must. Also those Thread.Sleep() are pretty strange in performance critical code (here I'd like to see a scheduler instead of polling a list.) In short: which are the results of your profiler? In my little experience guesses about performance are - more often than not - just wrong. \$\endgroup\$ – Adriano Repetti Sep 19 '16 at 12:46
  • \$\begingroup\$ If there is nothing to do then it would be a shame to keep the thread occupied, the Thread.Sleep lowers the cpu usage if there's nothing to do. I do not have a profiler, nothing to share there. There is no need for guessing, if there are obvious flaws to the code or best practices i'm missing then that's what should be pointed out. \$\endgroup\$ – Vincent Sep 19 '16 at 15:17
  • \$\begingroup\$ I there is nothing to do...then you should put your thread to sleep. You know when next operation has to be executed (it's in queue). You may even avoid a separate thread and use the pool. A scheduler does this. I don't even suggest to write one (it may be an hard task to do it right) but there are pretty good libraries for that. I mean: condition is not satisfied for 1 ms and you sleep 10 ms before you check again...you're wasting 9 ms. Don't you have a profiler? VS has and there are few nice free tools. Seriously, if you're optimizing for performance...you should have it. \$\endgroup\$ – Adriano Repetti Sep 19 '16 at 15:58
  • \$\begingroup\$ Why not using a timer instead of the while loop with Thread.Sleep? \$\endgroup\$ – JanDotNet Sep 19 '16 at 17:56
  • 1
    \$\begingroup\$ IMHO yes! There are also some SO discussion about the topic: see here or here or just ask google ;) \$\endgroup\$ – JanDotNet Sep 20 '16 at 11:16
1
\$\begingroup\$

I don't really get what your queue does but this is my understanding of what you're trying to do, and how I would do it.

What you want to accomplish: recur various chores at x intervals.

How I would do it (without caring too much about SOLID etc at this point):

  1. Each chore recurs itself. But this ties the timer in with the business logic. Makes starting and stopping them from whatever is controlling them very easy. Also that handles the issue of the timer expiring before the chore has completed.

Anyway here's an example:

class Program
{
    static void Main(string[] args)
    {
        var list = new List<Chore>();
        for (int i = 0; i < 500; i++)
            list.Add(new Chore(i));

        var rand = new Random();
        list.ForEach(c => c.Start(new TimeSpan(0, 0, 0, rand.Next(1, 5), rand.Next(300, 750))));

        Console.WriteLine("Started");
        Console.ReadLine();

        list.ForEach(c => c.Stop());
        Console.WriteLine("Stopped");
        Console.ReadLine();

    }
}


class Chore
{
    private readonly object syncLock = new object();
    private readonly int choreId;
    private Timer timer;
    private TimeSpan interval;

    public Chore(int id)
    {
        choreId = id;
        timer = new Timer(x => Task.Factory.StartNew(DoChore));
    }

    public void DoChore()
    {
        StopTimer(false);

        Console.WriteLine("Chore {0} on thread {1} doing work", choreId, Thread.CurrentThread.ManagedThreadId);

        StartTimer(interval, interval);
    }

    public void Start(TimeSpan interval)
    {
        this.interval = interval;

        StartTimer(TimeSpan.Zero, interval);
    }

    private void StartTimer(TimeSpan start, TimeSpan interval)
    {
        lock (syncLock)
        {
            if (timer != null)
                timer.Change(start, interval);
        }
    }

    public void Stop()
    {
        StopTimer(true);
    }

    private void StopTimer(bool shutdown)
    {
        lock (syncLock)
        {
            if (timer != null)
            {
                timer.Change(Timeout.Infinite, Timeout.Infinite);

                if (shutdown)
                    timer = null;
            }
        }
    }


}
  1. Use timers in the controlling class. You could have a list of an object that holds a timer and a chore, or associate multiple chores with a timer if the interval is the same, etc. Anyway when the timer goes off the "controller" can stop the timer, start the associated chore, and the task can have a ContinueWith to set the timer back to its interval.

Basically polling is imo a bad idea, you need something like timers, however you use them.

Couple other things: comparing explicitly against false/true looks a bit weird. Also your IDisposable implementation is a bit unnecessary because you don't have a destructor/finalizer, so it's a bit pointless to call GC.SuppressFinalize(this), so you may as well put all your disposing logic in one Dispose method and get rid of the bool disposing since it will only ever be called manually.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Good point about the disposing method, But if i have thousands of chores, wouldn't that be a problem if i'd create thousands of timers? \$\endgroup\$ – Vincent Sep 26 '16 at 7:16
  • \$\begingroup\$ Shouldn't be a problem, though you'd have to test. I tested this with 5000 timers all running on the thread pool, firing between 100 and 2000 ms, doing basically no work (because I'm just trying to see the impact of the timers firing), the program remained at 0% CPU usage in task manager: \$\endgroup\$ – 404 Sep 26 '16 at 10:20
  • \$\begingroup\$ static volatile int counter = 0; static void Main(string[] args) { var l = new List<Timer>(); var r = new Random(); for (int i = 0; i < 5000; i++) { var t = r.Next(100, 2000); l.Add(new Timer(x => Task.Factory.StartNew(() => Interlocked.Increment(ref counter)), null, t, t)); } Console.ReadLine(); } \$\endgroup\$ – 404 Sep 26 '16 at 10:20
  • \$\begingroup\$ Hmm well that seems promising. One other question, why do you not dispose the timer but instead just make it NULL? Is it something you forgot or do you not need to dispose it? \$\endgroup\$ – Vincent Sep 26 '16 at 11:26
  • 1
    \$\begingroup\$ I think disposing a timer just stops it, but there's no reason not to dispose it as well. However I set it to null because the DoChore method stops it on another thread and then starts it again after completing the chore. If you externally call Stop on the object while the chore is running, you don't want the chore to start it again when complete. Simply disposing the timer would result in an ObjectDisposedException when DoChore tries to start it again, so it needs to know if it was permanently stopped. Rather than using a separate variable to store this, I just set the timer to null. \$\endgroup\$ – 404 Sep 26 '16 at 11:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.