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 ICorProfilerInfo13 to support object handle creation/destruction/access #71257

Merged
merged 15 commits into from
Jul 16, 2022

Conversation

chrisnas
Copy link
Contributor

@chrisnas chrisnas commented Jun 24, 2022

In the context of #49424, 3 methods are added to ICorProfilerInfo to manage strong, weak and pinned handles.

The first usage will be to wrap the reference provided by AllocationTick_V4 by a weak reference handle and use them to figure out if the object is still alive after a certain amount of time. This should help building automatic memory leak detection system.

Note: the corresponding documentation is made available via dotnet/docs#30131

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 24, 2022
@teo-tsirpanis
Copy link
Contributor

@davmason

@ghost
Copy link

ghost commented Jun 25, 2022

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

Issue Details

In the context of #55888, 3 methods are added to ICorProfilerInfo to manage strong, weak and pinned handles.

The first usage will be to wrap the reference provided by AllocationTick_V4 by a weak reference handle and use them to figure out if the object is still alive after a certain amount of time. This should help building automatic memory leak detection system.

Author: chrisnas
Assignees: -
Labels:

area-Diagnostics-coreclr, community-contribution

Milestone: -

@chrisnas chrisnas closed this Jun 29, 2022
@chrisnas chrisnas force-pushed the profiler-can-create-handles branch from 75571c0 to 2a38e20 Compare June 29, 2022 16:12
@davmason
Copy link
Member

@chrisnas if closing it was by accident, you should be able to reopen it. I can't for some reason

@chrisnas
Copy link
Contributor Author

git error...

@chrisnas chrisnas reopened this Jun 30, 2022
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.

Just a few small changes, otherwise looks good to me!

src/coreclr/vm/proftoeeinterfaceimpl.cpp Show resolved Hide resolved
break;

default:
handle = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

We should return E_INVALIDARG in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ Good point. I'm also setting *pHandle to NULL

src/coreclr/vm/proftoeeinterfaceimpl.cpp Show resolved Hide resolved
src/tests/profiler/native/README.md Show resolved Hide resolved
src/tests/profiler/handles/handles.cs Show resolved Hide resolved
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jul 6, 2022
@davmason
Copy link
Member

@chrisnas I think this is ready to merge, when are you planning on doing the docs on docs.microsoft.com? I would really prefer to wait for a docs PR before merging, otherwise docs tend to never get done.

@chrisnas
Copy link
Contributor Author

@chrisnas I think this is ready to merge, when are you planning on doing the docs on docs.microsoft.com? I would really prefer to wait for a docs PR before merging, otherwise docs tend to never get done.

@davmason : is there anything else to do beyond dotnet/docs#30131?

@davmason
Copy link
Member

is there anything else to do beyond dotnet/docs#30131?

Nope, just missed that PR. Thanks, I'll merge this now

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

@davmason davmason merged commit 4d4ddf9 into dotnet:main Jul 16, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Diagnostics-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants