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.

Sign up for GitHub

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

Ensure that WaitForPendingFinalizers has seen the expected Full GC count #105289

Merged
merged 6 commits into from
Jul 25, 2024
Merged

Ensure that WaitForPendingFinalizers has seen the expected Full GC count #105289

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
12aeee5
Ensure that WaitForPendingFinalizers has seen the expected Full GC
VSadov Jul 23, 2024
334f1c7
NativeAOT and some renames
VSadov Jul 23, 2024
f6b5acf
a testcase
VSadov Jul 23, 2024
55830b1
make the test not unsafe and make OuterLoop
VSadov Jul 24, 2024
d1d2b7e
Use unsigned math when comparing collection ticks
VSadov Jul 24, 2024
c2b8cf9
cast the diff to int when comparing gc ticks
VSadov Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,10 @@ EXTERN_C void QCALLTYPE RhWaitForPendingFinalizers(UInt32_BOOL allowReentrantWai
// Wait for the finalizer thread to get back to us.
g_FinalizerDoneEvent.Wait(INFINITE, false, allowReentrantWait);

if (desiredFullGcCount - g_fullGcCountSeenByFinalization > 0)
// we use unsigned math here as the collection counts, which are size_t internally,
// can in theory overflow an int and wrap around.
// unsigned math would have more defined/portable behavior in such case
if ((unsigned int)desiredFullGcCount - (unsigned int)g_fullGcCountSeenByFinalization > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to cast the result back to int?

unsigned minus unsigned is going to be unsigned. It means that the current version is same as if (desiredFullGcCount != g_fullGcCountSeenByFinalization).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think I need a cast to int. The result of unsigned subtraction is indeed unsigned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I think we see that in gcstress taking long time.

Copy link
Member Author

@VSadov VSadov Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is relatively fast to run gcstress these days (about 1.5 - 2 hours), but in this PR it's been 3+ hours and nothing completed yet.

{
// There were some Full GCs happening before we started waiting and possibly not seen by the
// last finalization cycle. This is rare, but we need to be sure we have seen those,
Expand Down
5 changes: 4 additions & 1 deletion src/coreclr/vm/finalizerthread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,10 @@ void FinalizerThread::FinalizerThreadWait()
DWORD status;
status = hEventFinalizerDone->Wait(INFINITE,TRUE);

if (desiredFullGcCount - g_fullGcCountSeenByFinalization > 0)
// we use unsigned math here as the collection counts, which are size_t internally,
// can in theory overflow an int and wrap around.
// unsigned math would have more defined/portable behavior in such case
if ((unsigned int)desiredFullGcCount - (unsigned int)g_fullGcCountSeenByFinalization > 0)
{
// There were some Full GCs happening before we started waiting and possibly not seen by the
// last finalization cycle. This is rare, but we need to be sure we have seen those,
Expand Down
27 changes: 16 additions & 11 deletions src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,9 +293,9 @@ private class TestObject
}
}

// [OuterLoop] // TODO: VS uncomment before merging
[OuterLoop]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
public unsafe static void WaitForPendingFinalizersRaces()
public static void WaitForPendingFinalizersRaces()
{
Task.Run(Test);
Task.Run(Test);
Expand All @@ -307,34 +307,39 @@ public unsafe static void WaitForPendingFinalizersRaces()

static void Test()
{
for (int i = 0; i < 2000; i++)
for (int i = 0; i < 20000; i++)
{
bool finalized = false;
MakeAndNull(&finalized);
BoxedFinalized flag = new BoxedFinalized();
MakeAndNull(flag);
GC.Collect();
GC.WaitForPendingFinalizers();
Assert.True(finalized);
Assert.True(flag.finalized);
}
}

[MethodImpl(MethodImplOptions.NoInlining)]
static void MakeAndNull(bool* flag)
static void MakeAndNull(BoxedFinalized flag)
{
var deadObj = new TestObjectWithFinalizer(flag);
// it's dead here
};
}

unsafe class TestObjectWithFinalizer
class BoxedFinalized
{
public bool finalized;
}

class TestObjectWithFinalizer
{
bool* _flag;
BoxedFinalized _flag;

public TestObjectWithFinalizer(bool* flag)
public TestObjectWithFinalizer(BoxedFinalized flag)
{
_flag = flag;
}

~TestObjectWithFinalizer() => *_flag = true;
~TestObjectWithFinalizer() => _flag.finalized = true;
}

[Fact]
Expand Down
Loading