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

Improve performance by using sealed classes #15110

Open
iSazonov opened this issue Mar 27, 2021 · 11 comments
Open

Improve performance by using sealed classes #15110

iSazonov opened this issue Mar 27, 2021 · 11 comments
Labels
Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality WG-Engine-Performance core PowerShell engine, interpreter, and runtime performance

Comments

@iSazonov
Copy link
Collaborator

iSazonov commented Mar 27, 2021

Inspired by dotnet/runtime#49944

  • We could think about getting rid of InternalsVisibleTo attributes (specially in SMA).
  • We could make (all) private classes sealed.
  • We could make (all) internal classes sealed.

This can be especially important if we use PGO for optimizing startup scenarios.

@iSazonov iSazonov added WG-Engine-Performance core PowerShell engine, interpreter, and runtime performance Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality Needs-Triage The issue is new and needs to be triaged by a work group. labels Mar 27, 2021
@daxian-dbw
Copy link
Member

We could think about getting rid of InternalsVisibleTo attributes (specially in SMA).

This is not possible given that management.dll, utility.dll, and security.dll depending on this flag. Not only that, subsystems will depend on that flag as well in future.

But yeah, it would be nice to follow this best practice in our code base to mark types sealed properly.

This can be especially important if we use PGO for optimizing startup scenarios

@iSazonov why PGO would require this? can you please clarify this statement? Also, can you be more specific how PGO can be used in PowerShell?

@daxian-dbw daxian-dbw removed the Needs-Triage The issue is new and needs to be triaged by a work group. label Jun 24, 2021
@iSazonov
Copy link
Collaborator Author

iSazonov commented Jun 24, 2021

My main concern is that using of InternalsVisibleTo will absolutely block PGO for us. There is still no documentation for PGO. You may want to talk to the authors of PGO.

I can only guess that InternalsVisibleTo and sealed both truncate any inheritance/virtualization and as a consequence allow bolder optimizations.

Also, can you be more specific how PGO can be used in PowerShell?

From my understanding it is the main purpose of the PGO to improve startup scenario(s). We could create specific PowerShell PGO profile and distribute it with PowerShell.

But yeah, it would be nice to follow this best practice in our code base to mark types sealed properly.

It seems we lost this in #11773 /cc @xtqqczze

@xtqqczze
Copy link
Contributor

It seems we lost this in #11773 /cc @xtqqczze

So perhaps we want a new PR similar to those changes (2069d92), but only private, internal?

@iSazonov
Copy link
Collaborator Author

From my understanding it is the main purpose of the PGO to improve startup scenario(s). We could create specific PowerShell PGO profile and distribute it with PowerShell.

Also I think we could accelerate other typical scenarios too like runspace/scriptblock creation.

@xtqqczze
Copy link
Contributor

xtqqczze commented Jun 25, 2021

We could look into: https://github.com/geeknoid/UnsealedAnalyzer

GitHub
Prototype of an analyzer to report unsealed types. Contribute to geeknoid/UnsealedAnalyzer development by creating an account on GitHub.

@xtqqczze
Copy link
Contributor

xtqqczze commented Jul 6, 2021

@iSazonov I wonder whether #15675 has improved performance and whether we could measure this?

@iSazonov
Copy link
Collaborator Author

iSazonov commented Jul 6, 2021

It is best practice. .Net Runtime can do more optimizations if a class cannot have derived classes and cannot be used outside.

@xtqqczze
Copy link
Contributor

xtqqczze commented Jul 6, 2021

It is best practice. .Net Runtime can do more optimizations if a class cannot have derived classes and cannot be used outside.

It would be good to quantify the improvement, to see whether further optimizations of the type would be fruitful.

@xtqqczze
Copy link
Contributor

xtqqczze commented Oct 4, 2022

We now have CA1852: Seal internal types in .NET 7, so we should try to enable it.

@xtqqczze
Copy link
Contributor

@iSazonov Could you please reopen?

@xtqqczze
Copy link
Contributor

xtqqczze commented Feb 7, 2024

Seal classes in RemotingProtocol2 (#21164) contributes to this issue.

xtqqczze added a commit to xtqqczze/PowerShell that referenced this issue Feb 13, 2024
The motivation of this PR is this comment by @PaulHigin PowerShell#11820 (comment).

_Contributes to PowerShell#15110._
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality WG-Engine-Performance core PowerShell engine, interpreter, and runtime performance
Projects
None yet
Development

No branches or pull requests

3 participants