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

ComWrappers test that allocate object can't inline #85583

Merged
merged 2 commits into from
May 2, 2023

Conversation

AaronRobinsonMSFT
Copy link
Member

There are some JIT stress settings that aggressively inline
functions, regardless of size/complexity. This inlining can
perturb the subtle GC assumptions inherent in the COM
wrappers tests.

Fixes #85262

/cc @jkoritzinsky

There are some JIT stress settings that aggressively
inline functions, regardless of size/complexity. This
inlining can perturb the subtle GC assumptions
inherent in the COM wrappers tests.
@AustinWise
Copy link
Contributor

Thanks for fixing. There should probably be a roslyn analyzer that takes you add NoInline everywhere when calling GC.Collect in a test 😁

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 4772b5d into dotnet:main May 2, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the runtime_85262 branch May 2, 2023 10:35
@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented May 2, 2023

There should probably be a roslyn analyzer that takes you add NoInline everywhere when calling GC.Collect in a test

Not sure that is good general policy as it is specific to this test and how it is run. For normal xUnit tests, this wouldn't be much of an issue because the test cases are executed via Reflection. The test pattern under src/tests is more akin to scenario tests and the custom source generator that we have for making them xUnit-like is basically identical to what is in this test's Main.

Calling GC.Collect() in any test, outside of the runtime repo, is likely hiding a bug or is a less-than-ideal architecture in the majority of cases.

@AustinWise
Copy link
Contributor

I was specifically thinking about the couple bugs of this type I've caused in src/tests when trying to write tests for things like critical finializers and ref-counted handles.

Is there a recommended way to catch these sorts of bugs before I submit a PR? Perhaps running the test in a loop with certain GC stress modes enabled? I can't find documentation what modes would be helpful for catching these sorts of problems.

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented May 2, 2023

Is there a recommended way to catch these sorts of bugs before I submit a PR?

Unfortunately not really. A lot of it is experience ex-post since seeing it prior to check-in is hard because of the 1000 other things you are likely worrying about during development. I've personally checked in flakey tests that suffer from this and then had to fix them so you are in good company. The experience part comes into play when you can get the failure and quickly see where the problem is without debugging or flipping on multiple logging tools. For JIT stress the common issue is inlining which can fundamentally change the lifetime of all locals in the inlined frame.

Perhaps running the test in a loop with certain GC stress modes enabled?

Yep, that is something I regularly do and it helps provide confidence on GC holes, but not always issues like this. Running the test in Checked with DOTNET_GCStress=C is typically a good start. The JIT stress side of things is much harder and I simply let the CI find issues and investigate them as they come in.

I wouldn't take any introduced test issues personally, it is hard to get tests right in general and non-determinism like the GC simply exacerbates an already very hard problem space - writing robust and accurate tests.

@AustinWise
Copy link
Contributor

Thanks for the tip about DOTNET_GCStress=C, I'll give it a try next time I'm writing tests that involve the finalizer.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 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.

jitstress-random ComWrappersTestsBuiltInComDisabled is failing
3 participants