Skip to content

Use forattributename jsgenerator - #78136

Merged
chsienki merged 3 commits into
dotnet:mainfrom
chsienki:use_forattributename_jsgenerator
Nov 17, 2022
Merged

Use forattributename jsgenerator#78136
chsienki merged 3 commits into
dotnet:mainfrom
chsienki:use_forattributename_jsgenerator

Conversation

@chsienki

@chsienkichsienki commented Nov 10, 2022

Copy link
Copy Markdown
Member

Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.

@ghostghost assigned chsienkiNov 10, 2022
@ghost

ghost commented Nov 10, 2022

Copy link
Copy Markdown

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

Issue Details

Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.

Author:chsienki
Assignees:-
Labels:

area-System.Runtime.InteropServices

Milestone:-

@AaronRobinsonMSFTAaronRobinsonMSFT added this to the 7.0.x milestone Nov 10, 2022
@build-analysisbuild-analysisBot mentioned this pull request Nov 10, 2022
Closed
@teo-tsirpanisteo-tsirpanis modified the milestones: 7.0.x, 8.0.0Nov 10, 2022
@teo-tsirpanis

teo-tsirpanis commented Nov 10, 2022

Copy link
Copy Markdown
Contributor

(if you want to backport it, the PR targeting release/7.0 should be in the 7.0.x milestone)

teo-tsirpanis
teo-tsirpanis suggested changes Nov 10, 2022
View reviewed changes
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSExportGenerator.cs
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 });

@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@chsienkichsienkiNov 10, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Agree with the symbols issue, but this was being done before. This PR doesn't change that, just makes the existing logic more efficient.

@jkoritzinskyjkoritzinskyNov 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs
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 });

@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same.

@build-analysisbuild-analysisBot mentioned this pull request Nov 11, 2022
Closed
@chsienki
chsienki merged commit 4acfa64 into dotnet:mainNov 17, 2022
@ghostghost locked as resolved and limited conversation to collaborators Dec 17, 2022
@RikkiGibson

RikkiGibson commented Apr 15, 2023

Copy link
Copy Markdown
Member

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.

image
image

@CyrusNajmabadi

CyrusNajmabadi commented Apr 15, 2023

Copy link
Copy Markdown
Contributor

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.

@jkoritzinsky

jkoritzinsky commented Apr 15, 2023

Copy link
Copy Markdown
Member

It shipped in .NET 7.

@CyrusNajmabadi

CyrusNajmabadi commented Apr 15, 2023

Copy link
Copy Markdown
Contributor

@jkoritzinsky thanks. Can we backport toys fix to 7.0? It's fairly brutal :-)

@jkoritzinsky

jkoritzinsky commented Apr 15, 2023

Copy link
Copy Markdown
Member

We should be able to. I'll try the backport bot on this and follow up on Monday to get it through servicing.

@github-actionsgithub-actionsBot unlocked this conversation Apr 15, 2023
@jkoritzinsky

jkoritzinsky commented Apr 15, 2023

Copy link
Copy Markdown
Member

/backport to release/7.0-staging

@github-actions

github-actionsBot commented Apr 15, 2023

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4706135407

@dotnetdotnet deleted a comment from github-actionsBotApr 15, 2023
@github-actions

github-actionsBot commented Apr 15, 2023

Copy link
Copy Markdown
Contributor

@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 128

Please backport manually!

@github-actions

github-actionsBot commented Apr 15, 2023

Copy link
Copy Markdown
Contributor

@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.

@jkoritzinsky

jkoritzinsky commented Apr 15, 2023

Copy link
Copy Markdown
Member

I'll manually backport this on Monday.

@CyrusNajmabadi

CyrusNajmabadi commented Apr 15, 2023

Copy link
Copy Markdown
Contributor

Thanks!

@jkoritzinskyjkoritzinsky mentioned this pull request Apr 17, 2023
Merged
@jkoritzinsky

jkoritzinsky commented Apr 17, 2023

Copy link
Copy Markdown
Member

Backport PR at #84936

