barnhill / barcodelib

C# Barcode Image Generation Library
Apache License 2.0
733 stars 238 forks source link

Regex usage performance opportunity #144

Closed udlose closed 2 years ago

udlose commented 2 years ago

Current usage of Regex.IsMatch(data, @"^\d+$", RegexOptions.Compiled) in BarcodeCommon.CheckNumericOnly(string data) has room for several performance improvements:

It uses the System.Text.RegularExpressions.Regex.IsMatch(string input, string pattern, RegexOptions options). Behind the scene, the runtime creates a new Regex instance (memory allocation) and compiles and caches the Regex. The size of the Regex cache is only 15 entries per AppDomain. So, depending on what else the app using this nuget package is doing, these new Regex instances may (or may not) be cached/compiled for every execution of CheckNumericOnly.

Changing this Regex instance to a static member of BarcodeCommon ensures that only 1 instance of Regex is ever created per AppDomain, ensures that regex compilation only occurs once, it is guaranteed to be "cached" since it is a static, and avoids being placed in the System.Text.RegularExpressions.Regex cache allowing the application using this nuget to be unaffected by its own usage of static calls to System.Text.RegularExpressions.Regex.

binki commented 2 years ago

Instead you might as well do something like

static bool CheckNumericOnly(string data) {
    for (var i = 0; i < data.Length; i++) {
        if (!char.IsDigit(data[i])) return false;
    }
    return data.Length > 0;
}
barnhill commented 2 years ago

Instead you might as well do something like

static bool CheckNumericOnly(string data) {
    for (var i = 0; i < data.Length; i++) {
        if (!char.IsDigit(data[i])) return false;
    }
    return data.Length > 0;
}

Might be more clear too.

udlose commented 2 years ago

It is as close to allocation-free as I can think of and slightly faster than using a regex. I considered it a micro-optimization but I'll make the change if you're ok with it.

udlose commented 2 years ago

Just to be clear, all of my perf measurements have been done using .NET 6 as I don't have other versions installed on my dev box.

udlose commented 2 years ago

Fixed in merge of PR https://github.com/barnhill/barcodelib/pull/143