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-preview7] Filter list of candidate invocations for parsing in binding generator #89266

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jul 20, 2023

Backport of #89226 to release/8.0-preview7

/cc @layomia

Customer Impact

Improves IDE perf when the generator is used. Average time spent in the generator goes from ~2s to ~145ms, using the Roslyn repo as a test. Details in #89226 / https://gist.github.com/layomia/8f51ac107f76853f4f37a6a7c596f67e.

Testing

Existing tests still pass. Manual testing with tracing demonstrates significant performance improvement.

Risk

Invocation processing logic is the same, just filtering based on the names of the input method calls, and avoiding expensive checks (i.e skipping from the list) when there's no match. There might method name aliasing which could cause dropping some calls, but it's probably fair to expect an exact match e.g. .Bind, .Configure, etc. It would be good to get any feedback/issues filed here.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 20, 2023
@ghost
Copy link

ghost commented Jul 20, 2023

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

Issue Details

Backport of #89226 to release/8.0-preview7

/cc @layomia

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-Extensions-Configuration, needs-area-label

Milestone: -

@ericstj ericstj added Servicing-consider Issue for next servicing release review and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 20, 2023
@tarekgh tarekgh added this to the 8.0.0 milestone Jul 20, 2023
@layomia layomia self-assigned this Jul 20, 2023
@ericstj ericstj added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jul 25, 2023
@ericstj
Copy link
Member

ericstj commented Jul 25, 2023

This was approved over mail last week, but it might be too late to merge at this point. Let's check with @mmitche and @carlossanlop on that.

@ericstj
Copy link
Member

ericstj commented Jul 25, 2023

Too late to merge for Preview 7

@ericstj ericstj closed this Jul 25, 2023
@ericstj ericstj removed the Servicing-approved Approved for servicing release label Jul 25, 2023
@carlossanlop
Copy link
Member

carlossanlop commented Jul 25, 2023

Reopening. The CI passed in the previous run before closing, so we can merge it ASAP. We still have runway.

@carlossanlop carlossanlop reopened this Jul 25, 2023
@carlossanlop carlossanlop added the Servicing-approved Approved for servicing release label Jul 25, 2023
@carlossanlop carlossanlop merged commit 65b696c into release/8.0-preview7 Jul 25, 2023
97 of 105 checks passed
@carlossanlop carlossanlop deleted the backport/pr-89226-to-release/8.0-preview7 branch July 25, 2023 18:56
@ghost ghost locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants