HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

performance on this substring search

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thissubstringperformancesearch

Problem

I have the code below that gets hit several hundred times per second. I'm wondering what I can do to improve performance. It seems that there should be some way to build up a substring index. Is there some existing examples of that or an explanation on how to do this?

``
using System;
using System.Collections.Generic;
using System.Linq;
using Asi.Server.Interfaces.History;

namespace Asi.Server.History.AssetHistory
{
public class TelemetrySubscriptionFilter: ITelemetryLogFilter, ITelemetryLogAutosubscriber
{
private readonly List _shouldNotLogs = new List
{
"Local Position Service",
"Global Position Service",
"PositionGroup.",
"DKS/"
};

public bool ShouldNotLog(string telemetryName)
{
float period;
if (ShouldAutoSubscribe(telemetryName, out period))
return false;

// telemetryName is longer than our filter texts
return _shouldNotLogs.Any(s => telemetryName.IndexOf(s, StringComparison.Ordinal) >= 0);
}

private readonly List _shouldAutoSubscribe = new List
{
"Manual Mode",
"Autonomous Driven",
"Battery Voltage",
"RPM",
"Engine On",
"Parking Brake",
"Gear",
"Stopping Distance",
"Ready For Motion",
"GPS Correction Sent",
"Setpoint",
"Dead Reckon",
"VCU uC",
"Feedback",
"Off Path",
"RMS",
"Yaw Rate",
"Stop Enabled",
"Arbiter",
"Velocity Error",
"Processor"
};

public bool ShouldAutoSubscribe(string telemetryName, out float period)
{
period = 0.375f; // chosen somewhat arbitrarily
return _shouldAutoSubscribe.Any(s => telemetryName.IndexOf(s, StringComparison.Ordinal) >= 0);
}
}
}

Solution

As with every performance question: run your code through a profiler

That said, your ShouldNotLog method can be simplified. There is not much reason to check your auto subscribe list prior to checking your suppress list:

  • _shouldNotLogs is a shorter list



  • presumably, none of the strings in _shouldNotLogs should exist in _shouldAutoSubscribe



As a result, checking _shouldAutoSubscribe only adds extra work. The only times you perform fewer operations is if the input name matches one of the first three elements in _shouldAutoSubscribe. It's a wash at four. Anything else is extra work.

This leaves us with the following:

public bool ShouldNotLog(string telemetryName)
    {
        return _shouldNotLogs.Any(s => telemetryName.IndexOf(s, StringComparison.Ordinal) >= 0);
    }


Next, for readability, I would instead use string.Contains. If you check the .NET source, string.Contains calls string.IndexOf internally with an Ordinal check, so it is effectively the same thing, but it declares your intent far better:

public bool ShouldNotLog(string telemetryName)
    {
        return _shouldNotLogs.Any(s => telemetryName.Contains(s));
    }


If possible, though, an even better way of doing it would be to extract your telemetry names to some numerical ID and use enums or ints. Number comparison will be far quicker than string comparison. However, you state that your input names are superstrings of those appearing in your lists, so I don't know if that is feasible.

Code Snippets

public bool ShouldNotLog(string telemetryName)
    {
        return _shouldNotLogs.Any(s => telemetryName.IndexOf(s, StringComparison.Ordinal) >= 0);
    }
public bool ShouldNotLog(string telemetryName)
    {
        return _shouldNotLogs.Any(s => telemetryName.Contains(s));
    }

Context

StackExchange Code Review Q#55518, answer score: 3

Revisions (0)

No revisions yet.