-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[RuntimeAsync] Removing TODO in methodtablebuilder.cpp. More robust insertion of methods. #122377
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
Conversation
|
Tagging subscribers to this area: @mangod9 |
Co-authored-by: VSadov <8218165+VSadov@users.noreply.github.com>
Co-authored-by: VSadov <8218165+VSadov@users.noreply.github.com>
Per @VSadov's feedback, replaced runtime type generation with actual source-defined classes containing 40k and 32k+ methods. Co-authored-by: VSadov <8218165+VSadov@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR removes a TODO comment in methodtablebuilder.cpp and makes the insertion of async method variants more robust by implementing proper capacity management and bounds checking. The changes enable RuntimeAsync by default and add comprehensive tests to verify the new method capacity limits.
- Implements robust capacity doubling for async methods (up to MAX_SLOT_INDEX - 1 = 65524)
- Adds runtime bounds checking before method insertion to prevent array overflow
- Enables RuntimeAsync by default for CoreCLR and enables corresponding tests
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/methodtablebuilder.cpp | Replaces conditional capacity doubling with robust upfront capacity calculation clamped to MAX_SLOT_INDEX - 1, and adds bounds check before method insertion to throw TypeLoadException when limit is exceeded |
| src/coreclr/utilcode/allocmemtracker.cpp | Increases consistency check threshold from 10,000 to 60,000 blocks to accommodate larger classes with up to 65,524 methods |
| src/coreclr/inc/clrconfigvalues.h | Enables RuntimeAsync feature by default (changes default value from 0 to 1) |
| src/tests/async/Directory.Build.targets | Removes the DisableProjectBuild condition to enable async tests for CoreCLR (non-nativeaot) builds |
| src/tests/async/capacity/capacity.csproj | Defines new test project structure including test file and three generated test class files |
| src/tests/async/capacity/capacity.cs | Adds three test scenarios: 40,000 int-returning methods (should succeed), 32,750 Task-returning methods (should succeed), and 32,763 Task-returning methods (should fail with TypeLoadException) |
jkotas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise. Thanks!
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
This reverts commit 6975252.
|
/ba-g failures in LibraryImportGenerator are all #122447 |
|
Thanks! |
Fixes: #121760
To make the insertion of methods more robust we:
MethodDesc.Note: it may result in some programs that used to run to start throwing
TypeLoadException.These would be extreme cases though. The capacity for simple methods is implementation-specific. In CoreCLR a type cannot contain more than 65524 methods due to internal limitations.
(More complex cases involving inheritance/virtual methods may have additional limits.)
With runtime async, methods that return
Task/ValueTaskmay consume 2 methods instead of 1 from this 65524 budget.In a worst case where all methods return
Task, the limit becomes 32762 methods in a single type, which still seems a very high limit for reasonably real programs that are not generated testcases.