-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
GC cleanup #105517
base: main
Are you sure you want to change the base?
GC cleanup #105517
Conversation
remove local logging undo line
Tagging subscribers to this area: @dotnet/gc |
please do not merge. I also have some cleanup stuff that I'll include. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I actually don't have time to do the cleanup changes I wanted to add due to my OOF. but could you please also add a change that rename the DYNAMIC_HEAP_COUNT
define to DYNAMIC_ADAPTATION_TO_APP
? since DATAS (and I imagine in the future we'd want to adapt to other things other than size) is not just limited to heap count changes.
Can we also remove this obsolete comment? // REGIONS TODO: we should reserve enough space at the end of what we reserved that's
// big enough to accommodate if we were to materialize all the GC bookkeeping datastructures.
// We only need to commit what we use and just need to commit more instead of having to
// relocate the existing table and then calling copy_brick_card_table.
// Right now all the non mark array portions are commmitted since I'm calling make_card_table
// on the whole range. This can be committed as needed. |
97114c5
to
347486d
Compare
476bc1c
to
0c4a3a9
Compare
|
||
heap_budget_in_region_units[i][basic_free_region] = 0; | ||
min_heap_budget_in_region_units[i] = 0; | ||
heap_budget_in_region_units[i][large_free_region] = 0; | ||
} | ||
|
||
dprintf (1, ("moved %2zd regions (%8zd) to decommit based on time", num_decommit_regions_by_time, size_decommit_regions_by_time)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dprintf(REGIONS_LOG)
@@ -14303,12 +14305,6 @@ HRESULT gc_heap::initialize_gc (size_t soh_segment_size, | |||
#ifdef USE_REGIONS | |||
if (regions_range) | |||
{ | |||
// REGIONS TODO: we should reserve enough space at the end of what we reserved that's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not obsolete - not reserving a range for this today
(see make_card_table)
add "no functional changes" to title and wait for regions work ("region units" changes won't be needed after that) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than the few trivial things you took notes off, the rest LGTM!
transfer_regions
totransfer_regions_from
dprintf
inget_region_info
dprintf
nearer to the relevant codedprintf
s