[release/7.0] Fix max chunk size limiting #81607
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
jeffschwMSFT
removed
the
Servicing-consider
carlossanlop
added
the
NO-MERGE
When the new stub heaps were implemented, the chunk size needed to be limited so that all precodes for a chunk fit into a single memory page. That was done in `MethodDescChunk::CreateChunk`. But it was discovered now that there is another place where it needs to be limited, the `MethodTableBuilder::AllocAndInitMethodDescChunk`. The JIT\opt\ObjectStackAllocation\ObjectStackAllocationTests started to fail in the outerloop due to too long chunk. The failure happens in crossgen2 as it is using a separate build of runtime in release and only that uncovers the problem. And only when DOTNET_TieredCompilation=0 and DOTNET_ProfApi_RejitOnAttach=0. This change fixes the problem. With this change applied to the dotnet runtime version used by the crossgen2 and patching it in place in the .dotnet/shared/.... directory, the issue doesn't occur. Close #81103
carlossanlop
force-pushed
the
backport/pr-81192-to-release/7.0
branch
from
dbf4532
to
c55282f
Compare
|
@janvorli Friendly reminder that today is the Code Complete date for the March Servicing Release. If you'd like this change to be included in next month's release, please add the |
|
@carlossanlop thanks for the reminder. In this case, we decided to hold for more customer information before consider for servicing. |
|
This is still marked as |
|
I'm retargeting this PR to the new Repo maintainers will now be allowed to merge their own servicing PR as long as it meets the requirements:
The new process is described here: runtime/docs/project/library-servicing.md. The infra team will be actively monitoring servicing PRs to ensure all requirements are met and to help with any issues. Let me know if you have any questions. |
|
Friendly reminder: if you want this servicing fix to be included in the September 2023 Release, you'll have to merge this PR before August 14th. |
|
@janvorli this hasn't been merged. Do we still want to keep it open? |
|
We haven't seen a customer hit this issue, so let's close it. |
|
@carlossanlop I have reopened this request since a customer has just hit the issue in .NET 7. |
|
@janvorli, forgot to add |
|
@teo-tsirpanis yes, thank you for spotting that. And it was actually already approved on 9/20 by the tactics. |
|
@carlossanlop should I actually set the label to servicing-approved then? |
jeffschwMSFT
added
Servicing-approved
|
@janvorli is this still no merge? |
|
@jeffschwMSFT no, it should not be marked as no merge. |
janvorli
removed
the
NO-MERGE
|
@carlossanlop this is good to go. Thanks! |
|
Nevermind. I saw the title, not the branch. All good. By the way, PR owners can merge their own PRs in servicing branches. |
ghost
locked as resolved and limited conversation to collaborators
Backport of #81192 to release/7.0
/cc @janvorli
Customer Impact
When a customer application has a class with 170 or more methods, it causes access violation when creating temporary entry points. It occurs in some cases only though. Here are some of them:
DOTNET_TieredCompilation=0) and the class is loaded in a collectible assemblyDOTNET_TieredCompilation=0) and Rejit is turned off (DOTNET_ProfAPI_RejitOnAttach=0)System.Runtime.TieredCompilation.QuickJitsetting is set to false orDOTNET_TC_QuickJit=0) and the assembly is not crossgened (or ReadyToRun is disabled usingDOTNET_ReadyToRun=0).Testing
Local manual testing of the affected scenario, CI tests.
Risk
Low, the change is in a frequently exercised code and affects only the case when it would cause the AV