Skip to content

Update wording for AggressiveOptimizations #5355

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

Closed
wants to merge 1 commit into from

Conversation

john-h-k
Copy link
Contributor

Summary

This is probably one of the most understood MethodImplOptions (if not enums) in this area. This PR aims to clarify it

cc @tannergooding cc @SingleAccretion

cc @tannergooding  cc @SingleAccretion 

This is probably one of the most understood MethodImplOptions (if not enums) in this area. This PR aims to clarify it
@@ -135,7 +135,9 @@
</ReturnValue>
<MemberValue>512</MemberValue>
<Docs>
<summary>The method contains a hot path and should be optimized.</summary>
<summary>The method should skip tiered-compilation and immediately reach tier0.
Copy link
Contributor

@SingleAccretion SingleAccretion Feb 20, 2021

Choose a reason for hiding this comment

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

Presumably, Tier1. However, I do not think this terminology is used anywhere in the public-facing documentation.

I would suggest just dropping the " and immediately reach tier0" part.

@@ -135,7 +135,9 @@
</ReturnValue>
<MemberValue>512</MemberValue>
<Docs>
<summary>The method contains a hot path and should be optimized.</summary>
<summary>The method should skip tiered-compilation and immediately reach tier0.
This can be beneficial for certain hot-path code which does not rely on any static state or any optimisations applied at re-jit time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Which optimizations, which paths are "certain"? As a someone who is looking to optimize my code and not sure whether I should use this attribute or not, I am afraid this is too little detail, at the same time, I do not know how to better word this...

I think the general guidance of: "this method is known to be hot, does not rely on state that is known only after is has been executed, and is on a startup path" is OK'ish?

At the same time, no Tier0 means no profile data and no PGO, so in a sense, all methods should benefit, except if they are hand-optimized with gotos and such.

@@ -135,7 +135,9 @@
</ReturnValue>
<MemberValue>512</MemberValue>
<Docs>
<summary>The method contains a hot path and should be optimized.</summary>
<summary>The method should skip tiered-compilation and immediately reach tier0.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just pasting that tag from the C# server if you want to inteegrate some of it in the remarks section 😄

The AggressiveOptimization option is meant for very specific cases and should only ever be used when sure that the method being annotated actually benefits from that (possibly running some benchmarks to confirm that). Despite what the name might suggest, this option will not "make your code faster", on the contrary it might very well make it slower in many situations. AggressiveOptimization means that the method will skip the tiered compilation and go directly to tier1. This can be beneficial for methods that are hot paths or long running, but only executed once, so that the tiered JIT wouldn't have time to kick in (for instance, for a complex app initialization method that is run at startup). But skipping the tier0 -> tier1 transition also means that a number of additional optimizations cannot be done by the JIT (eg. removing checks for the static constructor, or inlining static readonly fields). Because of this, in many cases applying this option might actually make your code slower. And regardless, the JIT will often skip tier0 anyway in many cases already (eg. in methods with a backwards branch), so it might very well be unnecessary anyway. Long story short: if you're not 100% sure, just don't use this option in your code.

@gewarren gewarren closed this Mar 3, 2021
@gewarren gewarren reopened this Mar 3, 2021
@opbld30
Copy link

opbld30 commented Mar 3, 2021

Docs Build status updates of commit 7d86f21:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Runtime.CompilerServices/MethodImplOptions.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@tannergooding
Copy link
Member

This likely needs review from @AndyAyersMS and @jkotas

<summary>The method contains a hot path and should be optimized.</summary>
<summary>The method should skip tiered-compilation and immediately reach tier0.
This can be beneficial for certain hot-path code which does not rely on any static state or any optimisations applied at re-jit time.
This attribute will not result in faster or better optimised code and is designed for use in very specific benchmark-lead scenarios.</summary>
</Docs>
Copy link
Member

Choose a reason for hiding this comment

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

This describes what this attribute does in the current coreclr runtime. I expect that the details will change a lot over time. We typically refrain from documenting implementation details like this since it makes the documentation constantly outdated.

The method contains a hot path and should be optimized

All methods are optimized. I would change the description to "The method contains a very critical hot path and should be optimized with highest optimization level possible.".

But I think we should avoid describing the current implementation details of coreclr runtime.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with Jan.

I also wouldn't connect it to benchmarking.

@MSDN-WhiteKnight
Copy link
Contributor

This PR likely conflicted with #9188. Is it still needed after changes in that PR?

@jkotas
Copy link
Member

jkotas commented Aug 18, 2023

I agree, this PR is outdated and the primary concern has been addressed by the other PR.

@john-h-k Thank you for your contribution. Please open a new PR with more updates if necessary.

@jkotas jkotas closed this Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants