There is a performance issue with WeatherForecastController.cs

[yangzhongke]

Is there an existing issue for this?

• I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

In the templates of ASP.NET Core Web API, the Get() method of 'aspnetcore/src/ProjectTemplates/Web.ProjectTemplates/content/WebApi-CSharp/Controllers/WeatherForecastController.cs ' is as follows:

public IEnumerable<WeatherForecast> Get()
{
	return Enumerable.Range(1, 5).Select(index => new WeatherForecast
	{
		Date = DateOnly.FromDateTime(DateTime.Now.AddDays(index)),
		TemperatureC = Random.Shared.Next(-20, 55),
		Summary = Summaries[Random.Shared.Next(Summaries.Length)]
	})
	.ToArray();
}

The return value of Get() method is of IEnumerable, and the Select() method returns the same type, so there was no need to convert it to an array using ToArray().
Furthermore, it can cause huge memory consumption problem, and I have stated it in a post: https://www.reddit.com/r/aspnetcore/comments/z7jk8r/who_says_net_doesnt_have_gc_tuning_changing_one/

Describe the solution you'd like

I suggest that remove the invocation of 'ToArray()', which could set up a better model for .NET developers.

Additional context

No response

[davidfowl]

This isn't even using a database. It's the least real example you could imagine. This is just a sample to get people started. Hopefully, they run the app and delete the endpoint and don't try to return 150,000 random weather forecasts unpaged from their endpoints 😁

[yangzhongke]

This isn't even using a database. It's the least real example you could imagine. This is just a sample to get people started. Hopefully, they run the app and delete the endpoint and don't try to return 150,000 random weather forecasts unpaged from their endpoints 😁

Thanks for your reply. The official templates should set up a good example for developers, and the redundant 'ToArray()' should be removed. Redundant is redundant. Any misleading usage from officials is intolerable.

[davidfowl]

I think the ToArray() is used here (with a reasonable page size) because typically, you want to materialize the entire payload before serializing it to the response. In the case where there's errors, you want the whole request to fail instead of failing midway during serialization.

Now I don't think that matters as much when all of the is data in memory, but calling this a performance issue is a stretch.

cc @DamianEdwards

[DamianEdwards]

FTR it's 5, not 150000: https://github.com/dotnet/aspnetcore/blob/main/src/ProjectTemplates/Web.ProjectTemplates/content/WebApi-CSharp/Controllers/WeatherForecastController.cs#L115

As @davidfowl points out, this is a statically defined sample endpoint and the .ToArray() can be rationalized and thus isn't redundant.

[yangzhongke]

Sorry, I posted the code for the stress test, and I have edited it.
But if I change 5 to 150,000, the GC problem happens.

Does it matter if an exception occurs while serializing a model to JSON?
An error in serializing the model to JSON is occasional; it is normal that the error does not occur. IMHO, our code should provide better performance for code that runs normally.

Competitors may use this issue to discredit. NET.
A tradeoff is to remind the developer in the comments not to return huge data at once when ToArray() is used.

[davidfowl]

The best performing sample would be one that didn't return any data at all, but that's not as useful. The default controller is 5, if you change it to a large number in any language, you'd get similarly bad results (even in C). There's no performance issue with the template, but there is one with the changes that you made to the template.

Like anything else in computer science, when you change the size of N then you normally have to change the algorithm to make sure that the performance is reasonable. This is no different.

[yangzhongke]

Removing 'ToArray()' is no harm but good. You didn't convince me, but I don't want to insist.
Good day!