Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RuntimeResourceSet improvements #44454

Merged
merged 4 commits into from
Nov 13, 2020
Merged

RuntimeResourceSet improvements #44454

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8599128
RuntimeResourceSet improvements
marek-safar Nov 10, 2020
fa39c8f
Move local caseInsensitiveTable initialisation
marek-safar Nov 10, 2020
4cc71bf
Clean up unused code
marek-safar Nov 12, 2020
c262d20
Revert "Clean up unused code"
marek-safar Nov 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 5 additions & 15 deletions src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,16 @@ namespace System.Resources
// default file format.
//

internal struct ResourceLocator
internal readonly struct ResourceLocator
{
internal object? _value; // Can be null.
internal int _dataPos;

internal ResourceLocator(int dataPos, object? value)
{
_dataPos = dataPos;
_value = value;
DataPosition = dataPos;
Value = value;
}

internal int DataPosition => _dataPos;

// Allows adding in profiling data in a future version, or a special
// resource profiling build. We could also use WeakReference.
internal object? Value
{
get => _value;
set => _value = value;
}
internal int DataPosition { get; }
internal object? Value { get; }

internal static bool CanCache(ResourceTypeCode value)
{
Expand Down
282 changes: 94 additions & 188 deletions src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

/*============================================================
**
**
**
**
**
** Purpose: CultureInfo-specific collection of resources.
**
**
===========================================================*/

#nullable enable
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
Expand Down Expand Up @@ -178,7 +166,7 @@ sealed class RuntimeResourceSet : ResourceSet, IEnumerable
private Dictionary<string, ResourceLocator>? _resCache;


// For our special load-on-demand reader, cache the cast. The
// For our special load-on-demand reader. The
// RuntimeResourceSet's implementation knows how to treat this reader specially.
private ResourceReader? _defaultReader;

Expand All @@ -189,32 +177,19 @@ sealed class RuntimeResourceSet : ResourceSet, IEnumerable
// don't exist.
private Dictionary<string, ResourceLocator>? _caseInsensitiveTable;

// If we're not using our custom reader, then enumerate through all
// the resources once, adding them into the table.
private bool _haveReadFromReader;

#if !RESOURCES_EXTENSIONS
internal RuntimeResourceSet(string fileName) : base(false)
internal RuntimeResourceSet(string fileName) :
this(new FileStream(fileName, FileMode.Open, FileAccess.Read, FileShare.Read))
{
_resCache = new Dictionary<string, ResourceLocator>(FastResourceComparer.Default);
Stream stream = new FileStream(fileName, FileMode.Open, FileAccess.Read, FileShare.Read);
_defaultReader = new ResourceReader(stream, _resCache, false);
Reader = _defaultReader;
}

internal RuntimeResourceSet(Stream stream, bool permitDeserialization = false) : base(false)
internal RuntimeResourceSet(Stream stream, bool permitDeserialization = false) :
base(false)
{
_resCache = new Dictionary<string, ResourceLocator>(FastResourceComparer.Default);
_defaultReader = new ResourceReader(stream, _resCache, permitDeserialization);
Reader = _defaultReader;
}
#else
private
#if NETFRAMEWORK
new
#endif
IResourceReader Reader => _defaultReader!;

internal RuntimeResourceSet(IResourceReader reader) :
// explicitly do not call IResourceReader constructor since it caches all resources
// the purpose of RuntimeResourceSet is to lazily load and cache.
Expand All @@ -235,32 +210,18 @@ internal RuntimeResourceSet(IResourceReader reader) :

protected override void Dispose(bool disposing)
{
if (Reader == null)
if (_defaultReader is null)
return;

if (disposing)
{
lock (Reader)
{
_resCache = null;
if (_defaultReader != null)
{
_defaultReader.Close();
_defaultReader = null;
}
_caseInsensitiveTable = null;
// Set Reader to null to avoid a race in GetObject.
base.Dispose(disposing);
}
}
else
{
// Just to make sure we always clear these fields in the future...
_resCache = null;
_caseInsensitiveTable = null;
_defaultReader = null;
base.Dispose(disposing);
_defaultReader?.Close();
}

_defaultReader = null;
_resCache = null;
_caseInsensitiveTable = null;
base.Dispose(disposing);
}

public override IDictionaryEnumerator GetEnumerator()
Expand All @@ -275,14 +236,13 @@ IEnumerator IEnumerable.GetEnumerator()

private IDictionaryEnumerator GetEnumeratorHelper()
{
IResourceReader copyOfReader = Reader;
if (copyOfReader == null || _resCache == null)
ResourceReader? reader = _defaultReader;
if (reader is null)
throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet);

return copyOfReader.GetEnumerator();
return reader.GetEnumerator();
}


public override string? GetString(string key)
{
object? o = GetObject(key, false, true);
Expand All @@ -309,158 +269,104 @@ private IDictionaryEnumerator GetEnumeratorHelper()
{
if (key == null)
throw new ArgumentNullException(nameof(key));
if (Reader == null || _resCache == null)

ResourceReader? reader = _defaultReader;
Dictionary<string, ResourceLocator>? cache = _resCache;
Dictionary<string, ResourceLocator>? caseInsensitiveTable = _caseInsensitiveTable;
if (reader is null || cache is null)
throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet);
marek-safar marked this conversation as resolved.
Show resolved Hide resolved

object? value = null;
ResourceLocator resLocation;
object? value;
ResourceLocator resEntry;

lock (Reader)
lock (cache)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to get some perf numbers after changing this locking. The scope of locking with the cache is now changed and I don't know how much this can effect the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? The lock before locked about 120 lines whereas now it locks about 20 lines with early exits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I means changing the lock from the reader to the cache is changing the old locking scope even if we are locking later. I guess you can have multiple different readers pointing at the same cache. The old lock using the cache later was executing very small block and the code doesn't have always reach this lock. now you are locking with cache which can affect any readers using this cache and all readers will be blocked. I am just trying to ensure we are not regressing anything here including the performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm could be missing something here but it does not matter which instance is used, all readers will be blocked if one enters the lock. If you are referring to this lock

runtime/src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs

Line 358 in e7204f5

lock (_resCache)
then that's a nested lock which would be entered without outer lock

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm could be missing something here but it does not matter which instance is used, all readers will be blocked if one enters the lock. If you are referring to this lock

That is my point, before your changes not all readers will be blocked. only the used reader instance will be blocked. now after your change, all readers sharing such cache will be blocked I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before your changes not all readers will be blocked.

I'm still confused by what you mean by that. There was huge lock across the whole method which would block all concurrent readers (

runtime/src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs

Line 318 in e7204f5

lock (Reader)
). The fact there was nested lock made it only worse not better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is, we have the following constructor.

internal RuntimeResourceSet(IResourceReader reader)

Which take the reader as input. That means you can create multiple resource sets with the same reader if needed. Before your change, the lock was taken on the reader which means it can synchronize between all instances created with the same reader. After your change, the synchronization would happen using the cache which is created per one resource set object. That is the difference I am trying to point at. I don't know exactly how this can affect the behavior or the perf.

The other side point, do you know if we ever compile with #if RESOURCES_EXTENSIONS? I am asking because this constructor is used only under this define. And I couldn't find who is using such constructor at all. I assume it was there for a good reason but I have no idea where or when it get used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually noticed that code is not needed anymore. Cleaned it up which should resolve your questions as well

{
if (Reader == null)
throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet);

if (_defaultReader != null)
// Find the offset within the data section
int dataPos;
if (cache.TryGetValue(key, out resEntry))
{
// Find the offset within the data section
int dataPos = -1;
if (_resCache.TryGetValue(key, out resLocation))
{
value = resLocation.Value;
dataPos = resLocation.DataPosition;
}
value = resEntry.Value;
if (value != null)
return value;

if (dataPos == -1 && value == null)
{
dataPos = _defaultReader.FindPosForResource(key);
}
// When data type cannot be cached
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true? I am seeing the old code was still doing the following in case dataPos != -1

                    resLocation = new ResourceLocator(dataPos, (ResourceLocator.CanCache(typeCode)) ? value : null);
                    lock (_resCache)
                    {
                        _resCache[key] = resLocation;
                    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which only re-insert the same resLocation with no reason because value is null

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but the comment would be confusing as it is already cached with null value.

Also, I assume dataPos will not equal -1 at that time. is that right? do we need to check for that? or assert at least?

In reply to: 520911963 [](ancestors = 520911963)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the old code was reinserting values which couldn't be cached into cache over again. What would you assert for, if FindPosForResource returns the negative value the is code which handles that already and the value is never inserted into the cache.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assert I meant is dataPos != -1. I guess we shouldn't have as -1 in here. right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, -1 won't ever be inserted into the cache. See https://github.com/marek-safar/runtime/blob/res/src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs#L297

dataPos = resEntry.DataPosition;
return isString ? reader.LoadString(dataPos) : reader.LoadObject(dataPos, out _);
}

if (dataPos != -1 && value == null)
{
Debug.Assert(dataPos >= 0, "data section offset cannot be negative!");
// Normally calling LoadString or LoadObject requires
// taking a lock. Note that in this case, we took a
// lock on the entire RuntimeResourceSet, which is
// sufficient since we never pass this ResourceReader
// to anyone else.
ResourceTypeCode typeCode;
if (isString)
{
value = _defaultReader.LoadString(dataPos);
typeCode = ResourceTypeCode.String;
}
else
{
value = _defaultReader.LoadObject(dataPos, out typeCode);
}

resLocation = new ResourceLocator(dataPos, (ResourceLocator.CanCache(typeCode)) ? value : null);
lock (_resCache)
{
_resCache[key] = resLocation;
}
}
dataPos = reader.FindPosForResource(key);
if (dataPos >= 0)
{
value = ReadValue(reader, dataPos, isString, out resEntry);
cache[key] = resEntry;
return value;
}
}

if (value != null || !ignoreCase)
{
return value; // may be null
}
} // if (_defaultReader != null)
if (!ignoreCase)
{
return null;
}

