GC bookkeeping data structure in-place commit

[cshung]

With a fixed reserved range in the USE_REGIONS case, we can simplify the logic for managing the GC bookkeeping data structures as we grow the heap by doing so in place.

Highlights:

• The code is structured so that the size and layout computation is centralized.
• In the USE_REGIONS case, the data structures are committing much less memory than it was.
• In the case of STRESS_REGIONS, at random times, the data structure is committed right at the level it needs to stress size computation logic.
• I made sure the arguments to virtual_commit are page-aligned.
• In the case of failure to speculatively commit for more memory for the data structure, the code fallbacks to commit less before reporting OOM.

Lowlights:

• It doesn't handle g_mark_list_piece, it will be done in follow-up PRs.
• We do not change the g_gc_highest_address value, it seems unnecessary for the x64 case.

[ghost]

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

Issue Details

---

With a fixed reserved range in the USE_REGIONS case, we can simplify the logic for managing the GC bookkeeping data structures as we grow the heap by doing so in place.

This is a work-in-progress - the goal of this PR right now is looking for early feedback only.

Highlights:

• The code is structured so that the size and layout computation is centralized.
• In the USE_REGIONS case, the data structures are committing less memory than it was.
• It often works (with the caveat about synchronization)

Lowlights:

• We need to change the g_gc_highest_address value (or do we have to, especially in the x64 case?)
• verify_card_bundles is disabled (because g_gc_highest_address is not updated yet but the function walks all cells based on that).
• Proper synchronization is required since the code is driven by allocation and therefore can be executed concurrently.
• We are committing exactly as needed for now (to stress boundary conditions), but not good for perf if we are frequently committing for a small amount of memory (or does it?)
• Namings need to be improved.
• Encapsulation is broken :(

Author:	cshung
Assignees:	-
Labels:	area-GC-coreclr
Milestone:	-

[Maoni0]

I would definitely not use the flat a, b, c.... to keep track of layout. it seems awkward. you can have an enum for each type of element the bookkeeping has and an array that keeps track of where each one starts in the array and commit as needed. pseudo code -

enum bookkeeping_element
{
    card_table_element = 0,
    brick_table_element = 1,
    ...
    mark_array_element = n,
    total_bookkeeping_elements = n + 1
};

struct bookkeeping_element_info
{
    // this is where this particular element starts in our reserved memory for bookkeeping.
    // just for recording purposes. 
    uint8_t* start;
    // this is how much we've committed so far. 
    uint8_t* committed;
};

uint8_t* bookkeeping_committed[total_bookkeeping_elements];

size_t calc_bookkeeping_element_size (uint8_t* start, uint8_t* end)
{
    size_t element_size = 0;
    switch (i)
    {
        case card_table_element:
            element_size = size_card_of (start, end);
        case brick_table_element:
            element_size = size_brick_of (start, end);
        ...
    }

    return element_size;
}

void in_place_bookkeeping_commit (uint8_t* old_used, uint8_t* new_used)
{
    for (int i = 0; i < total_bookkeeping_elements; i++)
    {
        size_t commit_inc = calc_bookkeeping_element_size (old_used, new_used);

        if (!commit_inc)
        {
            virtual_commit (bookkeeping_committed[i], commit_inc);
            gc_bookkeeping[i] += commit_inc;
        }
    }
}

for initial reserve (you do need to commit the first page for card_table_info) you can call calc_bookkeeping_element_size, get the total size and populate committed for each one without actually committing.

[Maoni0]

I don't think you need to do this under a lock unless you are using an OS with a memory manager that can't commit already committed memory like on Nintendo Switch (both windows and linux can handle it just now). you could have 2 threads both commit up to their used, then use an interlocked compare exchange to update used to a new value.

also you are not using the same policy as grow_brick_card_tables which is to extend it more than strictly needed to avoid having to commit again. I think that's a good policy that should be kept especially in the region case as we get regions a lot more frequently than segments.

[cshung]

I don't think you need to do this under a lock unless you are using an OS with a memory manager that can't commit already committed memory like on Nintendo Switch (both windows and linux can handle it just now). you could have 2 threads both commit up to their used, then use an interlocked compare exchange to update used to a new value.

Thanks for the suggestion, this definitely will help with perf. Although I need to think more about that. It looks like we will be double counting the commit towards the commit limit if we do that.

As per offline discussion, this is non-trivial and we can always optimize later.

also you are not using the same policy as grow_brick_card_tables which is to extend it more than strictly needed to avoid having to commit again. I think that's a good policy that should be kept especially in the region case as we get regions a lot more frequently than segments.

This is currently being tracked as point 4 under lowlights (in the initial PR comment). The intention, for now, is to make sure the calculation is correct by committing exactly as required so that test could reveal if there are any out-of-bound accesses. I think I will want to keep this current mode under STRESS_REGIONS and implement the policy to commit more than necessary when I am convinced the code is working.

It is done.

[Maoni0]

this looks good to me in general. as we discussed, please verify for segments, all the sizes/alignments (reserved/committed) should be the same except for mark_array (and without the page alignment mark_array should also have the same size) in both make_card_table and grow_brick_card_tables.

[Maoni0]

LGTM!