Fix #3862 - Double-Check locked patterns and consider static or Lazy<T> for Singletons #16216

WinCPP · 2017-02-16T02:57:59Z

This is first of the few PRs for the changes to replace double-check locked patterns with static or Lazy constructs.

…3862

…T> for Singletons

…3862

stephentoub · 2017-02-16T06:34:16Z

src/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/RuntimeBinder.cs

-
-        private static readonly object s_singletonLock = new object();
-        private static volatile RuntimeBinder s_instance;
+        private static Lazy<RuntimeBinder> s_lazyInstance = new Lazy<RuntimeBinder>(() => new RuntimeBinder());


Nit: readonly

stephentoub · 2017-02-16T06:36:50Z

...tem.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

+        private static readonly Lazy<Hashtable> s_propertyCacheLazy = new Lazy<Hashtable>(() => new Hashtable());
+        private static readonly Lazy<Hashtable> s_eventCacheLazy = new Lazy<Hashtable>(() => new Hashtable());
+        private static readonly Lazy<Hashtable> s_attributeCacheLazy = new Lazy<Hashtable>(() => new Hashtable());
+        private static readonly Lazy<Hashtable> s_extendedPropertyCacheLazy = new Lazy<Hashtable>(() => new Hashtable());


I think all of these would be better off using LazyInitializer.EnsureInitialized rather than Lazy.

@stephentoub Thanks. I will change this and use it in similar places...

Just to make sure I understand correctly, LazyInitializer.EnsureInitialized could be preferred over Lazy<T> in all the changes that I am / will be doing? (ref.: this link) Rules to use LazyInitializer.EnsureInitialized could be,

Places where the init code is not expected to throw exception that should be stored for later use, else use Lazy<T>. (Lazy<T>.Value would make it available later on as well).

In case the init code is expensive (in terms of duplicate unused objects that would end up into GC, or changes to static state fields that need synchronization), it would be better to use the overload of LazyInitializer.EnsureInitialized that accepts a lock.

[A side thought: A Property get-accessor would be a typical way to wrap LazyInitializer.EnsureInitialized call supported by a backing field. Rosyln proposals 8364 or others might help reduce the verbosity in this case as well and avoid getting direct access to backing field bypassing the getter.

private static Hashtable s_propertyCache private static Hashtable PropertyCache => LazyInitializer.EnsureInitialized(ref s_propertyCache, () => new Hashtable());
could be reduced to following which would eliminate direct access

private static Hashtable PropertyCache => LazyInitializer.EnsureInitialized(ref field, () => new Hashtable()); where field is the placeholder for auto-generated backing field
... just a thought]

@stephentoub I edited my above comment. Wasn't sure if you would get another notification for the edits... hence this comment to bump the thread :)

Just to make sure I understand correctly, LazyInitializer.EnsureInitialized could be preferred over Lazy in all the changes that I am / will be doing?

At least all of the ones where the object in question is a Hashtable. There's one that's involving a RuntimeBinder, for example, and comments in the ctor for RuntimeBinder suggests it's very expensive to create one, so that might be better as a Lazy<T>. The issue is that constructing a Lazy<T> itself has a bit of cost, e.g. it's at least an allocation, so we have to weigh that against no allocation but an optimistic concurrency approach where in the worst case you could have a bunch of threads all racing to initialize and each end up allocating the very expensive thing.

stephentoub · 2017-02-16T06:43:10Z

src/System.ComponentModel.TypeConverter/src/System/Drawing/ColorConverter.cs

