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

Enable GDV with multiple guesses for NativeAOT #87380

Merged
merged 2 commits into from Jun 19, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jun 11, 2023

I've tested the size impact on the Minimal API TodosApp:

MaxTypeChecks -O (default), KB -Ot (Prefer Speed), KB
0 28466 30212
1 (Current default) 28468 30216
2 28504 30267
3 (Proposed default) 28552 30321
4 28577 30362
5 28706 30501

(for reference, -Os is 27980)

Numbers are sizes (in KB) for differen values of --codegenopt:JitGuardedDevirtualizationMaxTypeChecks=X

See #87055 for an example of what GDV with multiple guesses can do.

so +84kb for -O and +105kb for -Ot (more aggressive inliner inlines more GDV guesses).

We (@AndyAyersMS) think that 3 is the optimal number for number of guesses because e.g. 5 means that if the last guess is the correct one we'll pay an overhead of 4 type checks before we hit it and that can make it slower than just a virtual call.

@EgorBo
Copy link
Member Author

EgorBo commented Jun 11, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Jun 11, 2023

Unrelated: played with some jit opts and -Os mode to check size savings:

  • Disabled "loop clonning" opt: -150kb
  • Disabled "'finally' block clonning": -60kb
  • Changed DEFAULT_MAX_INLINE_SIZE from 100 to 20: -450kb

@MichalStrehovsky
Copy link
Member

This is great! I'm not worried about the size hit, thanks for measuring! I assume 3 type checks means that if we have an interface that is implemented by 4 types, this would be covered (we generate 3 checks and a fallback to the last possible option).

Just a general question - I'm not opposed to having this on the ILC side, but wouldn't it be better to have these defaults set on the RyuJIT side? We do pass SIZE_OPT to RyuJIT so it could also set the right default there. E.g. I wouldn't expect us to pass a DEFAULT_MAX_INLINE_SIZE from ILC side to RyuJIT - this all feels like part of the general "codegen policy" we specify with SIZE_OPT/SPEED_OPT/BLENDED and how exactly RyuJIT achieves that is RyuJIT's implementation detail.

@EgorBo
Copy link
Member Author

EgorBo commented Jun 12, 2023

This is great! I'm not worried about the size hit, thanks for measuring! I assume 3 type checks means that if we have an interface that is implemented by 4 types, this would be covered (we generate 3 checks and a fallback to the last possible option).

Nope, if there are 4 impl and the MaxTypeChecks=3 we give up - it can be changed, the problem that we don't have probabilities here like with PGO, so we may end up expanding a call for 3 type checks which will never be taken as an app only uses the 4th one so we just add 3 type checks + lots of inlined code for them that is never executed.

Just a general question - I'm not opposed to having this on the ILC side, but wouldn't it be better to have these defaults set on the RyuJIT side? We do pass SIZE_OPT to RyuJIT so it could also set the right default there. E.g. I wouldn't expect us to pass a DEFAULT_MAX_INLINE_SIZE from ILC side to RyuJIT - this all feels like part of the general "codegen policy" we specify with SIZE_OPT/SPEED_OPT/BLENDED and how exactly RyuJIT achieves that is RyuJIT's implementation detail.

No strong opinion on that, it's just that I want DOTNET_JitGuardedDevirtualizationMaxTypeChecks to be 3 for NAOT and 1 for CLR (CLR doesn't support "exact" devirtualization, only PGO-driven one and for that I plan to do measurements separately) and make it configurable from ILC's args - so I don't see how it can be done on JIT side, if I do on the jit side something like:

if (nativeAOT)
maxTypeChecks = 3;

I'll basically make it non-configurable from ILC args - if that's ok I can change

@jkotas
Copy link
Member

jkotas commented Jun 12, 2023

I don't see how it can be done on JIT side

  • Give it fixed default value of -1
  • Substitute -1 with the actual default based on the ABI

@EgorBo
Copy link
Member Author

EgorBo commented Jun 12, 2023

@jkotas @MichalStrehovsky addressed

@MichalStrehovsky
Copy link
Member

@jkotas @MichalStrehovsky addressed

Thanks! This looks great to me but it is a JIT change now and someone from the JIT team would be more qualified to sign off.

@EgorBo EgorBo merged commit 6bf7c02 into dotnet:main Jun 19, 2023
127 checks passed
@EgorBo EgorBo deleted the enable-gdv-naot branch June 19, 2023 20:17
@MichalStrehovsky
Copy link
Member

I think we're seeing a 1.5% improvement in the Stage1 app with SizeOpts at https://aka.ms/aspnet/nativeaot/benchmarks:

image

@EgorBo
Copy link
Member Author

EgorBo commented Jun 20, 2023

I think we're seeing a 1.5% improvement in the Stage1 app with SizeOpts at https://aka.ms/aspnet/nativeaot/benchmarks:

image

I am hoping to see improvements when we also enable GDV for interface calls where we don't inline them - currently we always give up on them.

@MichalStrehovsky
Copy link
Member

I am hoping to see improvements when we also enable GDV for interface calls where we don't inline them - currently we always give up on them.

Ah, that would probably explain why this didn't help much without the speedopts - without tier-1 inlining budgets we likely give up often.

@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants