Make sure we at least sweep the loh even when we wanted to compact it

[cshung]

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

[ghost]

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

Issue Details

---

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

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

[Maoni0]

we shouldn't be reverting the loh_compacted_p here because we would have already done the work of planning loh earlier which would be pointless to do.

I think we should move the code that does plan_loh to be after we've completely made the decision whether we will sweep or compact, and only check settings.loh_compaction if special_sweep_p is not on.

 if (condemned_gen_number == max_generation)
{
#ifdef FEATURE_LOH_COMPACTION
if (settings.loh_compaction)
{
if (plan_loh())
{
should_compact = TRUE;
get_gc_data_per_heap()->set_mechanism (gc_heap_compact, compact_loh_forced);
loh_compacted_p = TRUE;
}
}
else
{
if ((heap_number == 0) && (loh_pinned_queue))
{
loh_pinned_queue_decay--;
if (!loh_pinned_queue_decay)
{
delete loh_pinned_queue;
loh_pinned_queue = 0;
}
}
}
if (!loh_compacted_p)
#endif //FEATURE_LOH_COMPACTION
{
GCToEEInterface::DiagWalkUOHSurvivors(__this, loh_generation);
sweep_uoh_objects (loh_generation);
}
GCToEEInterface::DiagWalkUOHSurvivors(__this, poh_generation);
sweep_uoh_objects (poh_generation);
}
else
{
settings.loh_compaction = FALSE;
}

I noticed this suspicious line -

if ((heap_number == 0) && (loh_pinned_queue))

this should be done for all heaps, not just heap0. I think when I wrote this code I meant just for the decay to be reduced on heap0 but used on all heaps. but that's just not worth it. we might as well make loh_pinned_queue_decay a per heap member and do this for all heaps (they should all have the same value anyway).

[Maoni0]

accidently approved it... was meant for another PR of yours

[cshung]

Move the deletion of the pin queue later as well

[cshung]

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

[ghost]

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

Issue Details

---

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

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

[Maoni0]

we shouldn't be reverting the loh_compacted_p here because we would have already done the work of planning loh earlier which would be pointless to do.

I think we should move the code that does plan_loh to be after we've completely made the decision whether we will sweep or compact, and only check settings.loh_compaction if special_sweep_p is not on.

 if (condemned_gen_number == max_generation)
{
#ifdef FEATURE_LOH_COMPACTION
if (settings.loh_compaction)
{
if (plan_loh())
{
should_compact = TRUE;
get_gc_data_per_heap()->set_mechanism (gc_heap_compact, compact_loh_forced);
loh_compacted_p = TRUE;
}
}
else
{
if ((heap_number == 0) && (loh_pinned_queue))
{
loh_pinned_queue_decay--;
if (!loh_pinned_queue_decay)
{
delete loh_pinned_queue;
loh_pinned_queue = 0;
}
}
}
if (!loh_compacted_p)
#endif //FEATURE_LOH_COMPACTION
{
GCToEEInterface::DiagWalkUOHSurvivors(__this, loh_generation);
sweep_uoh_objects (loh_generation);
}
GCToEEInterface::DiagWalkUOHSurvivors(__this, poh_generation);
sweep_uoh_objects (poh_generation);
}
else
{
settings.loh_compaction = FALSE;
}

I noticed this suspicious line -

if ((heap_number == 0) && (loh_pinned_queue))

this should be done for all heaps, not just heap0. I think when I wrote this code I meant just for the decay to be reduced on heap0 but used on all heaps. but that's just not worth it. we might as well make loh_pinned_queue_decay a per heap member and do this for all heaps (they should all have the same value anyway).

[Maoni0]

accidently approved it... was meant for another PR of yours

[cshung]

Move the deletion of the pin queue later as well

[cshung]

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

[ghost]

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

Issue Details

---

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

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

[Maoni0]

we shouldn't be reverting the loh_compacted_p here because we would have already done the work of planning loh earlier which would be pointless to do.

I think we should move the code that does plan_loh to be after we've completely made the decision whether we will sweep or compact, and only check settings.loh_compaction if special_sweep_p is not on.

 if (condemned_gen_number == max_generation)
{
#ifdef FEATURE_LOH_COMPACTION
if (settings.loh_compaction)
{
if (plan_loh())
{
should_compact = TRUE;
get_gc_data_per_heap()->set_mechanism (gc_heap_compact, compact_loh_forced);
loh_compacted_p = TRUE;
}
}
else
{
if ((heap_number == 0) && (loh_pinned_queue))
{
loh_pinned_queue_decay--;
if (!loh_pinned_queue_decay)
{
delete loh_pinned_queue;
loh_pinned_queue = 0;
}
}
}
if (!loh_compacted_p)
#endif //FEATURE_LOH_COMPACTION
{
GCToEEInterface::DiagWalkUOHSurvivors(__this, loh_generation);
sweep_uoh_objects (loh_generation);
}
GCToEEInterface::DiagWalkUOHSurvivors(__this, poh_generation);
sweep_uoh_objects (poh_generation);
}
else
{
settings.loh_compaction = FALSE;
}