@@ -18,7 +18,13 @@ namespace System.Drawing
    public class ColorConverter : TypeConverter
    {
        private static object s_valuesLock = new object();


Is s_valuesLock used anywhere after your change?

…T> for Singletons Changes to use LazyInitializer.EnsureInitialized instead of Lazy<T> for light weight objects.

stephentoub · 2017-02-18T15:39:06Z

...tem.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

@@ -352,7 +331,7 @@ private static Hashtable GetEditorTable(Type editorBaseType)
                // actually run.
                //
                System.Runtime.CompilerServices.RuntimeHelpers.RunClassConstructor(editorBaseType.TypeHandle);
-                table = s_editorTables[editorBaseType];
+                table = EditorTables[editorBaseType];


None of these (this one or the one below) need to change from s_editorTables to EditorTables: we know it's already been initialized because it was accessed above. LazyInitialized.EnsureInitialized should be very cheap, but it can't be any cheaper than just going to the field directly. (We could even cache it in a local, but for the purposes of this change, I think it's best to minimize what's touched to the minimal necessary.)

@stephentoub Got it! I used a safe programming approach, but true, it is overhead. Will change it.

If you're concerned about it, I'd be OK with it stored into a local on first access and then using the local.

stephentoub · 2017-02-18T15:39:47Z

...tem.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

            if (attrs != null)
            {
                return attrs;
            }

            lock (s_internalSyncObject)
            {
-                attrs = (Attribute[])s_attributeCache[type];
+                attrs = (Attribute[])AttributeCache[type];


stephentoub · 2017-02-18T15:40:07Z

...tem.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

            if (attrs != null)
            {
                return attrs;
            }

            lock (s_internalSyncObject)
            {
-                attrs = (Attribute[])s_attributeCache[member];
+                attrs = (Attribute[])AttributeCache[member];


stephentoub · 2017-02-18T15:40:17Z

...tem.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

            if (events != null)
            {
                return events;
            }

            lock (s_internalSyncObject)
            {
-                events = (EventDescriptor[])s_eventCache[type];
+                events = (EventDescriptor[])EventCache[type];


stephentoub · 2017-02-18T15:40:43Z

...tem.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

            if (extendedProperties == null)
            {
                lock (s_internalSyncObject)
                {
-                    extendedProperties = (ReflectPropertyDescriptor[])s_extendedPropertyCache[providerType];
+                    extendedProperties = (ReflectPropertyDescriptor[])ExtendedPropertyCache[providerType];


stephentoub · 2017-02-18T15:40:51Z

...tem.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

            if (properties != null)
            {
                return properties;
            }

            lock (s_internalSyncObject)
            {
-                properties = (PropertyDescriptor[])s_propertyCache[type];
+                properties = (PropertyDescriptor[])PropertyCache[type];


stephentoub

A few more minor comments, otherwise LGTM. Thanks.

WinCPP · 2017-02-18T16:15:23Z

...tem.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

-                [s_intrinsicNullableKey] = typeof(NullableConverter),
-            };
-        }
+        private static Hashtable EditorTables => LazyInitializer.EnsureInitialized(ref s_editorTables, () => new Hashtable(4));


@stephentoub Sorry for asking too many questions. I wanted to understand how EnsureInitialized works and found its implementation in the coreclr repo. For the above overload, the implementation is at this link.

It uses Volatile.Read to load address of target (s_editorTables in this case). I think the Volatile.Read would introduce memory barrier? The ask in current task is to eliminate memory barriers which are a consequence of volatile fields. But then EnsureInitialized appears to rely on similar strategy involving volatile reads... Which appears to be contradictory to what we are trying to achieve.

Whereas, the Lazy<T>.Value implementation (link) appears to directly return the value once initialised under a lock.

I might be missing something, but I thought I would check with you. Thanks.

The (new) alternative Lazy<T>.Value is here where _state is a volatile member.

The ask in current task is to eliminate memory barriers which are a consequence of volatile fields.

That's not the goal. The goal is to ensure that lazy initialization is done correctly, safely, and consistently, while minimizing the code required to do so.

But then EnsureInitialized appears to rely on similar strategy involving volatile reads

Any thread-safe lazy initialization strategy is going to rely on volatile reads. That Lazy<T> doesn't appear to be reading _boxed via a volatile read technically looks like a bug to me. Now, due to the semantics enforced by the runtime, it's likely going to be made volatile by the runtime anyway, such that the difference wouldn't be noticeable, and even then on x32/64 the hardware enforces a strong memory model such that it further wouldn't be noticeable, but it's still good to do the right thing in the code.

@stephentoub Thanks for the clarification. Actually I got obsessed :( with removal of volatiles due to my interpretation of, or rather being overwhelmed by, the quote in this comment in issue #3862, which I took as necessary for ARM while introducing lazy init. And I got stuck with it... I am good now.

manofstick · 2017-02-19T02:39:20Z

@WinCPP

I doubt it changes any of your thinking but Lazy is (probably) getting a revamp under this PR.

WinCPP · 2017-02-19T19:25:21Z

Thanks @manofstick .

Do we see a conflict here between new Lazy<T> behavior with the core of the issue #3862 vis-a-vis volatile variables / reads?
The implementation (here) of the overload EnsureInitialized(ref T target, Func<T> valueFactory) (MSDN) uses Volatile.Read, which I am unable to understand in context of the current issue and also through implementation point of view. From documentation, I thought, except for the lock based overload of EnsureInitialized (here), the other overloads would allow multiple threads to initialize the object but only one of them would be CompareExchange'd as actual value with init'd objects being left for garbage collection; and hence I wonder why do we require the Volatile.Read.

@stephentoub , @karelz , @omariom, @manofstick Appreciate your inputs. I am not so well versed in the C# compiler internals / optimization and JIT. So thought I would rather seek advice through discussion before doing any more changes.

Thanks in advance.

manofstick · 2017-02-20T04:13:31Z

@WinCPP

Hmm.

I will have to defer to the real experts here. My possibly naive understanding is that volatile fields are expensive to write to on ARM (cache flush which could be 100s cycles?) But reads were relatively cheap (just don't allow the optimiser to do any reordering).

The existing Lazy implementation does a cast from a object wrapper on each read. My belief is that that is more expensive than the volatile read. From testing on Intel architecture it definitely is.

I don't have any ARM machines to play with.

stephentoub · 2017-02-20T13:31:40Z

Do we see a conflict here between new Lazy behavior with the core of the issue #3862 vis-a-vis volatile variables / reads?

No, using a volatile read is correct.

hence I wonder why do we require the Volatile.Read

To ensure read reorderings/introductions/etc. aren't allowed. While the current runtime / hardware likely prevent such things, the ECMA C# spec would allow such reorderings in a way that could cause problems for lazy init, hence why the lazy constructs use it.

jkotas · 2017-02-20T14:58:12Z

possibly naive understanding is that volatile fields are expensive to write to on ARM

They are not that expensive on any recent (last 5 years?) ARM processors. .NET Core on ARM is actually using a lot of implicit memory barriers to guarantee type safety. For example:

class Test
{
    Object _value;

    // _value = value has implicit memory barrier on ARM in the current implementation of .NET Core
    public SetValue(object value) { _value = value; }
}

Assignments of object references have extra overhead anyway because of the extra tracking for GC. The memory barrier makes this overhead a bit higher, but not prohibitively more. The relevant code is here: https://github.com/dotnet/coreclr/blob/master/src/vm/arm/asmhelpers.S#L1242

…T> for Singletons Minor edits to directly access the field instead of property once the latter has been called once resulting into lazy initialization.

…3862

omariom · 2017-02-21T10:01:07Z

@jkotas

They are not that expensive on any recent (last 5 years?) ARM processors.

Would be great if JIT could use ARMv8's ldar and stlr instead of dmbs.

stephentoub · 2017-02-21T17:25:48Z

src/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs

-        private static volatile WeakHashtable s_associationTable;
+
+        // A table of type -> default provider to track DefaultTypeDescriptionProviderAttributes.
+        private static readonly Lazy<Hashtable> s_defaultProvidersLazy = new Lazy<Collections.Hashtable>(() => new Hashtable());


I just noticed that in the original code, s_defaultProviders is initialized at its declaration site, so it's not actually lazy. We shouldn't wrap it in Lazy then. The only thing we should do is change the volatile to readonly and remove the code elsewhere that checks whether it could be null.

Yes it was null-checked here and I gave it more importance than init at declaration. I will revert and make it readonly.

stephentoub · 2017-02-21T17:27:33Z

src/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs


            if (associations == null)
            {
-                lock (s_associationTable)
+                lock (s_associationTableLazy)


This is changing the semantic, in that previously it was locking on the table, now it's locking on the lazy. Is there code elsewhere that was expecting all locking to be done on the table itself? Let's keep it the way it was, e.g. lock (s_associationTableLazy.Value) rather than lock (s_associationTableLazy)

On the lines of s_defaultProviders reversal discussion above, does s_associationTable really require lazy initialisation. The initialisation is just object creation and does not do any heavy code that may cause delays. So how about making it readonly and initialize in-place?

Extending a bit further, do we want to say that lazy initialisation using Lazy<T> or LazyInitializer.EnsureInitialized makes more sense if the initialisation is non-trivial. (E.g. s_ProviderTable and s_ProvideTypeTable) in this same file which I left untouched. s_associationTable could also be treated in same way....?

On the lines of s_defaultProviders reversal discussion above, does s_associationTable really require lazy initialisation. The initialisation is just object creation and does not do any heavy code that may cause delays. So how about making it readonly and initialize in-place?

I would like to avoid changing lazy/prompt semantics in this PR. The original developer likely had a reason for what they did (lazy vs non-lazy), and so as part of this change I would like to keep it that way. If subsequently you'd like to investigate with appropriate performance tests whether it would be worth changing, that'd be fine as a separate, isolated thing to explore.

Ok. I understand the subtlety.

…T> for Singletons Made changes as per review comments to use s_defaultProviders as defined originally and to lock on s_associationTableLazy.Value instead of s_associationTableLazy object to preserve semantics.

…sue-3862

WinCPP · 2017-02-23T19:16:04Z

@karelz, @stephentoub has approved this PR, pending some corrections that I have done just now. I noticed commits are automatically added to existing open PR and so should newer commits (for newly edited files)...? So, I am not sure how to get them into different (multiple small) PRs as we discussed in issue #3862...

stephentoub · 2017-02-23T20:13:50Z

src/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs

-        private static volatile Hashtable s_defaultProviders = new Hashtable(); // A table of type -> default provider to track DefaultTypeDescriptionProviderAttributes.
-        private static volatile WeakHashtable s_associationTable;
+        private static readonly Hashtable s_defaultProviders = new Hashtable(); // A table of type -> default provider to track DefaultTypeDescriptionProviderAttributes.
+        private static readonly Lazy<WeakHashtable> s_associationTableLazy = new Lazy<WeakHashtable>(() => new WeakHashtable());


Last change I think:
Sorry if I mislead you on the last round of reviews, but it looks like WeakHashtable just derives from Hashtable and adds a few small fields... probably better to make this one LazyInitializer instead of a readonly Lazy.

Nope. I'm getting a lot to learn. Thanks for support.

stephentoub

Thanks for working on this. One final comment, then looks good to merge.

karelz · 2017-02-23T20:23:34Z

So, I am not sure how to get them into different (multiple small) PRs as we discussed in issue #3862...

@WinCPP did you try to use different branch for each PR?

tarekgh · 2017-02-24T16:59:23Z

@WinCPP could yo please fix the code according to last comment from @stephentoub so we can merge the changes? Thanks.

WinCPP · 2017-02-24T17:50:58Z

@tarekgh yup working on it. Please give me another hour... I should have changes in.

…3862

…T> for Singletons Made changes as per review comments to use LazyInitializer for s_associationTable.

stephentoub · 2017-02-24T19:40:19Z

Thanks, @WinCPP.

…atic or Lazy<T> for Singletons (dotnet/corefx#16216) * Fix dotnet/corefx#3862 - Double-Check locked patterns and consider static or Lazy<T> for Singletons Commit migrated from dotnet/corefx@3e7fb77

WinCPP added 7 commits February 11, 2017 20:36

Merge remote-tracking branch 'refs/remotes/dotnet/master'

7df27c3

Merge remote-tracking branch 'refs/remotes/dotnet/master'

6787890

Merge remote-tracking branch 'refs/remotes/dotnet/master' into issue-…

63b3492

…3862

Merge remote-tracking branch 'refs/remotes/dotnet/master' into issue-…

fb07102

…3862

Merge remote-tracking branch 'refs/remotes/dotnet/master' into issue-…

9700f34

…3862

Fix #3862 - Double-Check locked patterns and consider static or Lazy<…

26e6c8c

…T> for Singletons

Merge remote-tracking branch 'refs/remotes/origin/master' into issue-…

13e5d73

…3862

dnfclas added the cla-already-signed label Feb 16, 2017

stephentoub reviewed Feb 16, 2017

View reviewed changes

karelz added the area-Meta label Feb 17, 2017

karelz assigned WinCPP, stephentoub and tarekgh Feb 17, 2017

Fix #3862 - Double-Check locked patterns and consider static or Lazy<…

70647d4

…T> for Singletons Changes to use LazyInitializer.EnsureInitialized instead of Lazy<T> for light weight objects.

stephentoub reviewed Feb 18, 2017

View reviewed changes

stephentoub approved these changes Feb 18, 2017

View reviewed changes

WinCPP commented Feb 18, 2017

View reviewed changes

WinCPP changed the title ~~Fix #3862 - Double-Check locked patterns and consider static or Lazy<T> for Singletons~~ Fix #3862 - [Do not merge] Double-Check locked patterns and consider static or Lazy<T> for Singletons Feb 19, 2017

manofstick mentioned this pull request Feb 20, 2017

Alternative System.Lazy implementation dotnet/coreclr#8963

Merged

7 tasks

WinCPP changed the title ~~Fix #3862 - [Do not merge] Double-Check locked patterns and consider static or Lazy<T> for Singletons~~ Fix #3862 - Double-Check locked patterns and consider static or Lazy<T> for Singletons Feb 20, 2017

WinCPP added 2 commits February 20, 2017 20:45

Fix #3862 - Double-Check locked patterns and consider static or Lazy<…

cbba579

…T> for Singletons Minor edits to directly access the field instead of property once the latter has been called once resulting into lazy initialization.

Merge remote-tracking branch 'refs/remotes/dotnet/master' into issue-…

87a2301

…3862

stephentoub reviewed Feb 21, 2017

View reviewed changes

WinCPP and others added 3 commits February 24, 2017 00:15

Merge branch 'master' into issue-3862

f4b2153

Fix #3862 - Double-Check locked patterns and consider static or Lazy<…

403a982

…T> for Singletons Made changes as per review comments to use s_defaultProviders as defined originally and to lock on s_associationTableLazy.Value instead of s_associationTableLazy object to preserve semantics.

Merge branch 'issue-3862' of https://github.com/WinCPP/corefx into is…

41c574c

…sue-3862

stephentoub reviewed Feb 23, 2017

View reviewed changes

stephentoub approved these changes Feb 23, 2017

View reviewed changes

WinCPP added 2 commits February 25, 2017 00:14

Merge remote-tracking branch 'refs/remotes/dotnet/master' into issue-…

9f6e914

…3862

Fix #3862 - Double-Check locked patterns and consider static or Lazy<…

4932e03

…T> for Singletons Made changes as per review comments to use LazyInitializer for s_associationTable.

stephentoub merged commit 3e7fb77 into dotnet:master Feb 24, 2017

karelz modified the milestone: 2.0.0 Mar 1, 2017

Fix #3862 - Double-Check locked patterns and consider static or Lazy<T> for Singletons #16216

Fix #3862 - Double-Check locked patterns and consider static or Lazy<T> for Singletons #16216

Conversation

WinCPP commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WinCPP Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manofstick commented Feb 19, 2017

WinCPP commented Feb 19, 2017 • edited Loading

manofstick commented Feb 20, 2017

stephentoub commented Feb 20, 2017

jkotas commented Feb 20, 2017 • edited by karelz Loading

omariom commented Feb 21, 2017

stephentoub Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WinCPP commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

karelz commented Feb 23, 2017

tarekgh commented Feb 24, 2017

WinCPP commented Feb 24, 2017

stephentoub commented Feb 24, 2017

WinCPP Feb 17, 2017 •

edited

Loading

stephentoub Feb 18, 2017 •

edited

Loading

WinCPP commented Feb 19, 2017 •

edited

Loading

jkotas commented Feb 20, 2017 •

edited by karelz

Loading

stephentoub Feb 21, 2017 •

edited

Loading

stephentoub Feb 21, 2017 •

edited

Loading