Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@ghost
Copy link

@ghost ghost commented Jul 11, 2019

PR #25350 changed fgn_maxgen_percent to be a per-heap property when
MULTIPLE_HEAPS is set. A few uses need to be updated.

  • In full_gc_wait, must re-read fgn_maxgen_percent before the
    second test of maxgen_percent == 0.
    (Otherwise the second test is statically unreachable.)
  • In RegisterForFullGCNotification, must set fgn_maxgen_percent when
    MULTIPLE_HEAPS is not set
  • In CancelFullGCNotification, must set fgn_maxgen_percent for each
    heap separately when MULTIPLE_HEAPS is set.

Fix dotnet/corefx#39374

PR #25350 changed `fgn_maxgen_percent` to be a per-heap property when
`MULTIPLE_HEAPS` is set. A few uses need to be updated.

* In `full_gc_wait`, must re-read `fgn_maxgen_percent` before the
  second test of `maxgen_percent == 0`.
  (Otherwise the second test is statically unreachable.)
* In RegisterForFullGCNotification, must set `fgn_maxgen_percent` when
  `MULTIPLE_HEAPS` is not set
* In CancelFullGCNotification, must set `fgn_maxgen_percent` for each
  heap separately when `MULTIPLE_HEAPS` is set.

Fix dotnet/corefx#39374
@ghost ghost requested a review from Maoni0 July 11, 2019 19:35
src/gc/gc.cpp Outdated
if ((wait_result == WAIT_OBJECT_0) || (wait_result == WAIT_TIMEOUT))
{
#ifdef MULTIPLE_HEAPS
maxgen_percent = g_heaps[0]->fgn_maxgen_percent;
Copy link
Member

Choose a reason for hiding this comment

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

maxgen_percent = g_heaps[0]->fgn_maxgen_percent; [](start = 7, length = 49)

maxgen_percent was already read at the beginning of this method.

Copy link
Member

Choose a reason for hiding this comment

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

oh I see your comment from above - is it because the 1st time we read the value we observe its not set yet?

Copy link
Author

Choose a reason for hiding this comment

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

The operation may be canceled while we're waiting, and CancelFullGCNotification works by setting fgn_maxgen_percent.

Copy link
Member

Choose a reason for hiding this comment

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

ahh right; we still wanna return the right status so we need to re-read. makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

btw we could also write the code this way (like what the other places are doing) so you don't have to duplicate the code:

#ifdef MULTIPLE_HEAPS
    gc_heap* hp = gc_heap::g_heaps[0];
#else
    gc_heap* hp = pGenGCHeap;
#endif //MULTIPLE_HEAPS

if (hp->fgn_maxgen_percent == 0)

@ghost
Copy link
Author

ghost commented Jul 11, 2019

Simplified test case:

using System;
using System.Threading;

namespace cs
{
    class Program
    {
        public static int Main(string[] args)
        {
        	Console.WriteLine($"Server gc? {System.Runtime.GCSettings.IsServerGC}");
            GC.RegisterForFullGCNotification(20, 20);
            Thread cancelProc = new Thread(new ThreadStart(CancelProc));
            cancelProc.Start();
            GCNotificationStatus result = GC.WaitForFullGCComplete(-1);
            cancelProc.Join();
            if (result == GCNotificationStatus.Canceled)
            {
            	Console.WriteLine("success");
            	return 0;
            }
            else
            {
                Console.Error.WriteLine($"Error - WaitForFullGCComplete result not Cancelled. Instead result is: {result}");
                return 1;
            }
        }

        private static void CancelProc()
        {
            Thread.Sleep(500);
            GC.CancelFullGCNotification();
        }
    }
}

Tested with both server and workstation GCs.

@ghost ghost requested a review from MeiChin-Tsai July 11, 2019 20:37
@MeiChin-Tsai
Copy link

approved to merge.

@ghost ghost merged commit 4b5ae70 into dotnet:master Jul 12, 2019
@ghost ghost deleted the fgn_maxgen_percent_fixes branch July 12, 2019 23:21
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Fixes when accessing fgn_maxgen_percent

PR dotnet/coreclr#25350 changed `fgn_maxgen_percent` to be a per-heap property when
`MULTIPLE_HEAPS` is set. A few uses need to be updated.

* In `full_gc_wait`, must re-read `fgn_maxgen_percent` before the
  second test of `maxgen_percent == 0`.
  (Otherwise the second test is statically unreachable.)
* In RegisterForFullGCNotification, must set `fgn_maxgen_percent` when
  `MULTIPLE_HEAPS` is not set
* In CancelFullGCNotification, must set `fgn_maxgen_percent` for each
  heap separately when `MULTIPLE_HEAPS` is set.

Fix dotnet/corefxdotnet/coreclr#39374

* Avoid duplicate code when getting fgn_maxgen_percent twice in full_gc_wait


Commit migrated from dotnet/coreclr@4b5ae70
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GCNotificationTests outerloop tests consistently failing on all platforms

2 participants