I noticed this suspicious line -

if ((heap_number == 0) && (loh_pinned_queue))

this should be done for all heaps, not just heap0. I think when I wrote this code I meant just for the decay to be reduced on heap0 but used on all heaps. but that's just not worth it. we might as well make loh_pinned_queue_decay a per heap member and do this for all heaps (they should all have the same value anyway).

[Maoni0]

accidently approved it... was meant for another PR of yours

[cshung]

Move the deletion of the pin queue later as well

[cshung]

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

[ghost]

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

Issue Details

---

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

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

[Maoni0]

we shouldn't be reverting the loh_compacted_p here because we would have already done the work of planning loh earlier which would be pointless to do.

I think we should move the code that does plan_loh to be after we've completely made the decision whether we will sweep or compact, and only check settings.loh_compaction if special_sweep_p is not on.

 if (condemned_gen_number == max_generation)
{
#ifdef FEATURE_LOH_COMPACTION
if (settings.loh_compaction)
{
if (plan_loh())
{
should_compact = TRUE;
get_gc_data_per_heap()->set_mechanism (gc_heap_compact, compact_loh_forced);
loh_compacted_p = TRUE;
}
}
else
{
if ((heap_number == 0) && (loh_pinned_queue))
{
loh_pinned_queue_decay--;
if (!loh_pinned_queue_decay)
{
delete loh_pinned_queue;
loh_pinned_queue = 0;
}
}
}
if (!loh_compacted_p)
#endif //FEATURE_LOH_COMPACTION
{
GCToEEInterface::DiagWalkUOHSurvivors(__this, loh_generation);
sweep_uoh_objects (loh_generation);
}
GCToEEInterface::DiagWalkUOHSurvivors(__this, poh_generation);
sweep_uoh_objects (poh_generation);
}
else
{
settings.loh_compaction = FALSE;
}

I noticed this suspicious line -

if ((heap_number == 0) && (loh_pinned_queue))

this should be done for all heaps, not just heap0. I think when I wrote this code I meant just for the decay to be reduced on heap0 but used on all heaps. but that's just not worth it. we might as well make loh_pinned_queue_decay a per heap member and do this for all heaps (they should all have the same value anyway).

[Maoni0]

accidently approved it... was meant for another PR of yours

[cshung]

Move the deletion of the pin queue later as well

[cshung]

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

[ghost]

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

Issue Details

---

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

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

[Maoni0]

we shouldn't be reverting the loh_compacted_p here because we would have already done the work of planning loh earlier which would be pointless to do.

I think we should move the code that does plan_loh to be after we've completely made the decision whether we will sweep or compact, and only check settings.loh_compaction if special_sweep_p is not on.

 if (condemned_gen_number == max_generation)
{
#ifdef FEATURE_LOH_COMPACTION
if (settings.loh_compaction)
{
if (plan_loh())
{
should_compact = TRUE;
get_gc_data_per_heap()->set_mechanism (gc_heap_compact, compact_loh_forced);
loh_compacted_p = TRUE;
}
}
else
{
if ((heap_number == 0) && (loh_pinned_queue))
{
loh_pinned_queue_decay--;
if (!loh_pinned_queue_decay)
{
delete loh_pinned_queue;
loh_pinned_queue = 0;
}
}
}
if (!loh_compacted_p)
#endif //FEATURE_LOH_COMPACTION
{
GCToEEInterface::DiagWalkUOHSurvivors(__this, loh_generation);
sweep_uoh_objects (loh_generation);
}
GCToEEInterface::DiagWalkUOHSurvivors(__this, poh_generation);
sweep_uoh_objects (poh_generation);
}
else
{
settings.loh_compaction = FALSE;
}

I noticed this suspicious line -

if ((heap_number == 0) && (loh_pinned_queue))

this should be done for all heaps, not just heap0. I think when I wrote this code I meant just for the decay to be reduced on heap0 but used on all heaps. but that's just not worth it. we might as well make loh_pinned_queue_decay a per heap member and do this for all heaps (they should all have the same value anyway).

[Maoni0]

accidently approved it... was meant for another PR of yours

[cshung]

Move the deletion of the pin queue later as well

[cshung]

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

[ghost]

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

Issue Details

---

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

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

[Maoni0]

we shouldn't be reverting the loh_compacted_p here because we would have already done the work of planning loh earlier which would be pointless to do.

I think we should move the code that does plan_loh to be after we've completely made the decision whether we will sweep or compact, and only check settings.loh_compaction if special_sweep_p is not on.

 if (condemned_gen_number == max_generation)
{
#ifdef FEATURE_LOH_COMPACTION
if (settings.loh_compaction)
{
if (plan_loh())
{
should_compact = TRUE;
get_gc_data_per_heap()->set_mechanism (gc_heap_compact, compact_loh_forced);
loh_compacted_p = TRUE;
}
}
else
{
if ((heap_number == 0) && (loh_pinned_queue))
{
loh_pinned_queue_decay--;
if (!loh_pinned_queue_decay)
{
delete loh_pinned_queue;
loh_pinned_queue = 0;
}
}
}
if (!loh_compacted_p)
#endif //FEATURE_LOH_COMPACTION
{
GCToEEInterface::DiagWalkUOHSurvivors(__this, loh_generation);
sweep_uoh_objects (loh_generation);
}
GCToEEInterface::DiagWalkUOHSurvivors(__this, poh_generation);
sweep_uoh_objects (poh_generation);
}
else
{
settings.loh_compaction = FALSE;
}

