Mark StringDictionary as obsolete

[Symbai]

I recently discovered StringDictionary. To me it is not clear what's the benefit of using it instead of Dictionary<string,string>. Also benchmarks reveal that it is slower comparing to Dictionary<string,string>. When I searched the internet for its purpose people said its coming from old times https://stackoverflow.com/questions/627716/stringdictionary-vs-dictionarystring-string. So I am suggestion to mark StringDictionary as obsolete and remove it in the future, telling people to use Dictionary<string,string> instead. And maybe doing the same with StringCollection? Please correct me if I am wrong and there is an important reason to use StringDictionary over Dictionary<string,string>.

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i7-4960X CPU 3.60GHz (Haswell), 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=3.1.100
  [Host]     : .NET Core 3.1.0 (CoreCLR 4.700.19.56402, CoreFX 4.700.19.56404), X64 RyuJIT
  Job-JPPKML : .NET Core 3.1.0 (CoreCLR 4.700.19.56402, CoreFX 4.700.19.56404), X64 RyuJIT

Runtime=.NET Core 3.1  InvocationCount=1  UnrollFactor=1

Add

Method	Mean	Error	StdDev	Median	Gen 0	Gen 1	Gen 2	Allocated
Dictionary_50	5.347 us	0.1100 us	0.2145 us	5.300 us	-	-	-	2.5 KB
StringDictionary_50	7.447 us	0.1526 us	0.2977 us	7.300 us	-	-	-	2.5 KB
Dictionary_50k	4,659.986 us	81.8376 us	72.5469 us	4,665.150 us	-	-	-	3124.38 KB
StringDictionary_50k	6,759.100 us	129.5854 us	121.2143 us	6,716.400 us	-	-	-	3124.38 KB

Add Benchmark

[CoreJob]
[MemoryDiagnoser]
[RPlotExporter]
public class DictionaryTest
{
    private Dictionary<string, string> dict = new Dictionary<string, string>();
    private StringDictionary stringDict = new StringDictionary();


    [IterationCleanup]
    public void Cleanup()
    {
        dict.Clear();
        stringDict.Clear();
    }

    [Benchmark]
    public bool Dictionary_50()
    {
        for (int i = 0; i < 50; i++)
        {
            dict.Add(i.ToString(), i.ToString());
        }
        return true;
    }

    [Benchmark]
    public bool StringDictionary_50()
    {
        for (int i = 0; i < 50; i++)
        {
            stringDict.Add(i.ToString(), i.ToString());
        }
        return true;
    }
    [Benchmark]
    public bool Dictionary_50k()
    {
        for (int i = 0; i < 50_000; i++)
        {
            dict.Add(i.ToString(), i.ToString());
        }
        return true;
    }

    [Benchmark]
    public bool StringDictionary_50k()
    {
        for (int i = 0; i < 50_000; i++)
        {
            stringDict.Add(i.ToString(), i.ToString());
        }
        return true;
    }

}

Contains

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
Dictionary_50	2.772 us	0.0139 us	0.0124 us	0.1602	-	-	1.25 KB
StringDictionary_50	3.211 us	0.0096 us	0.0075 us	0.1602	-	-	1.25 KB
Dictionary_50k	3,219.860 us	45.3422 us	42.4131 us	203.1250	-	-	1562.21 KB
StringDictionary_50k	5,157.772 us	104.9752 us	207.2108 us	203.1250	-	-	1562.19 KB

Contains Benchmark

[CoreJob]
[MemoryDiagnoser]
[RPlotExporter]
public class DictionaryTest
{
    private Dictionary<string, string> dict = new Dictionary<string, string>();
    private StringDictionary stringDict = new StringDictionary();

    [GlobalSetup]
    public void Setup()
    {
        for (int i = 0; i < 50_000; i++)
        {
            dict.Add(i.ToString(), i.ToString());
            stringDict.Add(i.ToString(), i.ToString());
        }
    }

    [Benchmark]
    public bool Dictionary_50()
    {
        for (int i = 0; i < 50; i++)
        {
            var success = dict.ContainsKey(i.ToString());
        }
        return true;
    }

