5
\$\begingroup\$

I have a bunch of different ports I want to listen on and the iterate over the available data to send to a Dataflow pipeline. In total I'm listening on 14 ports. I'm looking for any advice on how to reduce the CPU usage of the following code.

I'm just passing in the ports to a method then adding them to a list:

   public static void AddPorts(Dictionary<int,string> ports)
{
    try
    {


            var NewEndPoints = new List<IPEndPoint>();
            foreach (var port in ports)
            {
                var endpoint = new IPEndPoint(IPAddress.Any, port.Key);
                NewEndPoints.Add(endpoint);
                if (!Endpoints.Contains(endpoint))
                {                            
                    Endpoints.Add(endpoint);
                    var client = new UdpClient(endpoint);
                    logger.Info("New Client added on port: {0}", endpoint.Port);
                    Clients.Add(client);

                }
                else
                {
                    if (IgnoredPorts.Contains(endpoint.Port))
                    {
                        logger.Info("Existing client enabled on port: {0}", endpoint.Port);
                        IgnoredPorts.Remove(port.Key);
                    }
                }
            }
            var differences = Endpoints.Except(NewEndPoints);
            differences.ToList().ForEach(d =>
            {                         
                if (!IgnoredPorts.Contains(d.Port))
                {
                    IgnoredPorts.Add(d.Port);
                    logger.Info("Client removed on port: {0}", d.Port);
                }
            });



    }
    catch (Exception ex)
    {
        logger.Error("Error creating udpclients", ex);
    } 
}

I then iterate the sockets for any available data

 Task.Run(async delegate
    {

        while (Receive)
        {
            try
            {
                // get any channels that have data availble
                // Iterate over the the channels and send to Dataflow pipeline
                var readyChannels =
            (from channel in Clients
             where channel.Available > 0 && !ListenersDF.IgnoredPorts.Contains(((IPEndPoint)channel.Client.LocalEndPoint).Port)
             select channel);

                // Iterate over the the channels and send to Dataflow pipeline
                foreach (var channel in readyChannels)
                {
                    // await on the result of the task
                    await ReceiveAndRespond(channel);
                }

            }
            catch (Exception ex)
            {
                logger.Error("Error sending packet to bufferBlock", ex);
            }
        }
    });

And finally send it to the TPL Dataflow pipline

async Task ReceiveAndRespond(UdpClient channel)
{
    UdpReceiveResult? result = null;

    try
    {
        result = await channel.ReceiveAsync();
    }
    catch (Exception exc)
    {
        logger.Error("Error receiving from channel: " + exc.Message, exc);
    }

    if (result != null)
    {
        var device = (from d in Ports
                      where d.Key == ((IPEndPoint)channel.Client.LocalEndPoint).Port
                      select d.Value).FirstOrDefault();
        UdpData data = new UdpData() { Client = channel, Data = result.Value.Buffer, LocalPort = ((IPEndPoint)channel.Client.LocalEndPoint).Port, LocalIP = ((IPEndPoint)channel.Client.LocalEndPoint).Address, RemoteEndpoint = result.Value.RemoteEndPoint, Device = device };
        Flow.bufferBlock.Post(data);

        // for testing logs the hex string to a log file              
        //logger.Warn(string.Format("Data received on port: {0} for device: {1} with data: {2}", data.LocalPort, data.Device, data.Data.ByteArrayToHexString()));              
    }
}

Then CPU sits at 50% with hardly any traffic and I'm sure there is some performance to be gained just not sure where.

\$\endgroup\$
3
\$\begingroup\$

Note that Endpoints.Contains(endpoint) will never be true, because you create a new IPEndPoint, and it does not implement IEquatable interface.

In addition to @mjolka's description why you have a 50% CPU load, I would like to suggest how you can avoid it.

Currently you have a central place that manages all sockets in one place. I suggest to do it differently - have each endpoint handle the data on its own, and manage all sockets only as a collection of ongoing processing (tasks) like this:

public async Task RunListenerAsync(int port)
{
    var endpoint = new IPEndPoint(IPAddress.Any, port);
    using (var client = new UdpClient(endpoint))
    {
        while (true)
        {
            try
            {
                var result = await client.ReceiveAsync().ConfigureAwait(false);
                ProcessEndpointData(client, result);
            }
            catch (Exception exc)
            {
                logger.Error("Error receiving from channel: " + exc.Message, exc);
                return;
            }
        }
    }
}

private void ProcessEndpointData(UdpClient client, UdpReceiveResult result)
{
    var localEndPoint = (IPEndPoint)client.Client.LocalEndPoint;
    var device = (from d in Ports
                    where d.Key == localEndPoint.Port
                    select d.Value).FirstOrDefault();

    var data = new UdpData
    {
        Client = client,
        Data = result.Buffer,
        LocalPort = localEndPoint.Port,
        LocalIP = localEndPoint.Address,
        RemoteEndpoint = result.RemoteEndPoint,
        Device = device
    };
    Flow.bufferBlock.Post(data);
}

Note that in this implementation the only way to stop listening the port is to get an exception on await client.ReceiveAsync(). You might want to introduce the CancellationToken to have a control when to stop this process.

With this approach registering and listening for all ports will be as simple as:

public Task ListenPortsAsync(IEnumerable<int> ports)
{
    return Task.WhenAll(ports.Select(RunListenerAsync));
}
\$\endgroup\$
0
2
\$\begingroup\$

It's not surprising that CPU usage is at 50%, it looks like you're busy-waiting.

while (Receive)
{
    try
    {
        var readyChannels = ...
        foreach (var channel in readyChannels)
        {
            ...
        }
    }
    catch (Exception ex)
    {
        ...
    }
}

From Wikipedia,

In software engineering, busy-waiting or spinning is a technique in which a process repeatedly checks to see if a condition is true, such as whether keyboard input or a lock is available.

...

Spinning can be a valid strategy in certain circumstances... In general, however, spinning is considered an anti-pattern and should be avoided, as processor time that could be used to execute a different task is instead wasted on useless activity.

You'll want to restructure your program so that loop can be removed.

\$\endgroup\$
1
  • \$\begingroup\$ The problem is he doesn't have any readyChannels sometimes (or always because contains returns false) and the await inside the foreach is never called \$\endgroup\$
    – JasonLind
    Feb 28 '15 at 18:54

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.