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/7.0-preview7] Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for LibraryImportGenerator). #72142

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

joperezr
Copy link
Member

Backport of #71652 into release/7.0-preview7

Description

Having the LibraryImportGenerator use the new Polyfill Roslyn API for fast Attribute-based source generators performance.

Customer Impact

This will help with editor experience for all developers, regardless if they use the LibraryImportGenerator or not. This generator is on by default, and runs on projects even when the consuming project doesn't consume the generator. These changes help boost the performance for all scenarios: when the source generator is consumed, when it is not used, and also makes the incremental editing experience better and faster as this generator spends much fewer CPU cycles.

Testing

Regular unit tests plus manual validation.

Risk

Low-Medium: This generator is on by default, so it runs for all applications which is why the change is deemed to be low-medium risk. The changes have been manually validated in main, and it is a perf-only change so it shouldn't alter behavior.

cc: @CyrusNajmabadi @jkoritzinsky @stephentoub

…roslyn (for LibraryImportGenerator). (dotnet#71652)

Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
@ghost
Copy link

ghost commented Jul 13, 2022

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

Issue Details

Backport of #71652 into release/7.0-preview7

Description

Having the LibraryImportGenerator use the new Polyfill Roslyn API for fast Attribute-based source generators performance.

Customer Impact

This will help with editor experience for all developers, regardless if they use the LibraryImportGenerator or not. This generator is on by default, and runs on projects even when the consuming project doesn't consume the generator. These changes help boost the performance for all scenarios: when the source generator is consumed, when it is not used, and also makes the incremental editing experience better and faster as this generator spends much fewer CPU cycles.

Testing

Regular unit tests plus manual validation.

Risk

Low-Medium: This generator is on by default, so it runs for all applications which is why the change is deemed to be low-medium risk. The changes have been manually validated in main, and it is a perf-only change so it shouldn't alter behavior.

cc: @CyrusNajmabadi @jkoritzinsky @stephentoub

Author: joperezr
Assignees: -
Labels:

area-System.Runtime.InteropServices

Milestone: -

@joperezr
Copy link
Member Author

cc: @ericstj @danmoseley

@joperezr joperezr added the Servicing-consider Issue for next servicing release review label Jul 13, 2022
@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jul 13, 2022
@carlossanlop
Copy link
Member

Approved by Tactics via email.
Will merge when the CI is done.
Who can sign off?

@CyrusNajmabadi
Copy link
Member

@stephentoub signed off on the original, so it would make sense on this PR as well IMO :)

@joperezr
Copy link
Member Author

yup, I added both @stephentoub and @jkoritzinsky who both signed off on the original

@danmoseley
Copy link
Member

Work Item baseservices.threading failed - timed out after 1 hr with no real log. @dotnet/dnceng can you tell whether this was Helix? If the test ran but hung I'd expect to see more than 1 line here.
https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-72142-merge-f27b61a3834946e092/baseservices.threading/1/console.7c4fb640.log?helixlogtype=result

clearly unrelated to this change.

@carlossanlop carlossanlop merged commit 664c43f into dotnet:release/7.0-preview7 Jul 14, 2022
@MattGal
Copy link
Member

MattGal commented Jul 14, 2022

Work Item baseservices.threading failed - timed out after 1 hr with no real log. @dotnet/danmeng can you tell whether this was Helix? If the test ran but hung I'd expect to see more than 1 line here. https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-72142-merge-f27b61a3834946e092/baseservices.threading/1/console.7c4fb640.log?helixlogtype=result

clearly unrelated to this change.

Sure, we'll take a look.

@MattGal
Copy link
Member

MattGal commented Jul 14, 2022

The work item exited with STATUS_DLL_INIT_FAILED, a classic problem with these ARM64 machines . I've offline-d the machine and will create an FR issue to reimage it. We've (relatively recently) reimaged all these machines and it made the problem much better, but it never really goes away.

Some historical examples:
https://github.com/dotnet/core-eng/issues/11777
https://github.com/dotnet/core-eng/issues/15112
https://github.com/dotnet/core-eng/issues/15818

My theory here is that basically, if you let ARM64 windows grind long enough on certain hardware, some system files get corrupted. Updating to a newer OS seems to have made it less prevalent but not gone.

@danmoseley
Copy link
Member

STATUS_DLL_INIT_FAILED

Is there a line that could go into the console log in that or a similar case ?

@MattGal
Copy link
Member

MattGal commented Jul 14, 2022

@danmoseley I was able to investigate the machine in trouble and it it does seem that skipping this week's rollout may literally be why this started; the machine had > 7 days of uptime, and as soon as I rebooted it the problem went away.

I filed dotnet/arcade#10013 to improve logging and cause this reboot in the cases where it happens.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants