Use forattributename jsgenerator #78136
Conversation
|
Tagging subscribers to this area: @dotnet/interop-contrib Issue DetailsUpdates the JSImportGenerator and JSExportGenerator to use
|
AaronRobinsonMSFT
added
area-System.Runtime.InteropServices.JavaScript
and removed
area-System.Runtime.InteropServices
labels
|
(if you want to backport it, the PR targeting |
| static modelData => modelData is not null); | ||
| .ForAttributeWithMetadataName(Constants.JSExportAttribute, | ||
| static (node, ct) => node is MethodDeclarationSyntax, | ||
| static (context, ct) => new { Syntax = (MethodDeclarationSyntax)context.TargetNode, Symbol = (IMethodSymbol)context.TargetSymbol }); |
There was a problem hiding this comment.
You are pushing symbols down in the pipeline, which affects incrementality.
Fixing it may require more extensive refactorings which I don't know if it can be backported, but I think for this PR that will go to 8, we should do it the right way.
There was a problem hiding this comment.
Agree with the symbols issue, but this was being done before. This PR doesn't change that, just makes the existing logic more efficient.
There was a problem hiding this comment.
This generator (along with the rest of the interop generators) use symbols for a few steps until we are able to build a model that does not contain symbols at a named step "CalculateStubInformation". From that point onwards, the generators are incremental. We can't do incrementality earlier as we need to get the symbols as well as some additional information from the Compilation.
| static modelData => modelData is not null); | ||
| .ForAttributeWithMetadataName(Constants.JSImportAttribute, | ||
| static (node, ct) => node is MethodDeclarationSyntax, | ||
| static (context, ct) => new { Syntax = (MethodDeclarationSyntax)context.TargetNode, Symbol = (IMethodSymbol)context.TargetSymbol }); |
|
We are seeing some heavy memory use from before this change show up in some VS perf traces. We might want to backport this change to 7 and 6. cc @CyrusNajmabadi.
|
|
Did this generator ship before net8? Or was it just added in net8? If so, we may just be behind wrt picking up this fix in Roslyn. |
|
It shipped in .NET 7. |
|
@jkoritzinsky thanks. Can we backport toys fix to 7.0? It's fairly brutal :-) |
|
We should be able to. I'll try the backport bot on this and follow up on Monday to get it through servicing. |
|
/backport to release/7.0-staging |
|
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4706135407 |
|
@jkoritzinsky backporting to release/7.0-staging failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Use ForAttributeWithMetadataName instead of CreateSyntaxProvider
Applying: Remove unused code
error: sha1 information is lacking or useless (src/libraries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Remove unused code
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@jkoritzinsky an error occurred while backporting to release/7.0-staging, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
|
I'll manually backport this on Monday. |
|
Thanks! |
|
Backport PR at #84936 |
* Use ForAttributeWithMetadataName instead of CreateSyntaxProvider * Remove unused code * Update export generator too
ghost
locked as resolved and limited conversation to collaborators
Updates the JSImportGenerator and JSExportGenerator to use
ForAttributeWithMetadataName(instead of directly creating a syntax provider) which improves performance.