Skip to content
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

Fix finalizer issue with regions #54550

Merged
merged 2 commits into from
Jun 23, 2021

Conversation

PeterSolMS
Copy link
Contributor

This fixes an issue in Server GC where an item in the finalizer queue became stale due to not being relocated.

The problem was that a finalizable object was allocated on one heap, but registered in the finalizer queue of another heap (this is possible due to heap balancing). In CFinalize::UpdatePromotedGenerations, we ask for the generation of an object, and move the object to the correct section of the finalizer queue. In the error case, we obtained the wrong result for the generation of the object because it lived on another heap, and that heap hadn't set the final generation for the region containing the object yet. So we ended up moving the finalizer entry to the section corresponding to gen 2, and missed a relocation of the object occurring in a gen 1 collection afterwards.

Fix: It seems best to make sure an object is always registered for finalization on the heap it's allocated from, so the fix simply fetches the heap from the alloc context after the allocation in the case of SOH, or determines it by calling gc_heap::heap_of in the case of LOH and POH. In the case of SOH, I added an assert to ensure that the heap obtained agrees with the result of calling gc_heap::heap_of.

I also added some dprintf calls to the finalizer logic to aid in future investigations.

… became stale due to not being relocated.

The problem was that a finalizable object was allocated on one heap, but registered in the finalizer queue of another heap (this is possible due to heap balancing). In CFinalize::UpdatePromotedGenerations, we ask for the generation of an object, and move the object to the correct section of the finalizer queue. In the error case, we obtained the wrong result for the generation of the object because it lives on another heap, and that heap hadn't set the final generation for the region containing the object yet.

The fix simply fetches the heap from the alloc context after the allocation. That is the correct heap to register the object on.
… add assert to enforce the invariant that we register the finalizable object on the heap it was allocated on.
@ghost
Copy link

ghost commented Jun 22, 2021

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

Issue Details

This fixes an issue in Server GC where an item in the finalizer queue became stale due to not being relocated.

The problem was that a finalizable object was allocated on one heap, but registered in the finalizer queue of another heap (this is possible due to heap balancing). In CFinalize::UpdatePromotedGenerations, we ask for the generation of an object, and move the object to the correct section of the finalizer queue. In the error case, we obtained the wrong result for the generation of the object because it lived on another heap, and that heap hadn't set the final generation for the region containing the object yet. So we ended up moving the finalizer entry to the section corresponding to gen 2, and missed a relocation of the object occurring in a gen 1 collection afterwards.

Fix: It seems best to make sure an object is always registered for finalization on the heap it's allocated from, so the fix simply fetches the heap from the alloc context after the allocation in the case of SOH, or determines it by calling gc_heap::heap_of in the case of LOH and POH. In the case of SOH, I added an assert to ensure that the heap obtained agrees with the result of calling gc_heap::heap_of.

I also added some dprintf calls to the finalizer logic to aid in future investigations.

Author: PeterSolMS
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@ViktorHofer ViktorHofer reopened this Jun 22, 2021
@PeterSolMS PeterSolMS merged commit ea3f403 into dotnet:main Jun 23, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 23, 2021
…nitial_config

* origin/main: (362 commits)
  [wasm][debugger] Reuse debugger-agent on wasm debugger (dotnet#52300)
  Put Crossgen2 in sync with dotnet#54235 (dotnet#54438)
  Move System.Object serialization to ObjectConverter (dotnet#54436)
  Move setting fHasVirtualStaticMethods out of sanity check section (dotnet#54574)
  [wasm] Compile .bc->.o in parallel, before passing to the linker (dotnet#54053)
  Change PathInternal.IsCaseSensitive to a constant (dotnet#54340)
  Make mono_polling_required a public symbol (dotnet#54592)
  Respect EventSource::IsSupported setting in more codepaths (dotnet#51977)
  Root ComActivator for hosting (dotnet#54524)
  Add ILLink annotations to S.D.Common related to DbConnectionStringBuilder (dotnet#54280)
  Fix finalizer issue with regions (dotnet#54550)
  Add support for multi-arch install locations (dotnet#53763)
  Update library testing docs page to reduce confusion (dotnet#54324)
  [FileStream] handle UNC and device paths (dotnet#54483)
  Update NetAnalyzers version (dotnet#54511)
  Added runtime dependency to fix the intermittent test failures (dotnet#54587)
  Disable failing System.Reflection.Tests.ModuleTests.GetMethods (dotnet#54564)
  [wasm] Move AOT builds from `runtime-staging` to `runtime` (dotnet#54577)
  Keep obj node for ArrayIndex. (dotnet#54584)
  Disable another failing MemoryCache test (dotnet#54578)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Jul 23, 2021
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.

None yet

3 participants