From 052f8ebe73092f006a5324a32f0984ecd94a319c Mon Sep 17 00:00:00 2001 From: Marek Safar Date: Sat, 31 Oct 2020 15:27:57 +0100 Subject: [PATCH 1/2] Use Dictionary for underlying cache of ResourceSet to reduce dependency chain - adds tests - replaces exceptions flow with type checks --- .../src/System/Resources/ResourceSet.cs | 107 +++++++----------- .../tests/ResourceSetTests.cs | 64 +++++++++++ 2 files changed, 105 insertions(+), 66 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs index 6ad05b68692935..5e920be3e6257f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs @@ -1,18 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -/*============================================================ -** -** -** -** -** -** Purpose: Culture-specific collection of resources. -** -** -===========================================================*/ - using System.Collections; +using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.IO; @@ -29,15 +19,15 @@ namespace System.Resources public class ResourceSet : IDisposable, IEnumerable { protected IResourceReader Reader = null!; - internal Hashtable? Table; - private Hashtable? _caseInsensitiveTable; // For case-insensitive lookups. + private Dictionary? _table; + private Dictionary? _caseInsensitiveTable; // For case-insensitive lookups. protected ResourceSet() { // To not inconvenience people subclassing us, we should allocate a new // hashtable here just so that Table is set to something. - Table = new Hashtable(); + _table = new Dictionary(); } // For RuntimeResourceSet, ignore the Table parameter - it's a wasted @@ -96,7 +86,7 @@ protected virtual void Dispose(bool disposing) } Reader = null!; _caseInsensitiveTable = null; - Table = null; + _table = null; } public void Dispose() @@ -134,10 +124,8 @@ IEnumerator IEnumerable.GetEnumerator() private IDictionaryEnumerator GetEnumeratorHelper() { - Hashtable? copyOfTable = Table; // Avoid a race with Dispose - if (copyOfTable == null) - throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); - return copyOfTable.GetEnumerator(); + // Avoid a race with Dispose + return _table?.GetEnumerator() ?? throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); } // Look up a string value for a resource given its name. @@ -145,48 +133,37 @@ private IDictionaryEnumerator GetEnumeratorHelper() public virtual string? GetString(string name) { object? obj = GetObjectInternal(name); - try - { - return (string?)obj; - } - catch (InvalidCastException) - { - throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name)); - } + if (obj is string s) + return s; + + if (obj is null) + return null; + + throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name)); } public virtual string? GetString(string name, bool ignoreCase) { - object? obj; - string? s; - // Case-sensitive lookup - obj = GetObjectInternal(name); - try - { - s = (string?)obj; - } - catch (InvalidCastException) - { + object? obj = GetObjectInternal(name); + if (obj is string s) + return s; + + if (obj is not null) throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name)); - } - // case-sensitive lookup succeeded - if (s != null || !ignoreCase) - { - return s; - } + if (!ignoreCase) + return null; // Try doing a case-insensitive lookup obj = GetCaseInsensitiveObjectInternal(name); - try - { - return (string?)obj; - } - catch (InvalidCastException) - { - throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name)); - } + if (obj is string si) + return si; + + if (obj is null) + return null; + + throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name)); } // Look up an object value for a resource given its name. @@ -208,13 +185,15 @@ private IDictionaryEnumerator GetEnumeratorHelper() protected virtual void ReadResources() { - Debug.Assert(Table != null); + Debug.Assert(_table != null); Debug.Assert(Reader != null); IDictionaryEnumerator en = Reader.GetEnumerator(); while (en.MoveNext()) { - object? value = en.Value; - Table.Add(en.Key, value); + if (en.Key is not string key) + continue; + + _table.Add(key, en.Value); } // While technically possible to close the Reader here, don't close it // to help with some WinRes lifetime issues. @@ -225,35 +204,31 @@ protected virtual void ReadResources() if (name == null) throw new ArgumentNullException(nameof(name)); - Hashtable? copyOfTable = Table; // Avoid a race with Dispose + Dictionary? copyOfTable = _table; // Avoid a race with Dispose if (copyOfTable == null) throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); - return copyOfTable[name]; + copyOfTable.TryGetValue(name, out object? value); + return value; } private object? GetCaseInsensitiveObjectInternal(string name) { - Hashtable? copyOfTable = Table; // Avoid a race with Dispose + Dictionary? copyOfTable = _table; // Avoid a race with Dispose if (copyOfTable == null) throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); - Hashtable? caseTable = _caseInsensitiveTable; // Avoid a race condition with Close + Dictionary? caseTable = _caseInsensitiveTable; // Avoid a race condition with Close if (caseTable == null) { - caseTable = new Hashtable(StringComparer.OrdinalIgnoreCase); - - IDictionaryEnumerator en = copyOfTable.GetEnumerator(); - while (en.MoveNext()) - { - caseTable.Add(en.Key, en.Value); - } + caseTable = new Dictionary(copyOfTable, StringComparer.OrdinalIgnoreCase); _caseInsensitiveTable = caseTable; } - return caseTable[name]; + caseTable.TryGetValue(name, out object? value); + return value; } } } diff --git a/src/libraries/System.Resources.ResourceManager/tests/ResourceSetTests.cs b/src/libraries/System.Resources.ResourceManager/tests/ResourceSetTests.cs index 2975326fe46191..57837b922498bf 100644 --- a/src/libraries/System.Resources.ResourceManager/tests/ResourceSetTests.cs +++ b/src/libraries/System.Resources.ResourceManager/tests/ResourceSetTests.cs @@ -5,6 +5,7 @@ using System.IO; using System.Reflection; using System.Globalization; +using System.Collections; using System.Collections.Generic; namespace System.Resources.Tests @@ -101,6 +102,69 @@ public void GetString() } } + public class ResourceSetTests_IResourceReader + { + class SimpleResourceReader : IResourceReader + { + Hashtable data; + public SimpleResourceReader() + { + data = new Hashtable(); + data.Add(1, "invalid"); + data.Add("String", "message"); + data.Add("Int32", 5); + } + + IEnumerator IEnumerable.GetEnumerator() + { + throw new NotSupportedException(); + } + + public IDictionaryEnumerator GetEnumerator() + { + return data.GetEnumerator(); + } + + public void Close() + { + } + + public void Dispose() + { + } + } + + [Fact] + public void Empty_Ctor() + { + Assert.Throws(() => new ResourceSet(null as IResourceReader)); + } + + [Fact] + public void GetObject() + { + var rs = new ResourceSet(new SimpleResourceReader()); + + Assert.Null(rs.GetObject("DoesNotExist")); + Assert.Null(rs.GetObject("1")); + + Assert.Equal(5, rs.GetObject("Int32")); + Assert.Equal(5, rs.GetObject("int32", true)); + } + + [Fact] + public void GetString() + { + var rs = new ResourceSet(new SimpleResourceReader()); + + Assert.Null(rs.GetString("DoesNotExist")); + Assert.Null(rs.GetString("1")); + + Assert.Equal("message", rs.GetString("String")); + Assert.Equal("message", rs.GetString("string", true)); + } + } + public class ResourceSetTests_StreamCtor : ResourceSetTests { public override ResourceSet GetSet(string base64Data) From a50326fc77b156d3a8455c8632b9740eb4db17ca Mon Sep 17 00:00:00 2001 From: Marek Safar Date: Wed, 4 Nov 2020 23:49:25 +0100 Subject: [PATCH 2/2] Use object type for key types --- .../src/System/Resources/ResourceSet.cs | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs index 5e920be3e6257f..6136c51f6e5757 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs @@ -20,14 +20,14 @@ public class ResourceSet : IDisposable, IEnumerable { protected IResourceReader Reader = null!; - private Dictionary? _table; + private Dictionary? _table; private Dictionary? _caseInsensitiveTable; // For case-insensitive lookups. protected ResourceSet() { // To not inconvenience people subclassing us, we should allocate a new // hashtable here just so that Table is set to something. - _table = new Dictionary(); + _table = new Dictionary(); } // For RuntimeResourceSet, ignore the Table parameter - it's a wasted @@ -190,10 +190,7 @@ protected virtual void ReadResources() IDictionaryEnumerator en = Reader.GetEnumerator(); while (en.MoveNext()) { - if (en.Key is not string key) - continue; - - _table.Add(key, en.Value); + _table.Add(en.Key, en.Value); } // While technically possible to close the Reader here, don't close it // to help with some WinRes lifetime issues. @@ -204,7 +201,7 @@ protected virtual void ReadResources() if (name == null) throw new ArgumentNullException(nameof(name)); - Dictionary? copyOfTable = _table; // Avoid a race with Dispose + Dictionary? copyOfTable = _table; // Avoid a race with Dispose if (copyOfTable == null) throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); @@ -215,7 +212,7 @@ protected virtual void ReadResources() private object? GetCaseInsensitiveObjectInternal(string name) { - Dictionary? copyOfTable = _table; // Avoid a race with Dispose + Dictionary? copyOfTable = _table; // Avoid a race with Dispose if (copyOfTable == null) throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); @@ -223,7 +220,14 @@ protected virtual void ReadResources() Dictionary? caseTable = _caseInsensitiveTable; // Avoid a race condition with Close if (caseTable == null) { - caseTable = new Dictionary(copyOfTable, StringComparer.OrdinalIgnoreCase); + caseTable = new Dictionary(copyOfTable.Count, StringComparer.OrdinalIgnoreCase); + foreach (var item in copyOfTable) + { + if (item.Key is not string s) + continue; + + caseTable.Add(s, item.Value); + } _caseInsensitiveTable = caseTable; }