Skip to content

Conversation

@cshung
Copy link
Contributor

@cshung cshung commented Oct 19, 2021

The change is supposed to change the behavior for USE_REGIONS only.

Overall idea

We may not have a continuous small object heap, therefore I removed the background_min_overflow_address and background_max_overflow_address. Instead, I introduced new flag named background_overflow_p to indicate a background mark stack overflow happened for the current heap, and the flag heap_segment_flags_overflow defined on a region is repurposed to mean any regions that need mark overflow processing.

Details:

When a background mark stack overflow happens. Instead of modifying the background overflow address range, we set the heap_segment_flags_overflow flag on the corresponding region and the background_overflow_p flag on the corresponding heap.

Without a range, we have to scan the whole region.

background_first_overflow is changed for regions so that it starts at the beginning of the region.

The only important change in background_process_mark_overflow_internal is that we are ignoring the input and setting current_min_add and current_max_add to the region's range when heap_segment_flags_overflow flag is on and (0,0) otherwise. Other changes only affect dprintfs.

In background_process_mark_overflow, preprocessing for the concurrent case, we removed the logic of setting the heap_segment_flags_overflow for ephemeral regions. The flag is no longer used for that purpose.

In background_process_mark_overflow, preprocessing for the non-concurrent case, we used the background_overflow_p flag instead of the range to detect a background mark stack overflow happened. The setting of background_overflow_p flag to true unconditionally is a bit tricky. That is meant to make sure we process the ephemeral regions that we skipped earlier, there isn't a separate indicator for whether or not there was a mark overflow happened in the last on the ephemeral regions.

The last change in background_process_mark_overflow simply pass (0, 0) as the range into background_process_mark_overflow_internal, the function will simply not use them.

We skipped the logic to compute the overflow range in background_scan_dependent_handles and in background_mark_phase.

Testing

The latest code is tested in the following configuration(s):

Test OS Build Configuration Duration Status
ReliabilityFramework Windows Debug Server GC 12 hours Passed without crash
ReliabilityFramework Windows Optimized Server GC with induced mark stack overflow 18 hours Passed without crash

@ghost ghost added the area-GC-coreclr label Oct 19, 2021
@ghost
Copy link

ghost commented Oct 19, 2021

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

Issue Details

It is a prototype, it seems to work with a test case with mark overflow. Here are some highlights of what I changed:

  • Instead of maintaining a range containing the overflowed objects (i.e. object that is marked but the children may not), its corresponding region is marked as heap_segment_flags_overflow instead.
  • A single flag background_overflowed is maintained per heap so that I don't need to iterate through all regions to check if there is an overflowed region.
  • We simply always pass (0, 0) as the range to background_process_mark_overflow_internal. The range is simply not used. In concurrent case, we process all regions that is marked as heap_segment_flags_overflow which is also not ephemeral. In the concurrent case, we process all the ephemeral regions in addition to that.

This should work, but I am not certain about its perf impact. Depending on the exact situation, it may or may not outperform the range solution.

Author: cshung
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@cshung cshung force-pushed the public/region-overflow branch 4 times, most recently from 9a13ccc to 1b935bb Compare November 18, 2021 04:28
@cshung cshung force-pushed the public/region-overflow branch from c8e512c to 45dec0f Compare December 7, 2021 00:08
@cshung cshung changed the title Prototype mark overflow fix for regions Mark overflow fix for regions Dec 7, 2021
@cshung cshung changed the title Mark overflow fix for regions [WIP] Mark overflow fix for regions Dec 7, 2021
@cshung cshung changed the title [WIP] Mark overflow fix for regions Mark overflow fix for regions Dec 7, 2021
Copy link
Member

@Maoni0 Maoni0 left a comment

Choose a reason for hiding this comment

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

sorry I didn't get to this yesterday like I said I would...

it might be worthwhile to think about some kind of formatting tool that can make the format for the #ifdef checks consistent so we don't have to go through formatting stuff.

otherwise LGTM. I still think there might be a problem on arm but it's fine to check this in - it might help us discover some problems on arm by CI (I doubt it though since very little is run with server GC in CI).

@cshung cshung merged commit 06bb200 into dotnet:main Dec 10, 2021
@cshung cshung deleted the public/region-overflow branch December 10, 2021 23:15
@ghost ghost locked as resolved and limited conversation to collaborators Jan 10, 2022
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.

2 participants