Remove asserts that were firing during rejit tests #33492
Conversation
|
I couldn't add an area label to this PR. Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future. |
src/coreclr/src/vm/callcounting.cpp
Outdated
| @@ -448,7 +448,7 @@ bool CallCountingManager::IsCallCountingEnabled(NativeCodeVersion codeVersion) | |||
| CONTRACTL_END; | |||
|
|
|||
| _ASSERTE(!codeVersion.IsNull()); | |||
| _ASSERTE(codeVersion.IsDefaultVersion()); | |||
| _ASSERTE(codeVersion.IsDefaultVersion() || codeVersion.GetILCodeVersionId() != 0); | |||
There was a problem hiding this comment.
This function shouldn't be called for non-default native code versions. It effectively is a storage location just for the default code version that affects the tier for the default code version. Non-default code versions have a storage location for the tier. It looks like this caller would be the issue (there are only two callers):
runtime/src/coreclr/src/vm/prestub.cpp
Lines 1051 to 1056 in 2b27323
That assert is actually unnecessary since the assert above it checks for tier 0, which implies what is intended to be verified. Could remove that assert instead (including the call to this function) and the only other call to this function should be fine. Do you have the stack trace of the failure to confirm? If you can take care of it then great, otherwise I'm working on a fix and I can include this fix in that, let me know.
There was a problem hiding this comment.
The issue I saw is that non-default IL versions will have a tier 0 that returns false for NativeCodeVersion::IsDefaultVersion. I haven't seen any evidence that it's called for tier 1 methods
There was a problem hiding this comment.
I don't have the stack trace any more, I can get it but it would have to be later today
There was a problem hiding this comment.
Yea makes sense. It would be great if you could confirm that as the cause, I'll go ahead and remove the assert in my change anyway. Also can you point me to the test that is failing?
- When QuickJit is disabled, the initial tier is Optimized instead of the correct Tier0. This causes assertion failures as tiering tries to count calls and promote the method to Tier1. - Does not appear to be an issue in release builds, as the methods are still call-counted and promoted despite the incorrect tier - Add some basic tiering tests for config modes that are exposed and supported through <app>.runtimeconfig.json, QuickJit and QuickJitForLoops, when on and off - Removed an invalid and redundant assertion that was causing a profiler rejit test to fail, see dotnet#33492 (comment). What the assertion was intending to verify is already verified by an assertion above it that checks the tier, which also covers the default native code version case.
ghost
locked as resolved and limited conversation to collaborators
#32250 had some asserts that would fire under certain situations with our rejit tests. The rejit path no longer suspends the runtime so that assert was removed, and call counting can happen for a non default code version with rejit since the IL is updated, so that assert was updated.