Alternative System.Lazy implementation #8963

manofstick · 2017-01-17T08:56:10Z

2017-21-01: I have modified the implementation a little, so updated the text to reflect the changes
2017-28-01: Updated text to represent current state of implementation

The current version of System.Lazy isn't particularly performant. @mrange had some analysis in an article called On the cost of being lazy.

Ultimately this stemmed back to FSharp's implementations of Seq.init(Infinite)? which wrapped each call to IEnumerator<T>.MoveNext() in a lazy that was only evaluated on Current. (Which is one of the issues that I have address when composing Seqs in my (incomplete) PR Seq Composer - although it still remains in there under some circumstances)

This version:

creates less objects for ExecutionAndPublish (2 objects created, vs 3), None (1 vs 2) & PublicationOnly (1 vs 2)
Faster access after initialization (just a null check, no casting)
Faster creation step - just one virtual call away
Arguably a more straight-forward implementation that is easier to follow as logic is partitioned.
Much less generics exposure, so as to improve JIT compilation

Still to do:

Add and merge your extra tests to for Lazy<T> to corefx repo. (System.Lazy regression tests corefx#15577)
Make sure that all corefx tests for Lazy<T> are passing
Incorporate any feedback/bug fixes
Merge to CoreCLR repo
Port the change to CoreRT repo
External (not @manofstick) validation of threading correctness
Solution for how to deal with Mode on complete item for Debug object (I suggest Nullable, if that is acceptable)

dnfclas · 2017-01-17T08:56:14Z

Hi @manofstick, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas · 2017-01-17T09:02:40Z

@manofstick, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

mrange · 2017-01-17T18:52:46Z

@manofstick - I have updated the charts to include your updated Lazy<'T> class.

https://gist.github.com/mrange/aa82d33e94ad76d0f33ed86e704d7492

mrange · 2017-01-17T18:59:12Z

src/mscorlib/src/System/Lazy.cs

+
+        private static class CreateInstance
+        {
+            private static T construct()


Minor comment: Construct?

mrange · 2017-01-17T19:01:14Z

src/mscorlib/src/System/Lazy.cs

        /// </summary>
-        private static object GetObjectFromMode(LazyThreadSafetyMode mode)
+        /// <param name="factory">The object factory used to create the underlying object</param>
+        /// <param name="forceMemoryBarrier">true when called with ExecutionAndPublication, false


Minor: No parameter named forceMemoryBarrier?

mrange · 2017-01-17T19:05:19Z

src/mscorlib/src/System/Lazy.cs

+        private T CreateValuePublicationOnly(Func<T> factory, PublicationOnly comparand)
+        {
+            var possibleValue = factory();
+            lock (comparand)


Personally I find a bit confusing that the lock for PublicationOnly happens inside the Create* method but for ExecutionAndPublication the caller holds the lock. Is there a reason the lock can't be moved into CreateValueExecutionAndPublication?

Yeah I agree with the asymmetry with the lock, but I need to call TakeFactory to null out the factory object in the case of ExecutionAndPublication. Anyway I have a plan to remove the lock from PublicationOnly, so that should make it better!

mrange · 2017-01-17T19:06:42Z

src/mscorlib/src/System/Lazy.cs

+        /// <returns>The underlying object</returns>
+        private T CreateValuePublicationOnly(Func<T> factory, PublicationOnly comparand)
+        {
+            var possibleValue = factory();


I was hoping that PublicationOnly only could get away for with an Interlock.CompareAndExchange over a full lock. What are the reasons a full lock is needed?

You've shamed me into it! I've thought of a cunning plan... soon, soon...

mrange · 2017-01-17T19:08:55Z

src/mscorlib/src/System/Lazy.cs

-
-                // Fall through to the slow path.
-                return LazyInitValue();
+                var implementation = m_implementation;


Would you think it's worth to add aggressive inlining to the performance critical property getters?

Hmmm... that ones above my pay grade :-) I'm guessing that the JIT is probably inlining the calls to Value anyway? Maybe you could check?

mrange · 2017-01-17T19:10:47Z

src/mscorlib/src/System/Lazy.cs

+            protected Func<T> TakeFactory()
+            {
+                var factory = Factory;
+                if (!ReferenceEquals(CreateInstance.Factory, factory))


Why is this special handling needed?

TakeFactory is required to handle re-enterency. In the case of recursive initialization it throws an exception. Needed to replicate existing functionality.

mrange · 2017-01-17T19:13:46Z

src/mscorlib/src/System/Lazy.cs

-                    m_valueFactory = ALREADY_INVOKED_SENTINEL;
+                    if (factory == null)
+                        throw new InvalidOperationException(Environment.GetResourceString("Lazy_Value_RecursiveCallsToValue"));
+                    Factory = null;


Am I right in that this handling is only correct if only one thread is working or we have execution and publication lock?

that's on the money.

mrange · 2017-01-17T19:14:35Z

src/mscorlib/src/System/Lazy.cs

+
+            public override T Value
+            {
+                get { return Owner.CreateValuePublicationOnly(Factory, this); }


No TakeFactory here I guess because it's not safe for concurrent calls? How is reentrancy detection handled for PublicationOnly

It isn't. It hangs! But same functionality as previous implementation.

mrange · 2017-01-17T19:20:01Z

src/mscorlib/src/System/Lazy.cs

+            var possibleValue = factory();
+            lock (comparand)
+            {
+                if (ReferenceEquals(m_implementation, comparand))


The reason we are comparing against a comparand is that during evaluation of the lazy value we might get an exception? That is captured in an object that is referenced by m_implementation. If we had no need to capture exceptions we could have compared against null?

mrange · 2017-01-17T19:24:12Z

src/mscorlib/src/System/Lazy.cs

-        [NonSerialized]
-        private object m_threadSafeObj;
+        // m_implementation, a volatile reference, is set to null after m_value has been set
+        private volatile ILazyItem m_implementation;


As the current implementation either holds a lock when updating m_implementation or we have no protection ==> Lazy<'T> can't be used concurrently then the only place where volatile matters is in the .ctor to ensure all writes to ILazyItem objects are visible to other threaads when they read it.

Not sure of your comment there? (Or is it a comment that is meant to be a comment? even then I don't think it reads very well?)

The volatile matters in all places where m_implementation is accessed. i.e. it is the gatekeeper to ensure that m_value has a valid value before it is accessed. As reference type writes are atomic, we're guaranteed that we'll either have a valid ILazyItem implementation (from the .ctor) or null, after m_value has been written. (I'm about to make that a little more complex...)

mrange · 2017-01-17T19:25:53Z

src/mscorlib/src/System/Lazy.cs

@@ -50,63 +42,22 @@ internal static class LazyHelpers
    [ComVisible(false)]
    [DebuggerTypeProxy(typeof(System_LazyDebugView<>))]
    [DebuggerDisplay("ThreadSafetyMode={Mode}, IsValueCreated={IsValueCreated}, IsValueFaulted={IsValueFaulted}, Value={ValueForDebugDisplay}")]
-    public class Lazy<T>
+    public class Lazy<T> :


Hi @manofstick, I have checked the code and added a few observations/questions. Overall I see no obvious weakness. You said there was no test-coverage for Lazy<'T> today. Can that be added (damn difficult for these kinds of classes but at least to cover the functionality)?

mrange · 2017-01-17T19:26:39Z

Hi @manofstick, I have checked the code and added a few observations/questions. Overall I see no obvious weakness. You said there was no test-coverage for Lazy<'T> today. Can that be added (damn difficult for these kinds of classes but at least to cover the functionality)?

mrange · 2017-01-17T19:27:58Z

I think the reason the Lazy<'T> implementation above performs worse for PublicationOnly then Lazzzy<'T> is because of the publication only lock.

mrange · 2017-01-17T19:29:26Z

From my performance testing the read overhead of a Lazy<'T> object that is initialized is 3x lower with the new implementation. That is pretty good!

stephentoub · 2017-01-17T19:30:27Z

I've not reviewed these changes yet, but there are functional tests for Lazy; as with most of the types in mscorlib, the full set of tests is in corefx, e.g.
https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System/LazyTests.cs

This involves a little bit of trickery. In avoiding the lock we need to go from m_implementation as PublicationOnly to null in a single step. We can't do this though without another object, so we reuse out Lazy object, which basically just spins its wheels until the value is ready. This shouldn't be a very common scenerio (i.e. hitting that code path) but it is possible.

manofstick · 2017-01-18T08:56:04Z

@mrange

OK; with a bit of trickery I restored the CompareExchange in PublicaitonOnly without having to create a new object. In my testing with lots of threads this performed very well. Can you update your numbers to see if you get the previous performance?

manofstick · 2017-01-18T08:57:32Z

Hiya @stephentoub !

Are you happy for me to dump all of my tests into the existing LazyTest file?

stephentoub · 2017-01-18T12:05:16Z

Are you happy for me to dump all of my tests into the existing LazyTest file?

Extra tests would be excellent, thanks! I'd ask, though, that you de-dup with the existing ones so that we're only adding new tests for behaviors that don't already have them.

Threading bug. TakeFactory() would crash on second call that was banked up behind the lock.

Storing factory in Lazy object, and passing Lazy as a parameter to ILazyItem functions. This means that None & PublicationOnly now need no creation of secondary object.

manofstick · 2017-01-21T09:03:40Z

@mrange sorry dude, I've modified things again, but I think I'm now at absolute minimum. I think None and PublicationOnly should show some improvements, as they no longer create any other objects other than the Lazy object itself (and as per previous comment, PublicationOnly's use of CompareExchange was restored)

@stephentoub hopefully I'll get a bit of chance to review tests tests within the next week (and hopefully no world war will begin within this time... hmmm...) Anyway, I think I'm pretty happy with the whole thing now, I've minimized object creation and all paths seem pretty optimal. I do hope serialization doesn't prove the show stopper...

jkotas · 2017-01-21T14:20:01Z

Before this optimization, there are 3 generic types that will be created per T: Lazy<T>, Boxed<T>, LazyInternalExceptionHolder<T>. The last one does not actually have to be generic - it can be moved out to LazyHelpers.

After this throughput optimization, there are 9 generic types that will be created per T: Lazy<T>, ILazyItem, CreateInstance<T>, LazyException, None, ExecutionAndPublication, PublicationOnly, PublicationOnlyWaiter, Boxed. And they are more expensive than the earlier ones because of they tend to implement interfaces.

The generic types take time and memory to create too, and if T is valuetype there is code JITed per T too. It would interesting to look at whether the ~5x+ increase in startup cost and footprint per T pays off in typical programs. For example, if I run https://github.com/aspnet/MusicStore app, how many different Lazy types are there and how many times is each one created?

manofstick · 2017-01-21T19:51:41Z

Hi @jkotas

I must admit I am ignorant in the ways of the JIT. but is it really creating all those types, of are they just potentially created (dare I say it, lazily created)? And if they are created eagerly is that just because they are inner classes, in which case they could be moved out and just the each other with internal scoped members?

I.e. following the happy path of creation would be, 'Lazy', 'ILazyItem', 'ExecutionAndPublication' and that's it. So still just 3. That would be most uses.

You should need 4 for 'PublicationOnly' and 3 for ’None'.

In the case of serialisation of exceptions an extra one each is required, but they are heavy operations anyway.

Anyway, let the knowledgeable people speak, I'll move back to my corner of ignorance!

jkotas · 2017-01-22T00:52:20Z

because they are inner classes

Inner class relationship does not matter for performance. What matters is how the implementation is interconnected. For example, execution of private T CreateValue(LazyThreadSafetyMode mode) will load LazyException<T> type too because of it is referenced within the method. It is even worse problem with AOT compilation because of the AOT compilers cannot typically tell which types referenced in the implementation are the useful ones and so they have to compile all of them. You end up paying for it in the AOT binary size and compilation time.

The idea about making the value a direct field and use auxiliary state object is good, ie this is the best part of the change:

     public T Value
     {
            get
            {
                var implementation = m_implementation;
                if (implementation == null)
                    return m_value;

However the number of extra generic plumbing types makes it hard to tell whether it is obvious good trade-off for programs out there. Could you please try to refactor the implementation to avoid the generic types, and use non-generic types instead? The existing implementation is not perfect in this regard either - you should be able to beat it on the startup performance (=number of generic types) as well. Feel free to bring up any issues you may run into.

I have created a small perf test that creates a lot of different Lazy instantiations that you can use to measure: https://gist.github.com/jkotas/9c492e62c59cb5e7220daeed587d254a . On my machines, it gives 3953ms without your changes, 5734ms with your current changes. You can compile it directly against corelib for convenience:

cd \coreclr\bin\Product\Windows_NT.x64.Release
csc /noconfig /nostdlib /r:System.Private.CoreLib.dll /o+ test.cs)

Thank you for your work on this so far!

Fixing startup performance concerns

manofstick · 2017-01-22T09:32:56Z

Howdy @jkotas,

OK, well after a little playing around I have found that the main performance issue that raised its head in regards to your concerns was not so much the creation of generics, but rather that a Func<T> was being created for the default constructor factory.

Anyway, I have made some modification to remove this, and the numbers appear a fair bit better to me. I ran some tests with 32 & 64-bit and class MyG<T, U> and struct MyG<T, U> (where I cut the depth by 1) and the new version wins some and loses some.

Let me know what you think.

jkotas · 2017-01-22T17:09:42Z

CreateInstance<T> and Func<T>

the new version wins some and loses some

Could you please still share some data about where it wins and loses?

I still believe it would be beneficial to make all the plumbing types non-generic. I think you may need to add extra cast from Object to Func<T> to make that work, but that should cost next to nothing.

manofstick · 2017-01-22T18:48:27Z

@jkotas

I don't think it's as simple as that. Maybe with a completely different implementation, but here I need to be able to call back into the 'Lazy' object from the ILazyItem implementation functions so they therefore need to be generic.

Anyway, I'll have a think. Not sure if I'll get much time to play this week, but I'll try to do a quick run to print timings from your testb at some stage.

jkotas · 2017-02-22T18:24:55Z

src/mscorlib/src/System/Lazy.cs

            }
        }

        /// <summary>
        /// Gets a value indicating whether this instance may be used concurrently from multiple threads.
        /// </summary>
-        internal LazyThreadSafetyMode Mode
+        internal LazyThreadSafetyMode? Mode


Nit: All these forwarders can be written as one-liners:

internal LazyThreadSafetyMode? Mode => LazyHelper.GetMode(_state);
internal bool IsValueFaulted => LazyHelper.GetIsValueFaulted(_state);
public bool IsValueCreated => _state == null;

Race condition was introduced when state wasn't passed to LazyGetValue. Also just made LazyGetValue into void CreateValue, as it was internally recursively calling back to Value, so I think this is easier to grok threading implications (i.e. why it wasn't just returning _value.)

manofstick · 2017-03-01T08:48:07Z

@jkotas / @stephentoub

OK, well I think that things are good to progress.

There had been a race condition introduced when I have removed the state from being passed to LazyGetValue, but I believe we're all good again now.

I'm getting "GitHub pull request #8963 of commit 77d3a7f, has merge conflicts." so I think that's why the build is failing? Anyway, all over to you guys.

jkotas · 2017-03-01T15:04:21Z

@manofstick Could you please merge current CoreCLR master and resolve the conflicts?

jkotas · 2017-03-01T15:10:39Z

src/mscorlib/src/System/Lazy.cs

@@ -322,147 +528,16 @@ public T Value
        {
            get
            {
-                Boxed boxed = null;
-                if (m_boxed != null )
+                while (true)


Having a while loop here will make the Value property to be a bad inlining candidate - bad for steady state perf. Could you please change CreateValue to return the value instead so that it can be return (_state == null) ? _value : CreateValue();

Or it can be - without changing CreateValue to return the value:

if (_state != null) CreateValue(); return _value;

This one is slightly better because it has smaller IL, and thus it will make it better inlining candidate.

Can't just return _value without check if _state (volatile semantics). I'm not sure I can guarantee that it would always be valid (although I'm sure it would be in tests, but that's not the same thing is it 😀) I'll return it to how it was.

jkotas · 2017-03-03T23:06:56Z

src/mscorlib/src/System/Lazy.cs

@@ -18,6 +18,7 @@
 using System.Runtime;
 using System.Runtime.InteropServices;
 using System.Security;
+using System.Security.Permissions;


You should not need to add using System.Security.Permissions back

@jkotas

Yeah, sorry about that I'm bashing around trying to control git (I'm used to mercurial at work, and I must say, beyond rudimentary commands git drives me insane!)

Anyway, I'll get there...

jkotas · 2017-03-04T03:14:45Z

Very well done - thanks a lot!

I will take care of porting this to CoreRT repo for you.

manofstick · 2017-03-04T08:44:00Z

Sorry @jkotas I just noticed that in my shameful use of git merging I splatted over your previous task which removed the ComVisible attribute. Can you just remove it; or do you want me to create a new PR?

jkotas · 2017-03-04T08:51:35Z

Create a new PR for it please

jkotas · 2017-03-04T08:53:09Z

I have just moved Lazy.cs to a different directory, so make sure to sync to latest master before creating the PR.

manofstick · 2017-03-04T09:03:28Z

@jkotas do you never sleep/watch a movie/go to pub/etc :-) :-)

Anyway, I'll do it tomorrow. Have a good weekend!

jkotas · 2017-03-05T05:43:25Z

I went to the pub twice this week, and watched iMax movie today :-)

Thanks for submitting the PR.

manofstick · 2017-03-05T08:50:03Z

@jkotas obviously you're just superhuman then! :-) No worries for PR; anyway, I can pack up my bags and return to the F# playground, I've left it rotting for a couple of months with an alternative linq-to-objects (F#'s Seq module) implementation. I just need to be a superhuman too to find a few weeks of time...

* Remove use of lock for PublicationOnly This involves a little bit of trickery. In avoiding the lock we need to go from m_implementation as PublicationOnly to null in a single step. We can't do this though without another object, so we reuse out Lazy object, which basically just spins its wheels until the value is ready. This shouldn't be a very common scenerio (i.e. hitting that code path) but it is possible. * Minimize additional object creation Storing factory in Lazy object, and passing Lazy as a parameter to ILazyItem functions. This means that None & PublicationOnly now need no creation of secondary object. * Remove Func<T> for default constructor invoking Fixing startup performance concerns * Moved non-generic functionality out of Lazy ...and into a helper class * Expression-bodied functions

* System.Lazy regression tests This is a prelude to an alternative Lazy implementation in a PR at dotnet/coreclr#8963 Commit migrated from dotnet/corefx@029f4c4

* Remove use of lock for PublicationOnly This involves a little bit of trickery. In avoiding the lock we need to go from m_implementation as PublicationOnly to null in a single step. We can't do this though without another object, so we reuse out Lazy object, which basically just spins its wheels until the value is ready. This shouldn't be a very common scenerio (i.e. hitting that code path) but it is possible. * Minimize additional object creation Storing factory in Lazy object, and passing Lazy as a parameter to ILazyItem functions. This means that None & PublicationOnly now need no creation of secondary object. * Remove Func<T> for default constructor invoking Fixing startup performance concerns * Moved non-generic functionality out of Lazy ...and into a helper class * Expression-bodied functions Commit migrated from dotnet/coreclr@2453adf

Alternative System.Lazy implementation

528e0e9

dnfclas added the cla-required label Jan 17, 2017

dnfclas added cla-signed and removed cla-required labels Jan 17, 2017

mrange reviewed Jan 17, 2017

View reviewed changes

src/mscorlib/src/System/Lazy.cs Outdated

private static class CreateInstance

{

private static T construct()

Copy link

mrange Jan 17, 2017

Choose a reason for hiding this comment

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

Minor comment: Construct?

mrange reviewed Jan 17, 2017

View reviewed changes

manofstick added 2 commits January 18, 2017 19:19

Minor naming fixes

e88a0a5

manofstick added 2 commits January 19, 2017 16:47

Need to check m_implementation before TakeFactory

38c739c

Threading bug. TakeFactory() would crash on second call that was banked up behind the lock.

Minimize additional object creation

c7c9dc3

Storing factory in Lazy object, and passing Lazy as a parameter to ILazyItem functions. This means that None & PublicationOnly now need no creation of secondary object.

Remove Func<T> for default constructor invoking

8c855c1

Fixing startup performance concerns

jkotas reviewed Feb 22, 2017

View reviewed changes

manofstick mentioned this pull request Feb 28, 2017

[WIP] Seq Composer dotnet/fsharp#1570

Closed

99 tasks

manofstick added 5 commits March 1, 2017 18:50

Tighter code around invalid code paths

00531f2

Added descriptive comment for LazyHelper

dbfa2f8

Expression-bodied functions

00e949f

Fix compilation errors due to negligence...

77d3a7f

jkotas reviewed Mar 1, 2017

View reviewed changes

Merge remote-tracking branch 'dotnet/master' into manofstick-lazy

fb03886

jkotas reviewed Mar 3, 2017

View reviewed changes

Removed looping from Value to enable(?) inlining

de2a4a7

jkotas merged commit 2453adf into dotnet:master Mar 4, 2017

jkotas mentioned this pull request Mar 4, 2017

Alternative System.Lazy implementation dotnet/corert#2900

Merged

manofstick mentioned this pull request Mar 5, 2017

Re-Remove ComVisible On Lazy as per #9371 #9962

Merged

manofstick deleted the manofstick-lazy branch March 5, 2017 02:39

karelz modified the milestone: 2.0.0 Aug 28, 2017

manofstick mentioned this pull request May 8, 2018

[WIP] [CompilerPerf] Seq - the next generation. dotnet/fsharp#2745

Closed

11 tasks

Alternative System.Lazy implementation #8963

Alternative System.Lazy implementation #8963

Conversation

manofstick commented Jan 17, 2017 • edited Loading

dnfclas commented Jan 17, 2017

dnfclas commented Jan 17, 2017

mrange commented Jan 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

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

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

mrange commented Jan 17, 2017

mrange commented Jan 17, 2017

mrange commented Jan 17, 2017

stephentoub commented Jan 17, 2017

manofstick commented Jan 18, 2017

manofstick commented Jan 18, 2017

stephentoub commented Jan 18, 2017

manofstick commented Jan 21, 2017

jkotas commented Jan 21, 2017 • edited Loading

manofstick commented Jan 21, 2017

jkotas commented Jan 22, 2017

manofstick commented Jan 22, 2017

jkotas commented Jan 22, 2017 • edited Loading

manofstick commented Jan 22, 2017

Choose a reason for hiding this comment

manofstick commented Mar 1, 2017

jkotas commented Mar 1, 2017

Choose a reason for hiding this comment

jkotas Mar 1, 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

jkotas commented Mar 4, 2017

manofstick commented Mar 4, 2017

jkotas commented Mar 4, 2017

jkotas commented Mar 4, 2017

manofstick commented Mar 4, 2017

jkotas commented Mar 5, 2017

manofstick commented Mar 5, 2017

manofstick commented Jan 17, 2017 •

edited

Loading

mrange commented Jan 17, 2017 •

edited

Loading

jkotas commented Jan 21, 2017 •

edited

Loading

jkotas commented Jan 22, 2017 •

edited

Loading

jkotas Mar 1, 2017 •

edited

Loading