// At this point, we either don't have our default resource reader
// or we haven't found the particular resource we're looking for
// and may have to search for it in a case-insensitive way.
if (!_haveReadFromReader)
{
// If necessary, init our case insensitive hash table.
if (ignoreCase)
{
_caseInsensitiveTable ??= new Dictionary<string, ResourceLocator>(StringComparer.OrdinalIgnoreCase);
}
// We haven't found the particular resource we're looking for
// and may have to search for it in a case-insensitive way.
bool initialize = false;
if (caseInsensitiveTable == null)
{
caseInsensitiveTable = new Dictionary<string, ResourceLocator>(StringComparer.OrdinalIgnoreCase);
initialize = true;
}

if (_defaultReader == null)
{
IDictionaryEnumerator en = Reader.GetEnumerator();
while (en.MoveNext())
{
DictionaryEntry entry = en.Entry;
string readKey = (string)entry.Key;
ResourceLocator resLoc = new ResourceLocator(-1, entry.Value);
_resCache.Add(readKey, resLoc);
if (ignoreCase)
{
Debug.Assert(_caseInsensitiveTable != null);
_caseInsensitiveTable.Add(readKey, resLoc);
}
}
// Only close the reader if it is NOT our default one,
// since we need it around to resolve ResourceLocators.
if (!ignoreCase)
Reader.Close();
}
else
{
Debug.Assert(ignoreCase, "This should only happen for case-insensitive lookups");
Debug.Assert(_caseInsensitiveTable != null);
ResourceReader.ResourceEnumerator en = _defaultReader.GetEnumeratorInternal();
while (en.MoveNext())
{
// Note: Always ask for the resource key before the data position.
string currentKey = (string)en.Key;
int dataPos = en.DataPosition;
ResourceLocator resLoc = new ResourceLocator(dataPos, null);
_caseInsensitiveTable.Add(currentKey, resLoc);
}
}
_haveReadFromReader = true;
}
object? obj = null;
bool found = false;
bool keyInWrongCase = false;
if (_defaultReader != null)
{
if (_resCache.TryGetValue(key, out resLocation))
{
found = true;
obj = ResolveResourceLocator(resLocation, key, _resCache, keyInWrongCase);
}
}
if (!found && ignoreCase)
lock (caseInsensitiveTable)
{
if (initialize)
{
Debug.Assert(_caseInsensitiveTable != null);
if (_caseInsensitiveTable.TryGetValue(key, out resLocation))
ResourceReader.ResourceEnumerator en = reader.GetEnumeratorInternal();
while (en.MoveNext())
{
found = true;
keyInWrongCase = true;
obj = ResolveResourceLocator(resLocation, key, _resCache, keyInWrongCase);
// The resource key must be read before the data position.
string currentKey = (string)en.Key;
ResourceLocator resLoc = new ResourceLocator(en.DataPosition, null);
caseInsensitiveTable.Add(currentKey, resLoc);
}

_caseInsensitiveTable = caseInsensitiveTable;
}
return obj;
} // lock(Reader)