    [Benchmark]
    public bool StringDictionary_50()
    {
        for (int i = 0; i < 50; i++)
        {
            var success = stringDict.ContainsKey(i.ToString());
        }
        return true;
    }
    [Benchmark]
    public bool Dictionary_50k()
    {
        for (int i = 0; i < 50_000; i++)
        {
            var success = dict.ContainsKey(i.ToString());
        }
        return true;
    }

    [Benchmark]
    public bool StringDictionary_50k()
    {
        for (int i = 0; i < 50_000; i++)
        {
            var success = stringDict.ContainsKey(i.ToString());
        }
        return true;
    }

}

[Clockwork-Muse]

Hm. We've traditionally been against marking things Obsolete (or removing them) due to breaking changes. Although possibly the next version of things will allow this.

Anecdotally, part of a class I took in college was writing your own specialized versions of collections. So basically this code.

[Gnbrkm41]

This (and probably many other specialised collections) are from pre-.NET Framework 2.0 when there was no generics. I guess this reduced the needs for having to write strongly-typed wrappers over HashTable/ArrayList/Etc. I think nobody just bothered to work on it because generic collections are superior over these specialised ones due to generic ones being multi-purpose compared to this.

I bet there's surprising amount of apps which use this and many other non-generic collections like ArrayList and HashTable (because "they just work"). Adding ObsoleteAttribute might cause some trouble for those.

[eiriktsarpalis]

@terrajobst what's our stance w.r.t. obsoleting non-generic collections?

[miloush]

I am generally sceptical about obsoleting non-generic collections, but I have to say that the fact that StringDictionary is lower casing all string keys using invariant culture is troubling me (and explains most of the performance difference). However it also means that StringDictionary cannot be simply replaced by Dictionary<string, string> (you could theoretically supply a custom comparer calling ToLower but that will not give you lower case keys on enumeration).

I don't know what the documentatation is built from, but the code already suggests this class should be considered obsolete, and the constructor notes it shouldn't be used for files etc. The [Obsolete] attribute seems to be justified and expected here.

I cannot say the same about StringCollection though. There is nothing wrong with that one, it's just non-generic. It's a heavily used type, most notably it's used in .settings files and the settings designer. If you are concerned about performance, you could probably just replace it with StringCollection : List<string> { } but don't forget these classes are serializable so it might be a breaking change.

[Symbai]

but the code already suggests this class should be considered obsolete, and the constructor notes it shouldn't be used for files etc. The [Obsolete] attribute seems to be justified and expected here.

That's the point. If the docs say it should be considered as obsolete, it should have an obsolete attribute. Obsolete don't break code, it only throws a warning and this warning is not an issue because the docs give this warning as well. If there are apps which relies on it they either find an alternative or stick to an obsolete class.

I've seen many stuff marked as obsolete:
#40756
#39842
#39636
#39413
#39324
#39261
#35722

This includes the BinaryFormatter and Assembly.CodeBase which are both heavily used things and for the binaryformatter there is no 1:1 replacement. I fully understand that making things as obsolete should be done carefully.. but hence if even the documentation says its obsolete, then I dont understand why the class itself does not have this attribute on it.

[miloush]

Just to clarify we are in agreement that StringDictionary should be marked [Obsolete] here.

Obsolete don't break code, it only throws a warning and this warning is not an issue because the docs give this warning as well.

It's not as simple as that. Some codebases treat warnings as errors, and there is an understandable expectation that obsolete stuff will be removed in the future. The fact that we cannot distinguish between "we messed up and this will be going away ASAP" and "you might want to consider this other thing if possible because we like the new design better" is rather unfortunate.

[iSazonov]

StringDictionary is used in public StringDictionary Attributes { get; } in public class TraceSource. It seems we need new API proposal for a replacement.

This is used in PowerShell Core in public API (public partial class PSTraceSource). /cc @SteveL-MSFT

[Kaelum]

@iSazonov you are aware that several of these classes, especially this one, are heavily used by the ASP.NET team, correct?

[iSazonov]

@Kaelum I only pointed that we should think about a replacement early because it affects other public APIs.