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

Implement ICorProfilerInfo14::EnumerateNonGCObjects #85100

Merged
merged 8 commits into from
Apr 26, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Apr 20, 2023

Contributes to dotnet/diagnostics#4156

@ghost
Copy link

ghost commented Apr 20, 2023

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

Issue Details

Contributes to dotnet/diagnostics#4156

Will add tests once #85017 is merged (to the same project).

Author: EgorBo
Assignees: EgorBo
Labels:

area-Diagnostics-coreclr

Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Apr 23, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo EgorBo marked this pull request as ready for review April 23, 2023 17:05
@EgorBo
Copy link
Member Author

EgorBo commented Apr 24, 2023

@davmason @noahfalk PTAL

cc @jkotas for changes in frozenobjectheap.*

RE: big diff is corprof.h seems to be expected? I copied it from artifacts\obj\coreclr\windows.x64.Checked\ide\inc\idls_out like @davmason suggested and judging by the previous update in #71257 it's normal?

@@ -7586,6 +7586,39 @@ HRESULT ProfToEEInterfaceImpl::GetObjectIDFromHandle(
return S_OK;
}

Copy link
Member

Choose a reason for hiding this comment

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

You'll also need to add ICorProfilerInfo14 to ProfToEEInterfaceImpl::QueryInterface

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 25, 2023
@davmason
Copy link
Member

Besides adding the QI it looks good to me

}
CONTRACTL_END;

GCX_COOP();
Copy link
Member

Choose a reason for hiding this comment

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

What portion of the implementation requires us to switch to coop mode? This might make the API challenging to use without a deadlock. The primary use I am anticipating from profiler vendors is that they would enumerate both the GC and non-GC heaps during a GC. This means they would invoke this API inside one of the callbacks that GC is starting or finishing and at that point the runtime would be suspended. I'm guessing the transition to COOP mode would block until the EE resumes, the EE won't resume until the profiler GC callback returns, and the GC callback won't return until this enumeration completes.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not needed indeed, removed

// Let's make sure we got the same number of objects as we got from the callback
// by testing the EnumerateNonGCObjects API.
ICorProfilerObjectEnum* pEnum = NULL;
HRESULT hr = pCorProfilerInfo->EnumerateNonGCObjects(&pEnum);
Copy link
Member

Choose a reason for hiding this comment

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

Can you modify the test case to enable the profiler COR_PRF_MONITOR_GC events and then try doing this enumeration inside the GarbageCollectionFinished callback? I think that is one of the more likely places a profiler might want to enumerate the non-GC heap. I'm suspicious the current implementation may deadlock but even if all works well it will be good for us to confirm that scenario doesn't regress.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 26, 2023
@EgorBo
Copy link
Member Author

EgorBo commented Apr 26, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

👍

@EgorBo EgorBo merged commit e5798eb into dotnet:main Apr 26, 2023
@EgorBo EgorBo deleted the enum-frozen-objects branch April 26, 2023 11:06
@EgorBo
Copy link
Member Author

EgorBo commented Apr 26, 2023

CI failures are known timeouts (#76454)

@ghost ghost locked as resolved and limited conversation to collaborators May 26, 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.

4 participants