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

Make GC stress usable with Libraries tests #103738

Merged
merged 11 commits into from
Jul 3, 2024
Merged

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Jun 19, 2024

Fixes: #101150

Also useful to investigate failures like: #102919

  • Speed up GC stress with GCSTRESS_INSTR_JIT
  • Disable GC stress when WaitForPendingFinalizers is in progress
  • Do not initiate GC stress when thread is in preemptive mode

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@VSadov
Copy link
Member Author

VSadov commented Jun 26, 2024

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@dotnet dotnet deleted a comment from azure-pipelines bot Jun 26, 2024
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Jun 26, 2024
@VSadov
Copy link
Member Author

VSadov commented Jun 26, 2024

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@VSadov
Copy link
Member Author

VSadov commented Jun 26, 2024

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@VSadov VSadov marked this pull request as ready for review June 27, 2024 20:51
@VSadov VSadov requested a review from cshung June 27, 2024 20:53
@jkotas jkotas requested a review from janvorli June 27, 2024 20:54
@VSadov
Copy link
Member Author

VSadov commented Jul 1, 2024

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

// Under GC stress the finalizer queue may never go empty as frequent
// GCs will keep filling up the queue with items.
// We will disable GC stress to make sure the current thread is not permanently blocked on that.
GCStressPolicy::InhibitHolder iholder;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InhibitHolder increments global counter without using Interlocked. We may want to fix to avoid race conditions that can make this inhibit holder ineffective intermittently and cause hangs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not realize that. This is dangerous and may be already causing some rare failures in stress.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

src/coreclr/vm/gcstress.h Outdated Show resolved Hide resolved
src/coreclr/vm/gcstress.h Outdated Show resolved Hide resolved
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
//----------------------------------------------------

DWORD status = hEventFinalizerDone->Wait(INFINITE,TRUE);
_ASSERTE(status == WAIT_OBJECT_0);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This place does not complain about unused local.

Copy link
Member Author

@VSadov VSadov Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I have seen before that this can break the build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the other case is included somewhere that is built with more strict rules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that this does not complain because DWORD is typedef, so it is a potentially complex type where the local variable may be impactful ... until some compiler decides to warn for this case too.

I would not hurt to change this one to the make-compiler-happy pattern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have quite a few of these. Fixed this one.

@VSadov VSadov merged commit f2dfdf6 into dotnet:main Jul 3, 2024
92 of 95 checks passed
@VSadov
Copy link
Member Author

VSadov commented Jul 3, 2024

Thanks!!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate ways to make GC stress compatible with WaitForPendingFinalizers
2 participants