jkoritzinsky pushed a commit to jkoritzinsky/runtime that referenced this pull request Apr 17, 2023
@chsienki@jkoritzinsky
Use forattributename jsgenerator (dotnet#78136)
5cf9e91
* Use ForAttributeWithMetadataName instead of CreateSyntaxProvider
* Remove unused code
* Update export generator too
jkoritzinsky added a commit that referenced this pull request Apr 21, 2023
@jkoritzinsky@chsienki
Backport#78136to .NET 7 (#84936)
6e3c5c7
Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
@ghostghost locked as resolved and limited conversation to collaborators May 17, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@pavelsavarapavelsavarapavelsavara approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner
+1 more reviewer
@teo-tsirpanisteo-tsirpanisteo-tsirpanis requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@chsienkichsienki

Labels

area-System.Runtime.InteropServices.JavaScript

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@chsienki@teo-tsirpanis@RikkiGibson@CyrusNajmabadi@jkoritzinsky@pavelsavara@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Use forattributename jsgenerator by chsienki · Pull Request #78136 · dotnet/runtime · GitHub
Skip to content
Use forattributename jsgenerator - #78136
Merged
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Nov 17, 2022
Merged
Use forattributename jsgenerator#78136
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Conversation
@chsienki
@chsienkichsienki
commented
Nov 10, 2022

Copy link


Copy Markdown

Member
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
@ghostghost
assigned chsienkiNov 10, 2022
@ghost
ghost
commented
Nov 10, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/interop-contrib

See info in area-owners.md if you want to be subscribed.
Issue Details
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
Author:chsienki
Assignees:-
Labels:
area-System.Runtime.InteropServices
Milestone:-
@AaronRobinsonMSFTAaronRobinsonMSFT
added this to the 7.0.x milestone
Nov 10, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 10, 2022

Closed

@teo-tsirpanisteo-tsirpanis
modified the milestones:
7.0.x, 8.0.0Nov 10, 2022
@teo-tsirpanis
teo-tsirpanis
commented
Nov 10, 2022

Copy link


Copy Markdown

Contributor
(if you want to backport it, the PR targeting release/7.0 should be in the 7.0.x milestone)
teo-tsirpanis
teo-tsirpanis
suggested changes
Nov 10, 2022
View reviewed changes
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSExportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

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.
@chsienkichsienkiNov 10, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agree with the symbols issue, but this was being done before. This PR doesn't change that, just makes the existing logic more efficient.
@jkoritzinskyjkoritzinskyNov 10, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Same.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 11, 2022

Closed

@chsienki
chsienki
merged commit 4acfa64
into
dotnet:mainNov 17, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 17, 2022
@RikkiGibson
RikkiGibson
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
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.
image
image
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
It shipped in .NET 7.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@jkoritzinsky thanks. Can we backport toys fix to 7.0? It's fairly brutal :-)
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
We should be able to. I'll try the backport bot on this and follow up on Monday to get it through servicing.
@github-actionsgithub-actionsBot
unlocked this conversation
Apr 15, 2023
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4706135407
@dotnetdotnet
deleted a comment from github-actionsBotApr 15, 2023
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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 128
Please backport manually!
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
I'll manually backport this on Monday.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Thanks!
@jkoritzinskyjkoritzinsky
mentioned this pull request
Apr 17, 2023
 Merged

@jkoritzinsky
jkoritzinsky
commented
Apr 17, 2023

Copy link


Copy Markdown

Member
Backport PR at #84936
jkoritzinsky pushed a commit
to jkoritzinsky/runtime
that referenced
this pull request
Apr 17, 2023
@chsienki@jkoritzinsky
Use forattributename jsgenerator (dotnet#78136)
5cf9e91
* Use ForAttributeWithMetadataName instead of CreateSyntaxProvider
* Remove unused code
* Update export generator too
jkoritzinsky added a commit
that referenced
this pull request
Apr 21, 2023
@jkoritzinsky@chsienki
Backport#78136to .NET 7 (#84936)
6e3c5c7
Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
@ghostghost
locked as resolved and limited conversation to collaborators
May 17, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@pavelsavarapavelsavarapavelsavara approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner

+1 more reviewer

@teo-tsirpanisteo-tsirpanisteo-tsirpanis requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@chsienkichsienki

Labels

area-System.Runtime.InteropServices.JavaScript

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@chsienki@teo-tsirpanis@RikkiGibson@CyrusNajmabadi@jkoritzinsky@pavelsavara@AaronRobinsonMSFT

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Use forattributename jsgenerator by chsienki · Pull Request #78136 · dotnet/runtime · GitHub
Skip to content
Use forattributename jsgenerator - #78136
Merged
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Nov 17, 2022
Merged
Use forattributename jsgenerator#78136
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Conversation
@chsienki
@chsienkichsienki
commented
Nov 10, 2022

Copy link


Copy Markdown

Member
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
@ghostghost
assigned chsienkiNov 10, 2022
@ghost
ghost
commented
Nov 10, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/interop-contrib

See info in area-owners.md if you want to be subscribed.
Issue Details
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
Author:chsienki
Assignees:-
Labels:
area-System.Runtime.InteropServices
Milestone:-
@AaronRobinsonMSFTAaronRobinsonMSFT
added this to the 7.0.x milestone
Nov 10, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 10, 2022

Closed

@teo-tsirpanisteo-tsirpanis
modified the milestones:
7.0.x, 8.0.0Nov 10, 2022
@teo-tsirpanis
teo-tsirpanis
commented
Nov 10, 2022

Copy link


Copy Markdown

Contributor
(if you want to backport it, the PR targeting release/7.0 should be in the 7.0.x milestone)
teo-tsirpanis
teo-tsirpanis
suggested changes
Nov 10, 2022
View reviewed changes
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSExportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

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.
@chsienkichsienkiNov 10, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agree with the symbols issue, but this was being done before. This PR doesn't change that, just makes the existing logic more efficient.
@jkoritzinskyjkoritzinskyNov 10, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Same.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 11, 2022

Closed

@chsienki
chsienki
merged commit 4acfa64
into
dotnet:mainNov 17, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 17, 2022
@RikkiGibson
RikkiGibson
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
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.
image
image
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
It shipped in .NET 7.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@jkoritzinsky thanks. Can we backport toys fix to 7.0? It's fairly brutal :-)
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
We should be able to. I'll try the backport bot on this and follow up on Monday to get it through servicing.
@github-actionsgithub-actionsBot
unlocked this conversation
Apr 15, 2023
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4706135407
@dotnetdotnet
deleted a comment from github-actionsBotApr 15, 2023
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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 128
Please backport manually!
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
I'll manually backport this on Monday.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Thanks!
@jkoritzinskyjkoritzinsky
mentioned this pull request
Apr 17, 2023
 Merged

@jkoritzinsky
jkoritzinsky
commented
Apr 17, 2023

Copy link


Copy Markdown

Member
Backport PR at #84936
jkoritzinsky pushed a commit
to jkoritzinsky/runtime
that referenced
this pull request
Apr 17, 2023
@chsienki@jkoritzinsky
Use forattributename jsgenerator (dotnet#78136)
5cf9e91
* Use ForAttributeWithMetadataName instead of CreateSyntaxProvider
* Remove unused code
* Update export generator too
jkoritzinsky added a commit
that referenced
this pull request
Apr 21, 2023
@jkoritzinsky@chsienki
Backport#78136to .NET 7 (#84936)
6e3c5c7
Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
@ghostghost
locked as resolved and limited conversation to collaborators
May 17, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@pavelsavarapavelsavarapavelsavara approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner

+1 more reviewer

@teo-tsirpanisteo-tsirpanisteo-tsirpanis requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@chsienkichsienki

Labels

area-System.Runtime.InteropServices.JavaScript

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@chsienki@teo-tsirpanis@RikkiGibson@CyrusNajmabadi@jkoritzinsky@pavelsavara@AaronRobinsonMSFT

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Use forattributename jsgenerator by chsienki · Pull Request #78136 · dotnet/runtime · GitHub
Skip to content
Use forattributename jsgenerator - #78136
Merged
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Nov 17, 2022
Merged
Use forattributename jsgenerator#78136
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Conversation
@chsienki
@chsienkichsienki
commented
Nov 10, 2022

Copy link


Copy Markdown

Member
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
@ghostghost
assigned chsienkiNov 10, 2022
@ghost
ghost
commented
Nov 10, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/interop-contrib

See info in area-owners.md if you want to be subscribed.
Issue Details
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
Author:chsienki
Assignees:-
Labels:
area-System.Runtime.InteropServices
Milestone:-
@AaronRobinsonMSFTAaronRobinsonMSFT
added this to the 7.0.x milestone
Nov 10, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 10, 2022

Closed

@teo-tsirpanisteo-tsirpanis
modified the milestones:
7.0.x, 8.0.0Nov 10, 2022
@teo-tsirpanis
teo-tsirpanis
commented
Nov 10, 2022

Copy link


Copy Markdown

Contributor
(if you want to backport it, the PR targeting release/7.0 should be in the 7.0.x milestone)
teo-tsirpanis
teo-tsirpanis
suggested changes
Nov 10, 2022
View reviewed changes
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSExportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

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.
@chsienkichsienkiNov 10, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agree with the symbols issue, but this was being done before. This PR doesn't change that, just makes the existing logic more efficient.
@jkoritzinskyjkoritzinskyNov 10, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Same.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 11, 2022

Closed

@chsienki
chsienki
merged commit 4acfa64
into
dotnet:mainNov 17, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 17, 2022
@RikkiGibson
RikkiGibson
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
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.
image
image
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
It shipped in .NET 7.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@jkoritzinsky thanks. Can we backport toys fix to 7.0? It's fairly brutal :-)
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
We should be able to. I'll try the backport bot on this and follow up on Monday to get it through servicing.
@github-actionsgithub-actionsBot
unlocked this conversation
Apr 15, 2023
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4706135407
@dotnetdotnet
deleted a comment from github-actionsBotApr 15, 2023
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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 128
Please backport manually!
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
I'll manually backport this on Monday.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Thanks!
@jkoritzinskyjkoritzinsky
mentioned this pull request
Apr 17, 2023
 Merged

@jkoritzinsky
jkoritzinsky
commented
Apr 17, 2023

Copy link


Copy Markdown

Member
Backport PR at #84936
jkoritzinsky pushed a commit
to jkoritzinsky/runtime
that referenced
this pull request
Apr 17, 2023
@chsienki@jkoritzinsky
Use forattributename jsgenerator (dotnet#78136)
5cf9e91
* Use ForAttributeWithMetadataName instead of CreateSyntaxProvider
* Remove unused code
* Update export generator too
jkoritzinsky added a commit
that referenced
this pull request
Apr 21, 2023
@jkoritzinsky@chsienki
Backport#78136to .NET 7 (#84936)
6e3c5c7
Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
@ghostghost
locked as resolved and limited conversation to collaborators
May 17, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@pavelsavarapavelsavarapavelsavara approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner

+1 more reviewer

@teo-tsirpanisteo-tsirpanisteo-tsirpanis requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@chsienkichsienki

Labels

area-System.Runtime.InteropServices.JavaScript

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@chsienki@teo-tsirpanis@RikkiGibson@CyrusNajmabadi@jkoritzinsky@pavelsavara@AaronRobinsonMSFT

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Use forattributename jsgenerator by chsienki · Pull Request #78136 · dotnet/runtime · GitHub
Skip to content
Use forattributename jsgenerator - #78136
Merged
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Nov 17, 2022
Merged
Use forattributename jsgenerator#78136
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Conversation
@chsienki
@chsienkichsienki
commented
Nov 10, 2022

Copy link


Copy Markdown

Member
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
@ghostghost
assigned chsienkiNov 10, 2022
@ghost
ghost
commented
Nov 10, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/interop-contrib

See info in area-owners.md if you want to be subscribed.
Issue Details
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
Author:chsienki
Assignees:-
Labels:
area-System.Runtime.InteropServices
Milestone:-
@AaronRobinsonMSFTAaronRobinsonMSFT
added this to the 7.0.x milestone
Nov 10, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 10, 2022

Closed

@teo-tsirpanisteo-tsirpanis
modified the milestones:
7.0.x, 8.0.0Nov 10, 2022
@teo-tsirpanis
teo-tsirpanis
commented
Nov 10, 2022

Copy link


Copy Markdown

Contributor
(if you want to backport it, the PR targeting release/7.0 should be in the 7.0.x milestone)
teo-tsirpanis
teo-tsirpanis
suggested changes
Nov 10, 2022
View reviewed changes
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSExportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

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.
@chsienkichsienkiNov 10, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agree with the symbols issue, but this was being done before. This PR doesn't change that, just makes the existing logic more efficient.
@jkoritzinskyjkoritzinskyNov 10, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Same.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 11, 2022

Closed

@chsienki
chsienki
merged commit 4acfa64
into
dotnet:mainNov 17, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 17, 2022
@RikkiGibson
RikkiGibson
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
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.
image
image
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
It shipped in .NET 7.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@jkoritzinsky thanks. Can we backport toys fix to 7.0? It's fairly brutal :-)
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
We should be able to. I'll try the backport bot on this and follow up on Monday to get it through servicing.
@github-actionsgithub-actionsBot
unlocked this conversation
Apr 15, 2023
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4706135407
@dotnetdotnet
deleted a comment from github-actionsBotApr 15, 2023
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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 128
Please backport manually!
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
I'll manually backport this on Monday.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Thanks!
@jkoritzinskyjkoritzinsky
mentioned this pull request
Apr 17, 2023
 Merged

@jkoritzinsky
jkoritzinsky
commented
Apr 17, 2023

Copy link


Copy Markdown

Member
Backport PR at #84936
jkoritzinsky pushed a commit
to jkoritzinsky/runtime
that referenced
this pull request
Apr 17, 2023
@chsienki@jkoritzinsky
Use forattributename jsgenerator (dotnet#78136)
5cf9e91
* Use ForAttributeWithMetadataName instead of CreateSyntaxProvider
* Remove unused code
* Update export generator too
jkoritzinsky added a commit
that referenced
this pull request
Apr 21, 2023
@jkoritzinsky@chsienki
Backport#78136to .NET 7 (#84936)
6e3c5c7
Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
@ghostghost
locked as resolved and limited conversation to collaborators
May 17, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@pavelsavarapavelsavarapavelsavara approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner

+1 more reviewer

@teo-tsirpanisteo-tsirpanisteo-tsirpanis requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@chsienkichsienki

Labels

area-System.Runtime.InteropServices.JavaScript

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@chsienki@teo-tsirpanis@RikkiGibson@CyrusNajmabadi@jkoritzinsky@pavelsavara@AaronRobinsonMSFT

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Use forattributename jsgenerator by chsienki · Pull Request #78136 · dotnet/runtime · GitHub
Skip to content
Use forattributename jsgenerator - #78136
Merged
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Nov 17, 2022
Merged
Use forattributename jsgenerator#78136
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Conversation
@chsienki
@chsienkichsienki
commented
Nov 10, 2022

Copy link


Copy Markdown

Member
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
@ghostghost
assigned chsienkiNov 10, 2022
@ghost
ghost
commented
Nov 10, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/interop-contrib

See info in area-owners.md if you want to be subscribed.
Issue Details
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
Author:chsienki
Assignees:-
Labels:
area-System.Runtime.InteropServices
Milestone:-
@AaronRobinsonMSFTAaronRobinsonMSFT
added this to the 7.0.x milestone
Nov 10, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 10, 2022

Closed

@teo-tsirpanisteo-tsirpanis
modified the milestones:
7.0.x, 8.0.0Nov 10, 2022
@teo-tsirpanis
teo-tsirpanis
commented
Nov 10, 2022

Copy link


Copy Markdown

Contributor
(if you want to backport it, the PR targeting release/7.0 should be in the 7.0.x milestone)
teo-tsirpanis
teo-tsirpanis
suggested changes
Nov 10, 2022
View reviewed changes
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSExportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

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.
@chsienkichsienkiNov 10, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agree with the symbols issue, but this was being done before. This PR doesn't change that, just makes the existing logic more efficient.
@jkoritzinskyjkoritzinskyNov 10, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Same.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 11, 2022

Closed

@chsienki
chsienki
merged commit 4acfa64
into
dotnet:mainNov 17, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 17, 2022
@RikkiGibson
RikkiGibson
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
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.
image
image
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
It shipped in .NET 7.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@jkoritzinsky thanks. Can we backport toys fix to 7.0? It's fairly brutal :-)
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
We should be able to. I'll try the backport bot on this and follow up on Monday to get it through servicing.
@github-actionsgithub-actionsBot
unlocked this conversation
Apr 15, 2023
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4706135407
@dotnetdotnet
deleted a comment from github-actionsBotApr 15, 2023
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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 128
Please backport manually!
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
I'll manually backport this on Monday.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Thanks!
@jkoritzinskyjkoritzinsky
mentioned this pull request
Apr 17, 2023
 Merged

@jkoritzinsky
jkoritzinsky
commented
Apr 17, 2023

Copy link


Copy Markdown

Member
Backport PR at #84936
jkoritzinsky pushed a commit
to jkoritzinsky/runtime
that referenced
this pull request
Apr 17, 2023
@chsienki@jkoritzinsky
Use forattributename jsgenerator (dotnet#78136)
5cf9e91
* Use ForAttributeWithMetadataName instead of CreateSyntaxProvider
* Remove unused code
* Update export generator too
jkoritzinsky added a commit
that referenced
this pull request
Apr 21, 2023
@jkoritzinsky@chsienki
Backport#78136to .NET 7 (#84936)
6e3c5c7
Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
@ghostghost
locked as resolved and limited conversation to collaborators
May 17, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@pavelsavarapavelsavarapavelsavara approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner

+1 more reviewer

@teo-tsirpanisteo-tsirpanisteo-tsirpanis requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@chsienkichsienki

Labels

area-System.Runtime.InteropServices.JavaScript

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@chsienki@teo-tsirpanis@RikkiGibson@CyrusNajmabadi@jkoritzinsky@pavelsavara@AaronRobinsonMSFT

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Use forattributename jsgenerator by chsienki · Pull Request #78136 · dotnet/runtime · GitHub
Skip to content
Use forattributename jsgenerator - #78136
Merged
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Nov 17, 2022
Merged
Use forattributename jsgenerator#78136
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Conversation
@chsienki
@chsienkichsienki
commented
Nov 10, 2022

Copy link


Copy Markdown

Member
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
@ghostghost
assigned chsienkiNov 10, 2022
@ghost
ghost
commented
Nov 10, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/interop-contrib

See info in area-owners.md if you want to be subscribed.
Issue Details
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
Author:chsienki
Assignees:-
Labels:
area-System.Runtime.InteropServices
Milestone:-
@AaronRobinsonMSFTAaronRobinsonMSFT
added this to the 7.0.x milestone
Nov 10, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 10, 2022

Closed

@teo-tsirpanisteo-tsirpanis
modified the milestones:
7.0.x, 8.0.0Nov 10, 2022
@teo-tsirpanis
teo-tsirpanis
commented
Nov 10, 2022

Copy link


Copy Markdown

Contributor
(if you want to backport it, the PR targeting release/7.0 should be in the 7.0.x milestone)
teo-tsirpanis
teo-tsirpanis
suggested changes
Nov 10, 2022
View reviewed changes
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSExportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

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.
@chsienkichsienkiNov 10, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agree with the symbols issue, but this was being done before. This PR doesn't change that, just makes the existing logic more efficient.
@jkoritzinskyjkoritzinskyNov 10, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Same.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 11, 2022

Closed

@chsienki
chsienki
merged commit 4acfa64
into
dotnet:mainNov 17, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 17, 2022
@RikkiGibson
RikkiGibson
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
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.
image
image
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
It shipped in .NET 7.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@jkoritzinsky thanks. Can we backport toys fix to 7.0? It's fairly brutal :-)
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
We should be able to. I'll try the backport bot on this and follow up on Monday to get it through servicing.
@github-actionsgithub-actionsBot
unlocked this conversation
Apr 15, 2023
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4706135407
@dotnetdotnet
deleted a comment from github-actionsBotApr 15, 2023
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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 128
Please backport manually!
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
I'll manually backport this on Monday.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Thanks!
@jkoritzinskyjkoritzinsky
mentioned this pull request
Apr 17, 2023
 Merged

@jkoritzinsky
jkoritzinsky
commented
Apr 17, 2023

Copy link


Copy Markdown

Member
Backport PR at #84936
jkoritzinsky pushed a commit
to jkoritzinsky/runtime
that referenced
this pull request
Apr 17, 2023
@chsienki@jkoritzinsky
Use forattributename jsgenerator (dotnet#78136)
5cf9e91
* Use ForAttributeWithMetadataName instead of CreateSyntaxProvider
* Remove unused code
* Update export generator too
jkoritzinsky added a commit
that referenced
this pull request
Apr 21, 2023
@jkoritzinsky@chsienki
Backport#78136to .NET 7 (#84936)
6e3c5c7
Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
@ghostghost
locked as resolved and limited conversation to collaborators
May 17, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@pavelsavarapavelsavarapavelsavara approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner

+1 more reviewer

@teo-tsirpanisteo-tsirpanisteo-tsirpanis requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@chsienkichsienki

Labels

area-System.Runtime.InteropServices.JavaScript

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@chsienki@teo-tsirpanis@RikkiGibson@CyrusNajmabadi@jkoritzinsky@pavelsavara@AaronRobinsonMSFT

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Use forattributename jsgenerator by chsienki · Pull Request #78136 · dotnet/runtime · GitHub
Skip to content
Use forattributename jsgenerator - #78136
Merged
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Nov 17, 2022
Merged
Use forattributename jsgenerator#78136
chsienki merged 3 commits into
dotnet:mainfrom 
chsienki:use_forattributename_jsgenerator
Conversation
@chsienki
@chsienkichsienki
commented
Nov 10, 2022

Copy link


Copy Markdown

Member
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
@ghostghost
assigned chsienkiNov 10, 2022
@ghost
ghost
commented
Nov 10, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/interop-contrib

See info in area-owners.md if you want to be subscribed.
Issue Details
Updates the JSImportGenerator and JSExportGenerator to use ForAttributeWithMetadataName (instead of directly creating a syntax provider) which improves performance.
Author:chsienki
Assignees:-
Labels:
area-System.Runtime.InteropServices
Milestone:-
@AaronRobinsonMSFTAaronRobinsonMSFT
added this to the 7.0.x milestone
Nov 10, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 10, 2022

Closed

@teo-tsirpanisteo-tsirpanis
modified the milestones:
7.0.x, 8.0.0Nov 10, 2022
@teo-tsirpanis
teo-tsirpanis
commented
Nov 10, 2022

Copy link


Copy Markdown

Contributor
(if you want to backport it, the PR targeting release/7.0 should be in the 7.0.x milestone)
teo-tsirpanis
teo-tsirpanis
suggested changes
Nov 10, 2022
View reviewed changes
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSExportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

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.
@chsienkichsienkiNov 10, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agree with the symbols issue, but this was being done before. This PR doesn't change that, just makes the existing logic more efficient.
@jkoritzinskyjkoritzinskyNov 10, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment thread...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs
 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 });
@teo-tsirpanisteo-tsirpanisNov 10, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Same.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 11, 2022

Closed

@chsienki
chsienki
merged commit 4acfa64
into
dotnet:mainNov 17, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 17, 2022
@RikkiGibson
RikkiGibson
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
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.
image
image
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
It shipped in .NET 7.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@jkoritzinsky thanks. Can we backport toys fix to 7.0? It's fairly brutal :-)
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
We should be able to. I'll try the backport bot on this and follow up on Monday to get it through servicing.
@github-actionsgithub-actionsBot
unlocked this conversation
Apr 15, 2023
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
/backport to release/7.0-staging
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4706135407
@dotnetdotnet
deleted a comment from github-actionsBotApr 15, 2023
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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 128
Please backport manually!
@github-actions
github-actionsBot
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
@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.
@jkoritzinsky
jkoritzinsky
commented
Apr 15, 2023

Copy link


Copy Markdown

Member
I'll manually backport this on Monday.
@CyrusNajmabadi
CyrusNajmabadi
commented
Apr 15, 2023

Copy link


Copy Markdown

Contributor
Thanks!
@jkoritzinskyjkoritzinsky
mentioned this pull request
Apr 17, 2023
 Merged

@jkoritzinsky
jkoritzinsky
commented
Apr 17, 2023

Copy link


Copy Markdown

Member
Backport PR at #84936
jkoritzinsky pushed a commit
to jkoritzinsky/runtime
that referenced
this pull request
Apr 17, 2023
@chsienki@jkoritzinsky
Use forattributename jsgenerator (dotnet#78136)
5cf9e91
* Use ForAttributeWithMetadataName instead of CreateSyntaxProvider
* Remove unused code
* Update export generator too
jkoritzinsky added a commit
that referenced
this pull request
Apr 21, 2023
@jkoritzinsky@chsienki
Backport#78136to .NET 7 (#84936)
6e3c5c7
Co-authored-by: Chris Sienkiewicz <chsienki@microsoft.com>
@ghostghost
locked as resolved and limited conversation to collaborators
May 17, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@pavelsavarapavelsavarapavelsavara approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner

+1 more reviewer

@teo-tsirpanisteo-tsirpanisteo-tsirpanis requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@chsienkichsienki

Labels

area-System.Runtime.InteropServices.JavaScript

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@chsienki@teo-tsirpanis@RikkiGibson@CyrusNajmabadi@jkoritzinsky@pavelsavara@AaronRobinsonMSFT