-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
JIT: Don't reorder handler blocks #112292
JIT: Don't reorder handler blocks #112292
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
I'm curious about the motivation -- what is the benefit of skipping the layout for handlers? Is there a good reason to not optimize handlers? Note that the VM side has switched stance on importance of EH perf recently: #77568. So I think it needs to come with some motivation if we are intentionally skipping opts for handlers. |
Not having to handle exceptional flow during block layout means we can simplify our implementation quite a bit. Our current implementation is relatively simple because it doesn't allow reordering blocks in different regions, but one issue I want to address in the consolidated implementation is subpar placement of whole try regions. The goal of the consolidation is to reduce block layout's workflow to the following:
If we implement step 3 with the invariant that we only reorder within EH regions, child regions frequently "sink" down the method. Take the following example initial layout (sorry it isn't smaller, I just grabbed this from
Even if we raised the threshold for cold blocks, we'd still have this awkward jump from
The EH nesting checks needed to do this are cumbersome, so to simplify my initial prototype, I defined "unimportant" in step 1 as "cold, or a handler block". Our 3-opt implementation never considered edges in hander regions to begin with, but if we believe we're losing something by not placing handler regions in RPO, I can easily add that functionality back into my prototype. #112004 is bigger and incurs more churn than I'd like it to as-is, so I'd prefer to add further enhancements to it after it's been checked in. |
Thanks! Sounds reasonable to me. |
Diffs show relatively little churn on most platforms. |
* main: Code clean up in AP for NonNull* (dotnet#112027) JIT: Invalidate LSRA's DFS tree if we aren't running new layout phase (dotnet#112364) Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250204.2 (dotnet#112339) Add doc on OS onboarding (dotnet#112026) Add `TypeName` APIs to simplify metadata lookup. (dotnet#111598) Internal monitor impl not using coop mutex causing deadlocks on Android. (dotnet#112358) Do not run NAOT arm64 OSX testing on all PRs (dotnet#112342) Special-case empty enumerables in AsyncEnumerable (dotnet#112321) Have mono handle ConvertToIntegerNative for Double and Single (dotnet#112206) Update dependencies from https://github.com/dotnet/arcade build 20250206.4 (dotnet#112338) System.Configuration.ConfigurationManager.Tests: use Assembly.Location to determine ThisApplicationPath. (dotnet#112231) Force write of local file header when "version needed to extract" changes (dotnet#112032) JIT: Don't reorder handler blocks (dotnet#112292) [RISC-V] Synthesize some floating constants inline (dotnet#111529) Enable `SA1000`: Spacing around keywords (dotnet#112302) Fix relocs for linux-riscv64 AOT (dotnet#112331)
Splitting this out of #112004 to simplify diff triage. The cost of executing a handler region should be dominated by the runtime overhead of exception handling, so I don't think we're losing anything meaningful in not reordering them. In the case of finally regions, if they are sufficiently hot, the JIT should've copied them into the main method region.