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.

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

Optimize the Regex source generator's handling of Compilation objects. #65431

Merged
merged 4 commits into from
Feb 23, 2022
Merged

Optimize the Regex source generator's handling of Compilation objects. #65431

merged 4 commits into from
Feb 23, 2022

Conversation

teo-tsirpanis
Copy link
Contributor

This PR optimizes the Regex source generator's pipeline in a similar way to what was done in #64579.

The Compilation object was mostly removed from the pipeline, with its presence being restricted to an IncrementalValueProvider<bool> that contains whether unsafe code is allowed, which is seamlessly combined to the main pipeline without needing a custom comparer.

The GetRegexTypeToEmit method was merged with the GetSemanticTargetForGeneration method and moved earlier in the pipeline, according to feedback from the linked PR.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 16, 2022
@ghost
Copy link

ghost commented Feb 16, 2022

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

Issue Details

This PR optimizes the Regex source generator's pipeline in a similar way to what was done in #64579.

The Compilation object was mostly removed from the pipeline, with its presence being restricted to an IncrementalValueProvider<bool> that contains whether unsafe code is allowed, which is seamlessly combined to the main pipeline without needing a custom comparer.

The GetRegexTypeToEmit method was merged with the GetSemanticTargetForGeneration method and moved earlier in the pipeline, according to feedback from the linked PR.

Author: teo-tsirpanis
Assignees: -
Labels:

area-System.Text.RegularExpressions
Milestone: -

@teo-tsirpanis teo-tsirpanis changed the title Optimize the Regex source generator's handling of Compilation objects. Optimize the Regex source generator's handling of Compilation objects. Feb 16, 2022
@stephentoub
Copy link
Member

@sharwell, @chsienki, please review.

chsienki
chsienki approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

Thanks @teo-tsirpanis, this looks like a nice improvement! I've written my thoughts on the attribute lookup change.

Will leave it to @stephentoub to decide, but maybe for this PR we should leave the old behavior using GetBestTypeByMetadataName and consider being more permissive in a separate change.

@teo-tsirpanis
Copy link
Contributor Author

PR feedback was addressed; we still use GetBestTypeByMetadataName.

@joperezr joperezr self-assigned this Feb 22, 2022
stephentoub
stephentoub approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks.

@stephentoub stephentoub merged commit 05920e1 into dotnet:main Feb 23, 2022
@teo-tsirpanis teo-tsirpanis deleted the regex-gen-optimize branch February 23, 2022 13:55
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sharwell sharwell sharwell approved these changes

@chsienki chsienki chsienki approved these changes

@stephentoub stephentoub stephentoub approved these changes

Assignees

@joperezr joperezr

Labels
area-System.Text.RegularExpressions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@teo-tsirpanis @stephentoub @sharwell @chsienki @joperezr