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

Source Generator Cancellation #13433

Merged
merged 2 commits into from
Feb 21, 2023
Merged

Source Generator Cancellation #13433

merged 2 commits into from
Feb 21, 2023

Conversation

mgoertz-msft
Copy link
Contributor

Description of Change

Added ThrowIfCancellationRequested from several strategic points within the source generator.

Issues Fixed

MAUI source generator runs a lot and for a long time and without paying attention to any cancellation requests that can lead to thread pool starvation.

Fixes AB#1741294: ThreadPool starvation due to source generators running on multiple threads

@mgoertz-msft
Copy link
Contributor Author

FYI, I separated out the cancellation changes from the rest of the optimizations I'm working on upon request (see linked bug).

@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formating your source code to follow our code guidelines.

StephaneDelcroix
StephaneDelcroix approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

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

I'm really interested by the results of this changes (numbers). Please share if you can collect them

@Redth Redth merged commit 9459133 into main Feb 21, 2023
@Redth Redth deleted the dev/mgoertz/cancel branch February 21, 2023 15:04
@mgoertz-msft
Copy link
Contributor Author

I'm really interested by the results of this changes (numbers). Please share if you can collect them

@StephaneDelcroix I would be interested too. During my debugging I only ran into one cancellation. Most of the time it was running without being cancelled. So while this should fix the thread starvation issue the overall perf problem still remains. I was already working on that part too but I was asked to split out the cancellation issue to reduce the risk so it can be backported to servicing.

@Redth what's the process for servicing?

@Redth Redth added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Feb 21, 2023
@Redth
Copy link
Member

Redth commented Feb 21, 2023

@mgoertz-msft I added the label which will cause this PR to show up in our next weekly MAUI call to review backport suggestions.

jasonmalinowski
jasonmalinowski reviewed Feb 21, 2023
View reviewed changes
src/Controls/src/SourceGen/CodeBehindGenerator.cs
@@ -293,7 +300,7 @@ static bool GetXamlCompilationProcessingInstruction(XmlDocument xmlDoc)
return false;
}

static IEnumerable<(string name, string type, string accessModifier)> GetNamedFields(XmlNode root, XmlNamespaceManager nsmgr, Compilation compilation, IList<XmlnsDefinitionAttribute> xmlnsDefinitionCache)
static IEnumerable<(string name, string type, string accessModifier)> GetNamedFields(XmlNode root, XmlNamespaceManager nsmgr, Compilation compilation, IList<XmlnsDefinitionAttribute> xmlnsDefinitionCache, CancellationToken cancellationToken)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to also get the cancellation token into GetTypeName, since I think that was the hotspot here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no loop inside of GetTypeName so the check in the GetNamedFields loop will take care of it.

Copy link
Member

Choose a reason for hiding this comment

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

The loop was here if I recall:

maui/src/Controls/src/SourceGen/CodeBehindGenerator.cs

Line 365 in 9459133

string typeName = xmlType.GetTypeReference<string>(xmlnsDefinitionCache, null,

Since that calls into:

maui/src/Controls/src/Xaml/XmlTypeXamlExtensions.cs

Lines 80 to 89 in 9459133

foreach (string typeName in lookupNames)
foreach (XmlnsDefinitionAttribute xmlnsDefinitionAttribute in lookupAssemblies)
potentialTypes.Add(new(typeName, xmlnsDefinitionAttribute.ClrNamespace, xmlnsDefinitionAttribute.AssemblyName));
T? type = null;
foreach (var typeInfo in potentialTypes)
if ((type = refFromTypeInfo(typeInfo)) != null)
break;
return type;

Which had that nested foreach loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That nested foreach isn't actually doing that much, it just creates a list of potential full type names, which rather small. The rest gets called a lot more often. This was just about cancelling at strategic places. I'm working on another change which should address the real perf issues in this generator.

tj-devel709 pushed a commit to tj-devel709/maui that referenced this pull request Feb 21, 2023
@mgoertz-msft
Source Generator Cancellation (dotnet#13433)
8ca35f2 
* ThrowIfCancellationRequested from several strategic points within the source generator.

* Auto-format source code

---------

Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com>
tj-devel709 pushed a commit to tj-devel709/maui that referenced this pull request Feb 21, 2023
@mgoertz-msft
Source Generator Cancellation (dotnet#13433)
a3054de 
* ThrowIfCancellationRequested from several strategic points within the source generator.

* Auto-format source code

---------

Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com>
@hartez
Copy link
Contributor

hartez commented Feb 23, 2023

/backport to net7.0

@github-actions
Copy link
Contributor

Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/4254271920

@github-actions github-actions bot mentioned this pull request Feb 23, 2023
Merged
@hartez hartez added the backport/approved After some discussion or review, this PR or change was approved to be backported. label Feb 23, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

@Redth Redth Redth approved these changes

@StephaneDelcroix StephaneDelcroix StephaneDelcroix approved these changes

Assignees
No one assigned
Labels
backport/approved After some discussion or review, this PR or change was approved to be backported. backport/suggested The PR author or issue review has suggested that the change should be backported.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mgoertz-msft @Redth @hartez @jasonmalinowski @StephaneDelcroix