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

[release/8.0] Optimize Options Source Gen when no need to run #93481

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 13, 2023

Backport of #93427 to release/8.0

/cc @tarekgh

Customer Impact

The Options source generator was introduced in .NET 8.0 and is activated by default, requiring no user intervention for its use. During design time, the generator is invoked with every keystroke. The modification here aims to prevent the generator from executing when no code generation is required. This enhancement accelerates this scenario and minimizes unnecessary memory allocation, addressing a minor performance regression that we have already observed.

Testing

Successfully passed all regression tests without any failures. We don't currently have a way to test privates for VS test runs and we'll look into improving that once we have the VMR

Risk

There is virtually no risk associated with this change. It is a minor, non-impactful modification that should not disrupt the logic of the source generator.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

@ghost
Copy link

ghost commented Oct 13, 2023

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

Issue Details

Backport of #93427 to release/8.0

/cc @tarekgh

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@tarekgh tarekgh added Servicing-consider Issue for next servicing release review area-Extensions-Options source-generator Indicates an issue with a source generator feature and removed area-Infrastructure-libraries labels Oct 13, 2023
@ghost
Copy link

ghost commented Oct 13, 2023

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

Issue Details

Backport of #93427 to release/8.0

/cc @tarekgh

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

Servicing-consider, area-Extensions-Options, source-generator

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Oct 13, 2023

@artl93 this one is ready for your review.

Copy link
Member

@artl93 artl93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

M2 approved.

@tarekgh - Just a thought - is it possible to assert the optimization worked for this case in a unit test? E.g. context did not end up adding sources, perhaps? (Curious)

@tarekgh
Copy link
Member

tarekgh commented Oct 13, 2023

@artl93 part of my manual testing I did is I confirmed the behavior under the debugger and ensured the generator didn't do anything and exited early when there is no need to generate any code. We have a work planned for 9.0 to address the whole incremental change with this generator and other generators. When we do that, I expect we'll add more granular test cases that should cover the concerned scenarios. Checking added sources is not going to confirm our scenario here because the generator can run fully and still does not generate any code.

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 13, 2023
@carlossanlop
Copy link
Member

Approved by Tactics via email.

@carlossanlop carlossanlop merged commit ae26d3e into release/8.0 Oct 13, 2023
109 of 116 checks passed
@carlossanlop carlossanlop deleted the backport/pr-93427-to-release/8.0 branch October 13, 2023 23:31
@ghost ghost locked as resolved and limited conversation to collaborators Nov 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Options Servicing-approved Approved for servicing release source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants