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 a few profiler related issues associated with frozen objects #79563

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

cshung
Copy link
Member

@cshung cshung commented Dec 12, 2022

This change fixes a few profiler-related issues associated with frozen objects.

  1. Reset the pinned queue only once
  2. Publish frozen objects
    3. Report the frozen segments as survived
  3. Set frozen segment gen_num

Reset the pinned queue only once

This was an existing bug that is unrelated to frozen objects (for reason I don't understand it only got exposed recently).

In walk_relocation, we walk the generations (from old to young), the associated bricks (in region list order), and the associated plug trees (using an in-order traversal). During the walk, we dequeue the pinned plugs from the pinned plug queue.

There is only one pinned queue shared across all generations. Therefore we should not reset the pinned queue for every generation.

Publish frozen objects

Some profilers track object instances. If we don't report an object as allocated, they won't be tracking it, and they might be surprised if later on the object is reported as root.

Before the change, frozen objects are not reported as allocated.

After the change, frozen objects allocated through FrozenObjectHeapManager::TryAllocate are now reported to the profiler through PublishObjectAndNotify, and therefore they will be available through ICorProfiler::ObjectAllocated callback.

Report the frozen segments as survived

Some profilers might assume that objects previously allocated not falling in that survived range are being collected.

Before the change, frozen segments are not reported, and therefore the profiler might assume frozen objects are collected, while they are not, and then got surprised that they got reported as root.

After the change, the frozen segments are reported as survived, to do that, the walk_relocation_sip method is renamed to walk_relocation_special_segments to also handle the special case for frozen segments.

Set frozen segment gen_num

As we report the regions to the profiler, we will read the gen_num of the heap segment. The current code didn't set that value, so we might be reading random numbers in the profiler. That's not good. As an implementation detail, we thread the frozen segment into gen2, so we might as well set the gen_num to 2 there.

@dotnet/gc

@ghost
Copy link

ghost commented Dec 12, 2022

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

Issue Details

This change fixes a few profiler-related issues associated with frozen objects.

  1. Reset the pinned queue only once
  2. Publish frozen objects
  3. Report the frozen segments as survived
  4. Set frozen segment gen_num

Reset the pinned queue only once

This was an existing bug that is unrelated to frozen objects (for reason I don't understand it only got exposed recently).

In walk_relocation, we walk the generations (from old to young), the associated bricks (in region list order), and the associated plug trees (using an in-order traversal). During the walk, we dequeue the pinned plugs from the pinned plug queue.

There is only one pinned queue shared across all generations. Therefore we should not reset the pinned queue for every generation.

Publish frozen objects

Some profilers track object instances. If we don't report an object as allocated, they won't be tracking it, and they might be surprised if later on the object is reported as root.

Before the change, frozen objects are not reported as allocated.

After the change, frozen objects allocated through FrozenObjectHeapManager::TryAllocate are now reported to the profiler through PublishObjectAndNotify, and therefore they will be available through ICorProfiler::ObjectAllocated callback.

Report the frozen segments as survived

Some profilers might assume that objects previously allocated not falling in that survived range are being collected.

Before the change, frozen segments are not reported, and therefore the profiler might assume frozen objects are collected, while they are not, and then got surprised that they got reported as root.

After the change, the frozen segments are reported as survived, to do that, the walk_relocation_sip method is renamed to walk_relocation_special_segments to also handle the special case for frozen segments.

Set frozen segment gen_num

As we report the regions to the profiler, we will read the gen_num of the heap segment. The current code didn't set that value, so we might be reading random numbers in the profiler. That's not good. As an implementation detail, we thread the frozen segment into gen2, so we might as well set the gen_num to 2 there.

@dotnet/gc

Author: cshung
Assignees: cshung
Labels:

area-Diagnostics-coreclr

Milestone: -

@cshung
Copy link
Member Author

cshung commented Dec 12, 2022

dotnet/diagnostics#4156

Copy link
Contributor

@PeterSolMS PeterSolMS left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@davmason davmason left a comment

Choose a reason for hiding this comment

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

LGTM from the profiler side, I am not a GC expert though :)

@cshung
Copy link
Member Author

cshung commented Jan 4, 2023

@Maoni0 and I had a discussion on this and we believe reporting the frozen segments as a plug is inappropriate. We need a different fix for that part.

src/coreclr/gc/gcee.cpp Outdated Show resolved Hide resolved
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
@cshung cshung merged commit 6aaaaaa into dotnet:main Jan 11, 2023
@cshung cshung deleted the public/profiler-fixes branch January 21, 2023 07:11
@ghost ghost locked as resolved and limited conversation to collaborators Feb 20, 2023
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.

5 participants