8000 Avoid closures due to implementations in local functions · Issue #906 · morelinq/MoreLINQ · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Avoid closures due to implementations in local functions #906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
viceroypenguin opened this issue Nov 26, 2022 · 3 comments · Fixed by #1064
Closed

Avoid closures due to implementations in local functions #906

viceroypenguin opened this issue Nov 26, 2022 · 3 comments · Fixed by #1064
Labels
good first issue Good for newcomers help wanted
Milestone
4.3.0

Comments

@viceroypenguin
Copy link
Contributor

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 atifaziz mentioned this issue Feb 10, 2023
Merged
@atifaziz
Copy link
Member

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 th 8000 e cost. In fact, if the local function is going to be static, then it might as well be a private method again.

@atifaziz atifaziz changed the title Simplify Closures Avoid closures due to implementations in local functions Jun 27, 2023
@JacobSilasBentley JacobSilasBentley mentioned this issue Apr 25, 2024
Merged
@atifaziz atifaziz added this to the 4.3.0 milestone Apr 27, 2024
atifaziz added a commit that referenced this issue May 1, 2024
@JacobSilasBentley @atifaziz
Use static iterator methods to avoid closure
017e477 
This is a squashed merge of PR #1064 that:

- closes #906
- fixes #1065

---------

Co-authored-by: Atif Aziz <code@raboof.com>
@julealgon
Copy link

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
Copy link
Contributor Author
viceroypenguin commented Jul 8, 2024 •
edited
Loading

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers help wanted
Projects
None yet
Milestone
4.3.0
Development

Successfully merging a pull request may close this issue.

Removed closures in favour of private static methods to close issue #906 JacobSilasBentley/MoreLINQ
3 participants
@atifaziz @julealgon @viceroypenguin
0