Clean up AggressiveOpt tier in vm

[EgorBo]

Closes #90490

Currently, MethodDesc with [AggressiveOptimization] attribute still returns true for IsEligibleForTieredCompilation(). This PR makes it false + clean up.

So now methods with that attribute are reported as FullOpts instead of Tier1.

PS: I recommend disabling "show whitespaces" on the review page.

[danmoseley]

Does this mean by adding that attribute my method can get optimized code immediately? If so that sounds like a workaround for #87753?

[EgorBo]

Does this mean by adding that attribute my method can get optimized code immediately? If so that sounds like a workaround for #87753?

This PR doesn't change anything for codegen, it only fixes "diagnostics" basically, it's now easier to understand from JitDisasm/JitDisasmSummary or even PerfView whether this method bypassed tier0 or not. The attribute always forces JIT to compile a method straight to the final tier (usually, it has worse performance compared to not having that attribute and reaching Tier1 naturally, especially now, with PGO being enabled by default)..

[danmoseley]

This PR doesn't change anything for codegen, it only fixes "diagnostics" basically, it's now easier to understand from JitDisasm/JitDisasmSummary or even PerfView whether this method bypassed tier0 or not. ....The attribute always forces JIT to compile a method straight to the final tier

I'm having trouble parsing this -- if it forces JIT to compile directly to the final tier, that's what we want for #87753 isn't it? And it's not just diagnostics - that would be customer visible.

[EgorBo]

This PR doesn't change anything for codegen, it only fixes "diagnostics" basically, it's now easier to understand from JitDisasm/JitDisasmSummary or even PerfView whether this method bypassed tier0 or not. ....The attribute always forces JIT to compile a method straight to the final tier

I'm having trouble parsing this -- if it forces JIT to compile directly to the final tier, that's what we want for #87753 isn't it? And it's not just diagnostics - that would be customer visible.

When JIT bypasses Tier0/instrumented tiers and goes straight to the final one, it usually produces worse (in terms of perf) codegen , so we try to avoid recommending [AggressiveOptimization] and, in fact, we've alsmost eliminated its uses in the BCL 🙂 there are only 2 uses left and there is already a PR to get rid of them filed.

The codegen is worse because of two reasons:

1. it won't collect any profile so no perf benefits from Dynamic PGO
2. even without PGO we still benefit from Tiered Compilation - e.g. in Tier1 we usually already have all types statically initialzed so no helper calls to init them are needed + static readonly fields can be folded as constants.
   There are more things like initialized runtime lookups, etc.

For the case you referenced [AggressiveOptimization] is better than nothing but some config knob to make tiering faster is, probably, a better option.

[stephentoub]

in Tier1 we usually already have all types statically initialzed so no helper calls to init them are needed + static readonly fields can be folded as constants

Which the source generated code for regexes relies on heavily, e.g. to eliminate all the checks related to timeouts, to devirtualize SearchValues-related object accesses, etc.

[danmoseley]

When JIT bypasses Tier0/instrumented tiers and goes straight to the final one, it usually produces worse (in terms of perf) codegen , so we try to avoid recommending [AggressiveOptimization]

Ah -- I didn't realize this. It's become a misleading name, I guess. The docs do say this

Use this attribute if running an unoptimized version of the method has undesirable effects, for instance causing too much overhead or extra memory allocation.

Methods with this attribute may not have optimal code generation. They bypass the first tier of Tiered Compilation and therefore can't benefit from optimizations that rely on tiering, for example, Dynamic PGO or optimizations based on initialized classes.

I wonder how widely this is understood in the ecosystem. My guess is that it's almost always used "because this path is hot"

[EgorBo]

I wonder how widely this is understood in the ecosystem. My guess is that it's almost always used "because this path is hot"

https://grep.app/search?current=2&q=AggressiveOptimization&words=true&filter[lang][0]=C%23

Doesn't look too bad on grep.app (no idea how representative it is)

[jkotas]

I wonder how widely this is understood in the ecosystem. My guess is that it's almost always used "because this path is hot"

Part of the ecosystem does not measure that is the number one rule of perf, and use AggressiveInlining and AggressiveOptimization just because it is there. When not used carefully, each of these attributes can and will have opposite effect and actually make the code run slower.

[danmoseley]

use AggressiveInlining and AggressiveOptimization just because it is there.

when AggressiveInlining is a deoptimization is that essentially because of less code localization (and fatter assemblies)? or are there bad interactions with PGO, tiering, or other JIT optimizations?

Unlike for AggressiveOptimization, the MethodImplOptions topic doesn't warn about it.

[danmoseley]

BTW, I assume we don't expect we would ever offer a [ThisMethodIsHot] attribute (like AggressiveOptimization, but a hint only) because we expect it would be used inappropriately + PGO should do better.

[jkotas]

when AggressiveInlining is a deoptimization

Too much of AggressiveInlining makes the JIT hit its internal implementation limits that makes it use conservative register allocator and turn off optimizations. In this repo, we have this problem in System.Text.Json (see #85531). IMHO, System.Text.Json uses AggressiveInlining too much.

[danmoseley]

How does dotnet/dotnet-api-docs#9188 look?

[EgorBo]

Ping @kouvel, the PR is simple if you check "ignore whitespaces" on "Files changed" page.