I noticed this suspicious line -

if ((heap_number == 0) && (loh_pinned_queue))

this should be done for all heaps, not just heap0. I think when I wrote this code I meant just for the decay to be reduced on heap0 but used on all heaps. but that's just not worth it. we might as well make loh_pinned_queue_decay a per heap member and do this for all heaps (they should all have the same value anyway).

[Maoni0]

accidently approved it... was meant for another PR of yours

[cshung]

Move the deletion of the pin queue later as well

[cshung]

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

[ghost]

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

Issue Details

---

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

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

[Maoni0]

we shouldn't be reverting the loh_compacted_p here because we would have already done the work of planning loh earlier which would be pointless to do.

I think we should move the code that does plan_loh to be after we've completely made the decision whether we will sweep or compact, and only check settings.loh_compaction if special_sweep_p is not on.

 if (condemned_gen_number == max_generation)
{
#ifdef FEATURE_LOH_COMPACTION
if (settings.loh_compaction)
{
if (plan_loh())
{
should_compact = TRUE;
get_gc_data_per_heap()->set_mechanism (gc_heap_compact, compact_loh_forced);
loh_compacted_p = TRUE;
}
}
else
{
if ((heap_number == 0) && (loh_pinned_queue))
{
loh_pinned_queue_decay--;
if (!loh_pinned_queue_decay)
{
delete loh_pinned_queue;
loh_pinned_queue = 0;
}
}
}
if (!loh_compacted_p)
#endif //FEATURE_LOH_COMPACTION
{
GCToEEInterface::DiagWalkUOHSurvivors(__this, loh_generation);
sweep_uoh_objects (loh_generation);
}
GCToEEInterface::DiagWalkUOHSurvivors(__this, poh_generation);
sweep_uoh_objects (poh_generation);
}
else
{
settings.loh_compaction = FALSE;
}

I noticed this suspicious line -

if ((heap_number == 0) && (loh_pinned_queue))

this should be done for all heaps, not just heap0. I think when I wrote this code I meant just for the decay to be reduced on heap0 but used on all heaps. but that's just not worth it. we might as well make loh_pinned_queue_decay a per heap member and do this for all heaps (they should all have the same value anyway).

[Maoni0]

accidently approved it... was meant for another PR of yours

[cshung]

Move the deletion of the pin queue later as well

[cshung]

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

[ghost]

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

Issue Details

---

For regions, it is possible for special_sweep_p to override the behavior of is_compaction_mandatory when all heaps are joining to make the decision whether to sweep or to compact.

This is causing a problem for LOH compaction because we are expecting the LOH is already swept at this point or it will be compacted in the compact_phase, but neither will happen if loh_compacted_p is true but got overridden by special_sweep_p.

This fix will make sure the LOH is swept if that happened. This is important to make sure the mark bits are reset for LOH.

As a side effect, this will walk the LOH survivor after the POH survivors - not sure if the profilers are happy with that.

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

[Maoni0]

we shouldn't be reverting the loh_compacted_p here because we would have already done the work of planning loh earlier which would be pointless to do.

I think we should move the code that does plan_loh to be after we've completely made the decision whether we will sweep or compact, and only check settings.loh_compaction if special_sweep_p is not on.

 if (condemned_gen_number == max_generation)
{
#ifdef FEATURE_LOH_COMPACTION
if (settings.loh_compaction)
{
if (plan_loh())
{
should_compact = TRUE;
get_gc_data_per_heap()->set_mechanism (gc_heap_compact, compact_loh_forced);
loh_compacted_p = TRUE;
}
}
else
{
if ((heap_number == 0) && (loh_pinned_queue))
{
loh_pinned_queue_decay--;
if (!loh_pinned_queue_decay)
{
delete loh_pinned_queue;
loh_pinned_queue = 0;
}
}
}
if (!loh_compacted_p)
#endif //FEATURE_LOH_COMPACTION
{
GCToEEInterface::DiagWalkUOHSurvivors(__this, loh_generation);
sweep_uoh_objects (loh_generation);
}
GCToEEInterface::DiagWalkUOHSurvivors(__this, poh_generation);
sweep_uoh_objects (poh_generation);
}
else
{
settings.loh_compaction = FALSE;
}

I noticed this suspicious line -

if ((heap_number == 0) && (loh_pinned_queue))

this should be done for all heaps, not just heap0. I think when I wrote this code I meant just for the decay to be reduced on heap0 but used on all heaps. but that's just not worth it. we might as well make loh_pinned_queue_decay a per heap member and do this for all heaps (they should all have the same value anyway).

[Maoni0]

accidently approved it... was meant for another PR of yours

[cshung]

Move the deletion of the pin queue later as well