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;

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

Thanks, DNFBOT;

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

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

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)?

I think the reason theLazy<'T> implementation above performs worse forPublicationOnly thenLazzzy<'T> is because of the publication only lock.

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

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

@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?

Hiya@stephentoub !

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

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.

@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...

Before this optimization, there are 3 generic types that will be created perT: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 perT: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 runhttps://github.com/aspnet/MusicStore app, how many different Lazy types are there and how many times is each one created?

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!

because they are inner classes

Inner class relationship does not matter for performance. What matters is how the implementation is interconnected. For example, execution ofprivate T CreateValue(LazyThreadSafetyMode mode) will loadLazyException<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.Releasecsc /noconfig /nostdlib /r:System.Private.CoreLib.dll /o+ test.cs)

Thank you for your work on this so far!

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 aFunc<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 andclass MyG<T, U> andstruct 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.

CreateInstance<T> andFunc<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 fromObject toFunc<T> to make that work, but that should cost next to nothing.

@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 /@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 commit77d3a7f, has merge conflicts." so I think that's why the build is failing? Anyway, all over to you guys.

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

Very well done - thanks a lot!

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

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?

Create a new PR for it please

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

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

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

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

Thanks for submitting the PR.

@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#'sSeq module) implementation. I just need to be a superhuman too to find a few weeks of time...