Avoid closures due to implementations in local functions

[viceroypenguin]

Currently, most operators are implemented as such:

public static IEnumerator<TSource> Operator<TSource>(this IEnumerable<TSource> source, ...)
{
   // guard statements
   return _();

   _()
   {
      yield return source.First();
      yield break;
   }
}

A better implementation would be the following:

public static IEnumerator<TSource> Operator<TSource>(this IEnumerable<TSource> source, ...)
{
   // guard statements
   return _(source, ...);

   static _(IEnumerable<TSource> source, ...)
   {
      yield return source.First();
      yield break;
   }
}

With the current implementation, the host method must create a closure class before even doing guard statements, copy parameters, and then call a method on that closure class. This means that there are two closure classes: one for the parameters, and one for the enumerator.

Switching to passing the parameters into the iterator method means that only one closure class has to be created, the one for the enumerator; the parameters will be held by that class. This should simplify and improve the IL, the JIT, and the output JIT-ted code.

[atifaziz]

Thanks for bringing this up and shame about the closure because it means reverting all of #290. The local functions really made things pleasant (for all the reasons listed in #290), but I agree that inducing a closure is not worth the cost. In fact, if the local function is going to be static, then it might as well be a private method again.

[julealgon]

if the local function is going to be static, then it might as well be a private method again.

@atifaziz the function being local has other benefits which are significant, such as being scoped only to that function instead of being callable by the entire class.

I don't think you should make them private methods again for that reason.

@viceroypenguin do you mind if I ask a related question here? Would this also apply to an async method that is split due to validation concerns like this?

public Task DoStuffAsync(string argument, CancellationToken cancellationToken)
{
    ArgumentNullException.ThrowIfNull(argument);
    return DoStuffAsync();

    async Task DoStuffAsync()
    {
        await SomethingElseAsync(cancellationToken);
    }
}

Would it have similar benefits here to make the local function static if possible?

[viceroypenguin]

@julealgon Yes, it does make a difference for async methods. See two comparisons here:

• closure: https://sharplab.io/#v2:D4AQTAjAsAUCAMACEEB0AVAFgJwKYEMATASwDsBzAblgWQgFZqYaBmZMRAYUQG9ZEByNiABsiACIB7AMoAXAK4AzRQEEAzgE9SAYwAUKJPmzl5AW1ylZAGi74duADYP8s4pNLpJAawuJtd7UdnV3dPH1IASn5BPhhBeMQVYzMLWQA5eScAUQAPQIAHENIMHEkAdwBJRQynXSMTc0sIpgTBEAB2CRkFZXUtPWbYaNaQAA5kEQAeAwA+LrklVU0dXSi41t5hjYEQAE4J1HFHfA1denh4QfXtnc76lMsWjYBfLdfmGGegA=
• argument: https://sharplab.io/#v2:D4AQTAjAsAUCAMACEEB0AVAFgJwKYEMATASwDsBzAblgWQgFZqYaBmZMRAYUQG9ZEByNiABsiACIB7AMoAXAK4AzRQEEAzgE9SAYwAUKJPmzl5AW1ylZAGi74duADYP8s4pNLpJAawuJtd7UdnV3dPH1IASn5BPhhBeMQVYzMLWQA5eScAUQAPQIAHENIMHEkAdwBJRQynXSMTc0sIpgTBEAB2CRkFZXUtPXqUpqZo1pAADmQRAB4DAD4uuSVVTR19CENkxtkouNbeUf2BEABOKdRxR3wNXXp4eGbDo47EQe2W/YBfQ+/mGE+gA=

Specifically, the closure version will create a new class and allocate an instance of it for use within the local method. However, the argument version will only create a struct and use it locally.

Note that for this reason, SuperLinq.Async does follow the same pattern to avoid closures.