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

Avoid reporting frozen segments in GetGenerationBounds #85727

Merged
merged 10 commits into from
Jul 1, 2023

Conversation

cshung
Copy link
Member

@cshung cshung commented May 3, 2023

Fixes #87834

  • Avoid reporting the frozen segments through GetGenerationBounds, this would be a duplicate with GetNonGCHeapBounds.

  • Reporting a slightly reduced value for reservedLength in GetNonGCHeapBounds to be consistent with GetGenerationBounds

  • Avoid reporting the GCGenerationBounds event for frozen segments.

  • Rename the GCHeapCollect keyword to ManagedHeapCollect.

Would love to merge this PR if there is no issue with the current code - customers are starting to notice the bug - if we found anything additional that will increase the scope of this PR we can address them later before .net 8.

@cshung cshung requested review from EgorBo and Maoni0 May 3, 2023 19:22
@cshung cshung self-assigned this May 3, 2023
@cshung cshung marked this pull request as draft May 3, 2023 19:22
@ghost
Copy link

ghost commented May 3, 2023

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

Issue Details

Avoid reporting the frozen segments through GetGenerationBounds, this would be a duplicate with GetNonGCHeapBounds.

Reporting a slightly reduced value for reservedLength in GetNonGCHeapBounds to be consistent with GetGenerationBounds

Open questions:

  • Do we want to set heap_segment_gen_num of a frozen segment to be INT32_MAX?
  • What do we want to do with the GCGenerationRange event?

TODO:

  • Double check to make sure things are okay for NativeAOT on the change.
Author: cshung
Assignees: cshung
Labels:

area-GC-coreclr

Milestone: -

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

Thanks Andrew!

src/tests/profiler/native/nongcheap/nongcheap.cpp Outdated Show resolved Hide resolved
src/tests/profiler/native/nongcheap/nongcheap.cpp Outdated Show resolved Hide resolved
src/tests/profiler/native/nongcheap/nongcheap.cpp Outdated Show resolved Hide resolved
src/tests/profiler/native/nongcheap/nongcheap.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/proftoeeinterfaceimpl.cpp Show resolved Hide resolved
src/coreclr/gc/gcee.cpp Outdated Show resolved Hide resolved
src/coreclr/gc/gcee.cpp Outdated Show resolved Hide resolved
@noahfalk
Copy link
Member

noahfalk commented May 5, 2023

Do we want to set heap_segment_gen_num of a frozen segment to be INT32_MAX?

Thats fine by me.

What do we want to do with the GCGenerationRange event?

We should not emit non-gc segments from a GCGenerationRange event, it doesn't match the abstraction we are trying to present to users.

@ghost
Copy link

ghost commented Jun 8, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ww898
Copy link
Contributor

ww898 commented Jun 28, 2023

Hi @cshung and @EgorBo,

My question is about the following statement:

Avoid reporting the frozen segments through GetGenerationBounds, this would be a duplicate with GetNonGCHeapBounds

I may wrong, but I see the issue with old profilers which does not known about ICorProfilerInfo14. The statement above breaks following rule: all objects reported by ICorProfilerCallback::ObjectReferences should be inside GC regoins reported by ICorProfilerInfo2::GetGenerationBounds. Is it correct that objects from any heap (SOH, LOH, POH) can reference objects from FOH? Then ICorProfilerCallback::ObjectReferences will report about FOH objects, but they will be absent in ICorProfilerInfo2::GetGenerationBounds output. It is the issue I would point to.

@cshung
Copy link
Member Author

cshung commented Jun 29, 2023

Is it correct that objects from any heap (SOH, LOH, POH) can reference objects from FOH?

Yes.

Then ICorProfilerCallback::ObjectReferences will report about FOH objects, but they will be absent in ICorProfilerInfo2::GetGenerationBounds output

Yes.

The statement above breaks the following rule ...

Yes. And this is part of the intentional breaking change related to NonGC heap diagnostics, see dotnet/diagnostics#4156 for more details.

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

cshung and others added 6 commits July 1, 2023 10:47
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
@cshung cshung force-pushed the public/avoid-frozen-bounds branch from db4755f to 08d1a42 Compare July 1, 2023 17:52
@cshung cshung marked this pull request as ready for review July 1, 2023 20:02
@cshung cshung merged commit d64c06f into dotnet:main Jul 1, 2023
117 checks passed
@cshung cshung deleted the public/avoid-frozen-bounds branch July 1, 2023 20:04
@ghost ghost locked as resolved and limited conversation to collaborators Aug 1, 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.

[ProfilerAPI] Different reserved region sizes reported for frozen segment
5 participants