if (!caseInsensitiveTable.TryGetValue(key, out resEntry))
return null;

if (resEntry.Value != null)
return resEntry.Value;

value = ReadValue(reader, resEntry.DataPosition, isString, out resEntry);

if (resEntry.Value != null)
caseInsensitiveTable[key] = resEntry;
}

return value;
}

// The last parameter indicates whether the lookup required a
// case-insensitive lookup to succeed, indicating we shouldn't add
// the ResourceLocation to our case-sensitive cache.
private object? ResolveResourceLocator(ResourceLocator resLocation, string key, Dictionary<string, ResourceLocator> copyOfCache, bool keyInWrongCase)
private static object? ReadValue (ResourceReader reader, int dataPos, bool isString, out ResourceLocator locator)
{
// We need to explicitly resolve loosely linked manifest
// resources, and we need to resolve ResourceLocators with null objects.
object? value = resLocation.Value;
if (value == null)
object? value;
ResourceTypeCode typeCode;

// Normally calling LoadString or LoadObject requires
// taking a lock. Note that in this case, we took a
// lock before calling this method.
if (isString)
{
ResourceTypeCode typeCode;
lock (Reader)
{
Debug.Assert(_defaultReader != null);
value = _defaultReader.LoadObject(resLocation.DataPosition, out typeCode);
}
if (!keyInWrongCase && ResourceLocator.CanCache(typeCode))
{
resLocation.Value = value;
copyOfCache[key] = resLocation;
}
value = reader.LoadString(dataPos);
typeCode = ResourceTypeCode.String;
}
else
{
value = reader.LoadObject(dataPos, out typeCode);
}

locator = new ResourceLocator(dataPos, ResourceLocator.CanCache(typeCode) ? value : null);
return value;
}
}
Expand Down
Loading