RuntimeResourceSet improvements

[marek-safar]

• remove unnecessary base class calls to trim more of the base type
• remove code paths which were never executed
• replaced nested locking with simple locks
• fix caching for case insensitive mode
• add tests for more code paths

[ghost]

Tagging subscribers to this area: @tarekgh, @buyaa-n, @krwq, @jeffhandley
See info in area-owners.md if you want to be subscribed.

---

Issue meta data

Issue content:	- remove unnecessary base class calls to trim more of the base type - remove code paths which were never executed - replaced nested locking with simple locks - fix caching for case insensitive mode - add tests for more code paths
Issue author:	marek-safar
Assignees:	-
Milestone:	-

[tarekgh]

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.

[marek-safar]

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

[tarekgh]

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.

[marek-safar]

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

[tarekgh]

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.

[marek-safar]

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.

[tarekgh]

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.

[marek-safar]

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

[tarekgh]

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;
                    }

[marek-safar]

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

[tarekgh]

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)

[marek-safar]

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.

[tarekgh]

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

[marek-safar]

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