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

Some improvements to RF #56659

Merged
merged 1 commit into from
Aug 17, 2021
Merged

Some improvements to RF #56659

merged 1 commit into from
Aug 17, 2021

Conversation

Maoni0
Copy link
Member

@Maoni0 Maoni0 commented Jul 31, 2021

usually I use RF to see if I get AVs, but with regions it's uncertain if tests are just taking a very long time.

  • added a maximumWaitTime config default to 10 mins
  • actually breaks into the debugger when it times out when debugBreakOnTestHang is specified
  • prints out the progress while we wait so it's obvious if it's hung or just because some
    tests are taking a long time. example output -
[7/30/2021 6:52:26 PM 1] ============current number of tests running   14, allocated 50,040,808,336 so far, 879,122,336 since last; (GC 439:420:420), waited 55s
[7/30/2021 6:52:26 PM 1] Still running: DirectedGraph.dll
[7/30/2021 6:52:26 PM 1] Still running: LargeObjectAlloc.dll
[7/30/2021 6:52:26 PM 1] Still running: LargeObjectAlloc1.dll
[7/30/2021 6:52:29 PM 1] Still running: LargeObjectAlloc2.dll

@ghost
Copy link

ghost commented Jul 31, 2021

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

Issue Details

usually I use RF to see if I get AVs, but with regions it's uncertain if tests are just taking a very long time.

  • added a maximumWaitTime config default to 10 mins
  • actually breaks into the debugger when it times out when debugBreakOnTestHang is specified
  • prints out the progress while we wait so it's obvious if it's hung or just because some
    tests are taking a long time. example output -
[7/30/2021 6:52:26 PM 1] ============current number of tests running   14, allocated 50,040,808,336 so far, 879,122,336 since last; (GC 439:420:420), waited 55s
[7/30/2021 6:52:26 PM 1] Still running: DirectedGraph.dll
[7/30/2021 6:52:26 PM 1] Still running: LargeObjectAlloc.dll
[7/30/2021 6:52:26 PM 1] Still running: LargeObjectAlloc1.dll
[7/30/2021 6:52:29 PM 1] Still running: LargeObjectAlloc2.dll
Author: Maoni0
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

Copy link
Member

@cshung cshung left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -661,20 +659,11 @@ private static void NoExitPoll()
}
internal static void MyDebugBreak(string extraData)
{
#if !PROJECTK_BUILD
Copy link
Member

Choose a reason for hiding this comment

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

Do we still want to keep the variant where PROJECTK_BUILD is not defined?
It looks like the code won't compile (with or without your change) without PROJECTK_BUILD defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I thought about just getting rid of the define (ie, it's always defined) since I don't see any value of maintaining it. but didn't do it with this PR... feel free to do that if you like.

waitCnt++;
}
}

// let the user know what tests haven't finished...
if (_testsRunningCount != 0)
{
string msg;

string msg = String.Format("************Timeout reached************");
Copy link
Member

Choose a reason for hiding this comment

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

The call to String.Format is unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

right 😁 that's from some copying/pasting. will get rid of it

_testsRunningCount, currentAllocatedBytes, (currentAllocatedBytes - lastAllocatedBytes),
GC.CollectionCount(0),
GC.CollectionCount(2),
GC.CollectionCount(2),
Copy link

@IAmTheCShark IAmTheCShark Aug 17, 2021

Choose a reason for hiding this comment

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

You called GC.CollectionCount(2) twice, looks like one should use 1 as an argument, or am I wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed it should be 1, thanks for spotting it!! 👍

+ added a maximumWaitTime config default to 10 mins
+ actually breaks into the debugger when it times out when debugBreakOnTestHang is specified
+ prints out the progress while we wait so it's obvious if it's hung or just because some
  tests are taking a long time. example output -

[7/30/2021 6:52:26 PM 1] ============current number of tests running   14, allocated 50,040,808,336 so far, 879,122,336 since last; (GC 439:420:420), waited 55s
[7/30/2021 6:52:26 PM 1] Still running: DirectedGraph.dll
[7/30/2021 6:52:26 PM 1] Still running: LargeObjectAlloc.dll
[7/30/2021 6:52:26 PM 1] Still running: LargeObjectAlloc1.dll
[7/30/2021 6:52:29 PM 1] Still running: LargeObjectAlloc2.dll
@Maoni0
Copy link
Member Author

Maoni0 commented Aug 17, 2021

failures are unrelated.

@Maoni0 Maoni0 merged commit a53ec50 into dotnet:main Aug 17, 2021
@hoyosjs
Copy link
Member

hoyosjs commented Aug 18, 2021

Issues were #57620 and #55449

@Maoni0
Copy link
Member Author

Maoni0 commented Aug 18, 2021

thanks @hoyosjs!

@ghost ghost locked as resolved and limited conversation to collaborators Sep 17, 2021
@Maoni0 Maoni0 deleted the rf_fix branch September 24, 2021 05:55
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.

4 participants