Skip to content
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.

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

Remove most LINQ usage from Microsoft.Extensions.Configuration #44825

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

stephentoub
Copy link
Member

Resulting in several hundred Func, IEnumerable, set, string, and other allocations at ASP.NET startup.

Related to #44598

Resulting in several hundred Func, IEnumerable, set, and string allocations at startup.
@ghost
Copy link

ghost commented Nov 17, 2020

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details
Description:

Resulting in several hundred Func, IEnumerable, set, string, and other allocations at ASP.NET startup.

Related to #44598

Author: stephentoub
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@stephentoub stephentoub added this to the 6.0.0 milestone Nov 17, 2020
@stephentoub stephentoub added the tenet-performance Performance related issue label Nov 17, 2020
@stephentoub stephentoub merged commit 57c14a2 into dotnet:master Nov 18, 2020
@stephentoub stephentoub deleted the configurationlinq branch November 18, 2020 17:13
@sfiruch
Copy link
Contributor

sfiruch commented Nov 30, 2020

Personally, I feel this is the wrong way around. Improving LINQ to objects instead (#4813) would be an alternative with more long-term benefits.

@stephentoub
Copy link
Member Author

Improving LINQ to objects instead (#4813) would be an alternative with more long-term benefits.

That issue is requesting the JIT to rewrite LINQ queries. I don't expect that will ever be an active area of investment. I also disagree with the suggestion that such transforms should be done by the JIT: if it's something desirable, it should be done by the language-level compiler or a build-time IL-to-IL transform.
cc: @jkotas

@sfiruch
Copy link
Contributor

sfiruch commented Nov 30, 2020

Hm... I think that issue is not asking for JIT rewriting? Linked in this issue are two alternative approaches (LinqOptimizer and the Stream Fusion paper) which perform the transformations at compile-time.

There are many performance-related tickets open that suggest to replace LINQ with something else. That's also the reason why Roslyn doesn't use LINQ. Similar tickets exist even in my own code bases. I was hoping that instead of fixing all the call-sites, fixing LINQ to Objects performance "once" might be worth the effort.

@jkotas
Copy link
Member

jkotas commented Nov 30, 2020

dotnet/roslyn#15929 also talks about this.

fixing LINQ to Objects performance "once" might be worth the effort.

Yep, that's the usual tradeoff between long-term and short-term investments. Building good robust Linq optimizer is large undertaking. There are no resources for this large undertaking right now, so we end up with removing Linq usage as necessary instead that is cheap to do.

@stephentoub
Copy link
Member Author

Hm... I think that issue is not asking for JIT rewriting?

My comment was based on the first sentence of the issue, which states "The following issue was filed twice at Roslyn, dotnet/roslyn#7580 and dotnet/roslyn#275 where @gafter suggested that this optimization should be done by the JIT rather than by the C# compiler."

That's also the reason why Roslyn doesn't use LINQ.

FWIW, it does. dotnet/roslyn-analyzers use a ton of LINQ. And even in dotnet/roslyn, using System.Linq; currently shows up over four thousand times, of which more than 1700 are in product rather than test code.

@sfiruch
Copy link
Contributor

sfiruch commented Nov 30, 2020

Hm... I think that issue is not asking for JIT rewriting?

My comment was based on the first sentence of the issue, which states "The following issue was filed twice at Roslyn, dotnet/roslyn#7580 and dotnet/roslyn#275 where @gafter suggested that this optimization should be done by the JIT rather than by the C# compiler."

I think this issue and comment ended up there just because the compiler and runtime teams played hot potato with the issue (😀), not for technical reasons, as far as I can tell. In the end most users won't care where/how the problem is solved.

That's also the reason why Roslyn doesn't use LINQ.

FWIW, it does. dotnet/roslyn-analyzers use a ton of LINQ. And even in dotnet/roslyn, using System.Linq; currently shows up over four thousand times, of which more than 1700 are in product rather than test code.

Yes, it uses LINQ, but it's discouraged in performance-sensitive paths. Quoting https://github.com/dotnet/roslyn/blob/master/CONTRIBUTING.md:

DO avoid allocations in compiler hot paths:

  • Avoid LINQ.

@stephentoub
Copy link
Member Author

it's discouraged in performance-sensitive paths

Yes. And for the same reason it's discouraged to add new such code, PRs like this one are removing such code where it's impactful.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants