1

I am working on a small app that would track certain GPU parameters. I am currently using 5 background workers for 5 different parameters that are being tracked, the operations run until my app is closed. I know this is probably not a good way to do it. What would be a good way to monitor these parameters in the background without having to create a worker for each parameter?

Edit: Reverted back to the original question that I asked now that the question was reopened.

Test file that monitors the temperature only.

using NvAPIWrapper.GPU;
using System.ComponentModel;
using System.Threading;
using System.Windows.Forms;

namespace TestForm
{
    public partial class Form1 : Form
    {
        private PhysicalGPU[] gpus = PhysicalGPU.GetPhysicalGPUs();

        public Form1()
        {
            InitializeComponent();
            GPUTemperature();
        }

        private void GPUTemperature()
        {
            backgroundWorker1.RunWorkerAsync();
        }

        private void backgroundWorker1_DoWork(object sender, DoWorkEventArgs e)
        {
            while (!backgroundWorker1.CancellationPending)
            {
                foreach (var gpu in gpus)
                {
                    foreach (var sensor in gpu.ThermalInformation.ThermalSensors)
                    {
                        backgroundWorker1.ReportProgress(sensor.CurrentTemperature);
                        Thread.Sleep(500);
                    }
                }
            }
        }

        private void backgroundWorker1_ProgressChanged(object sender,
            ProgressChangedEventArgs e)
        {
            temperature.Text = e.ProgressPercentage.ToString();
        }
    }
}
  • 1
    I'm voting to close this question as off-topic because questions about _code optimisations_ are off-topic. See _[Can I post questions about optimizing code on Stack Overflow?](https://meta.stackoverflow.com/questions/261841/can-i-post-questions-about-optimizing-code-on-stack-overflow)_ and _[Is it okay to ask for code optimization help?](https://meta.stackoverflow.com/questions/286557/is-it-okay-to-ask-for-code-optimization-help)_ for more information. –  Jul 11 '21 at 02:43
  • 1
    My prefered method would be to use the `PeriodicAsync` method found in [this](https://stackoverflow.com/questions/30462079/run-async-method-regularly-with-specified-interval/62724908#62724908) answer. Its advantage over using a `BackgroundWorker` is that it doesn't block a `ThreadPool` thread. The supplied action is invoked on the UI thread, so if I had to I would use `Task.Run` and a `Progress` as shown [here](https://stackoverflow.com/questions/12414601/async-await-vs-backgroundworker/64620920#64620920). Its advantage over using a `System.Timers.Timer` is that it's not reentrant. – Theodor Zoulias Jul 11 '21 at 06:10
  • 1
    I am voting to reopen this question because 1) questions about code optimization are [on topic](https://meta.stackoverflow.com/questions/261841/can-i-post-questions-about-optimizing-code-on-stack-overflow) on StackOverflow and 2) this question is not about code optimization, it is about how to avoid a technique that is known to be wasteful. Not blocking your application's `ThreadPool` threads is not a luxury, it is a necessity. – Theodor Zoulias Jul 11 '21 at 06:20
  • 2
    I agree with @TheodorZoulias, this question appears to be on topic, CodeReview is not the only site delegate to handle this kind of questions. There's also a small debugging issue, since `e.ProgressPercentage` is used in `ReportProgress` and `ProgressChanged`, which is clearly the wrong type: the `ReportProgress(Int32, Object)` overload should be used, in case a BackgroundWorker remains the tool of choice. A (single) threaded Timer is probably enough for this. Handled in a way that doesn't cause overlapping events. – Jimi Jul 11 '21 at 22:22
  • @isqrt3 out of curiosity, why are you using one `BackgroundWorker` for each GPU parameter? Why don't you track all 5 parameters, the one after the other, using a single `BackgroundWorker`? – Theodor Zoulias Jul 11 '21 at 22:42
  • @TheodorZoulias Yes, I am using one for each, I was not sure how to do it with a single worker for multiple parameters. –  Jul 12 '21 at 01:02
  • 1
    Ah, I see. The `Thread.Sleep(500);` is in the inner `foreach` loop, not in the outer `while` loop. Doesn't this result to slow and inconsistent updates? Wouldn't it be better if you updated all sensors of all GPUs every half a second? – Theodor Zoulias Jul 12 '21 at 01:30
  • I actually just changed the `Thread.Sleep(500);` to the outer while loop and It actually reports the GPU temperature back even faster than the NVIDIA sensors overlay does. My only issue now is I'm not sure how to use a single Worker to report all my sensors. Im going to look into what you mentioned about `PeriodicAsync` as well. –  Jul 12 '21 at 01:44
  • 1
    As mentioned, you should use the `ReportProgress(Int32, Object)` overload. Its `Object` parameter can be a class object where you store the values of all the sensors. In `ProgressChanged`, cast `e.UserState` to the class Type and assign its values to multiple UI elements. – Jimi Jul 12 '21 at 02:15
  • @Jimi Thank you! this worked perfectly, I just tested this out with all the parameters I was trying to monitor and I got rid of all those extra background workers. Now one is doing just fine :D Also I didn't see your earlier reply I apologize. Thanks all for helping me even though the question was closed I really appreciate it. –  Jul 12 '21 at 03:11

3 Answers3

1

I was able to solve the issue after getting some help in the comments. Here is my final working code.

using NVIDIAGPU.GPUClock;
using NVIDIAGPU.GPUFan;
using NVIDIAGPU.Memory;
using NVIDIAGPU.Thermals;
using NVIDIAGPU.Usage;
using System.ComponentModel;
using System.Threading;
using System.Windows.Forms;

namespace SysMonitor
{
    public partial class Form1 : Form
    {
        private int[] sensorValues = new int[5];

        public Form1()
        {
            InitializeComponent();

            StartWorkers();
        }

        /// <summary>
        /// Store sensor parameters.
        /// </summary>
        public int[] SensorValues { get => sensorValues; set => sensorValues = value; }

        private void StartWorkers()
        {
            thermalsWorker.RunWorkerAsync();
        }

        #region ThermalWorker

        private void thermalsWorker_DoWork(object sender, DoWorkEventArgs e)
        {
            while (!thermalsWorker.CancellationPending)
            {
                // Assign array values.
                SensorValues[0] = GPUThermals.CurrentTemeperature;
                SensorValues[1] = GPUUsage.CurrentUsage;
                SensorValues[2] = (int)GPUClock.CurrentGPUClock;
                SensorValues[3] = (int)GPUMemory.CurrentMemoryClock;
                SensorValues[4] = GPUFan.CurrentFanRPM;

                // Pass the SensorValues array to the userstate parameter.
                thermalsWorker.ReportProgress(0, SensorValues);
                Thread.Sleep(500);
            }
        }

        private void backgroundWorker1_ProgressChanged(object sender,
            ProgressChangedEventArgs e)
        {
            // Cast user state to array of int and assign values.
            int[] result = (int[])e.UserState;
            gpuTemperatureValue.Text = result[0].ToString() + " °C";
            gpuUsageValue.Text = result[1].ToString() + " %";
            gpuClockValue.Text = result[2].ToString() + " MHz";
            gpuMemoryValue.Text = result[3].ToString() + " MHz";
            gpuFanRPMValue.Text = result[4].ToString() + " RPM";

            
        }
        #endregion ThermalWorker
    }
}
0

I have a problem with all the existing answers as they all fail on basic separation of concerns.

However, to fix this problem, we need to reframe the question. In this case, we do not want to "run an operation", but instead want to "observer/subscribe" to the GPU state.

I prefer observe over sub/pub in this case, as work is done, and you really want to know if there is someone listening to your tree falling in the forest.

Therefore, here is the code for an RX.Net implementation.

public class GpuMonitor
{
    private IObservable<GpuState> _gpuStateObservable;

    public IObservable<GpuState> GpuStateObservable => _gpuStateObservable;

    public GpuMonitor()
    {
        _gpuStateObservable = Observable.Create<GpuState>(async (observer, cancellationToken) => 
        {
            while(!cancellationToken.IsCancellationRequested)
            {
                 await Task.Delay(1000, cancellationToken);
                 var result = ....;
                 observer.OnNext(result);
            }
        })
            .SubscribeOn(TaskPoolScheduler.Default)
            .Publish()
            .RefCount();
    }
}

Then to consume.

public class Form1
{
    private IDisposable _gpuMonitoringSubscription;


    public Form1(GpuMonitor gpuMon)
    {
        InitializeComponent();
        _gpuMonitoringSubscription = gpuMon.GpuStateObservable
                .ObserveOn(SynchronizationContext.Current)
                .Susbscribe(state => {
                     gpuUsageValue.Text = $"{state.Usage} %";
                     //etc
                });
    }

    protected override void Dispose(bool disposing)
    {
        _gpuMonitoringSubscription.Dispose();
        base.Dispose(disposing);
    }

}

The advantage here, is that you can reuse this component in multiple places, with different threads, etc.

Aron
  • 15,464
  • 3
  • 31
  • 64
  • Be careful when you use the `Observable.Create` with async delegate, and then subscribe without `onError` handler. There is a nasty exception-swallowing [bug](https://stackoverflow.com/questions/51714221/async-create-hanging-while-publishing-observable/) that may bite you in this case. If you want to do the looping with Rx, you'd better use the [robust](https://stackoverflow.com/questions/67987954/how-to-make-an-iobservablestring-from-console-input/67995158#67995158) `Repeat` operator instead. – Theodor Zoulias Jul 12 '21 at 12:36
-1

My suggestion is to scrap the technologically obsolete BackgroundWorker, and use instead an asynchronous loop. The reading of the sensor values can be offloaded to a ThreadPool thread by using the Task.Run method, and the idle period between each iteration can be imposed by awaiting a Task.Delay task:

public Form1()
{
    InitializeComponent();
    StartMonitoringSensors();
}

async void StartMonitoringSensors()
{
    while (true)
    {
        var delayTask = Task.Delay(500);
        var (temperature, usage, gpuClock, memory, fan) = await Task.Run(() =>
        {
            return
            (
                GPUThermals.CurrentTemperature,
                GPUUsage.CurrentUsage,
                GPUClock.CurrentGPUClock,
                GPUMemory.CurrentMemoryClock,
                GPUFan.CurrentFanRPM
            );
        });
        gpuTemperatureValue.Text = $"{temperature} °C";
        gpuUsageValue.Text = $"{usage} %";
        gpuClockValue.Text = $"{gpuClock} MHz";
        gpuMemoryValue.Text = $"{memory} MHz";
        gpuFanRPMValue.Text = $"{fan} RPM";
        await delayTask;
    }
}

What you get with this approach:

  1. A ThreadPool thread is not blocked during the idle period between reading the sensors. This fact would be impactful if your application was more complex, and was making heavy use of the ThreadPool. But for a simple application like this, that does nothing else than displaying sensor values, this benefit is mostly academic. The ThreadPool thread will have nothing else to do during the idle period, and it will be idled anyway. In any case, it is a good habit to avoid blocking threads needlessly, whenever you can.

  2. You get strongly typed sensor values passed to the UI thread. No need to cast from the object type, and no need to convert all the values to ints.

  3. Any exceptions thrown by reading the GPUThermals, GPUUsage etc properties, will be rethrown on the UI thread. Your application will not just stop working, without giving any indication that something wrong happened. That's the reason for choosing async void in the signature of the StartMonitoringSensors method. Async void should be avoided in general, but this is an exceptional case where async void is preferable to async Task.

  4. You get consistent updates of the sensor values every 500 msec, because the time needed to read the sensor values is not added to the idle period. This happens because the Task.Delay(500) task is created before reading the values, and it is awaited afterwards.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • 1
    Going to accept this as the answer since It shows a more modern approach as Theodor mentioned. I got a good idea now on how both methods work now thanks everyone. –  Jul 12 '21 at 17:41