search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.96k stars 4.65k forks source link

Improve `LoggerMessageGenerator` incrementality #76119

Open Youssef1313 opened 1 year ago

Youssef1313 commented 1 year ago

Description

https://github.com/dotnet/runtime/blob/985eedd68df0b4fb3f541fe266c95fa0a1bc4a0a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Roslyn4.0.cs#L34-L35

I think having the Compilation in the generator pipeline will cause it to fully run on every change, so this is losing the benefits of being incremental.

cc @333fred

Reproduction Steps

-

Expected behavior

Generator pipeline shouldn't have a Compilation or ISymbols, etc.

Actual behavior

-

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-extensions-logging See info in area-owners.md if you want to be subscribed.

Issue Details
### Description https://github.com/dotnet/runtime/blob/985eedd68df0b4fb3f541fe266c95fa0a1bc4a0a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Roslyn4.0.cs#L34-L35 I think having the Compilation in the generator pipeline will cause it to fully run on every change, so this is losing the benefits of being incremental. cc @333fred ### Reproduction Steps - ### Expected behavior Generator pipeline shouldn't have a `Compilation` or `ISymbol`s, etc. ### Actual behavior - ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information _No response_
Author: Youssef1313
Assignees: -
Labels: `untriaged`, `area-Extensions-Logging`
Milestone: -
eerhardt commented 1 year ago

Note that the exact same line exists in the System.Text.Json source generator:

https://github.com/dotnet/runtime/blob/985eedd68df0b4fb3f541fe266c95fa0a1bc4a0a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn4.0.cs#L39-L40

So if a change is needed here - it should be made in both places.

cc @CyrusNajmabadi @joperezr

Youssef1313 commented 1 year ago

Yeah, I think a change is needed to JsonSourceGenerator too.

ericstj commented 1 month ago

Related https://github.com/dotnet/runtime/issues/93309

We discussed improving the incremental characteristics of the runtime source generators and scoped it out of 9.0