Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement ICorProfilerInfo14::GetNonGCHeapBounds #85434

Merged
merged 6 commits into from
May 2, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Apr 27, 2023

Contributes to dotnet/diagnostics#4156

@ghost
Copy link

ghost commented Apr 27, 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

Author: EgorBo
Assignees: EgorBo
Labels:

area-Diagnostics-coreclr

Milestone: -

Object* result = reinterpret_cast<Object*>(nextObj);
INDEBUG(result->Validate());
return result;
return reinterpret_cast<Object*>(nextObj);
Copy link
Member Author

Choose a reason for hiding this comment

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

we agreed not to use COOP (Validate() needs it)

@EgorBo EgorBo marked this pull request as ready for review April 27, 2023 09:42
// Add test coverage for IsFrozenObject API, currently, it is expected to return true
// for objects from non-GC heap (it might also return true for frozen segments we don't track)
BOOL isFrozen;
hr = pCorProfilerInfo->IsFrozenObject(obj, &isFrozen);
Copy link
Member Author

Choose a reason for hiding this comment

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

@cshung added test coverage for IsFrozenObject on your request

@EgorBo EgorBo requested review from noahfalk and davmason April 27, 2023 11:18
@EgorBo
Copy link
Member Author

EgorBo commented Apr 27, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Apr 27, 2023

Failures are #85081 and ci timeouts

@EgorBo
Copy link
Member Author

EgorBo commented May 1, 2023

@davmason @noahfalk PTAL

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 the one comment, otherwise looks good

@EgorBo
Copy link
Member Author

EgorBo commented May 2, 2023

Failures are known, #85637

@EgorBo EgorBo merged commit 8091325 into dotnet:main May 2, 2023
@EgorBo EgorBo deleted the GetNonGCHeapBounds branch May 2, 2023 18:10

if (pcObjectRanges != nullptr)
{
*pcObjectRanges = segmentsToInspect;
Copy link
Member

Choose a reason for hiding this comment

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

@EgorBo - I think we want *pcObjectRanges = segmentsCount

I'd expect the current behavior will make a test case like this fail:

ULONG count
GetNonGCHeapBounds(0, &count, nullptr);
ranges = new ranges[count];
GetNonGCHeapBounds(count, nullptr, ranges);

Copy link
Member Author

Choose a reason for hiding this comment

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

@EgorBo - I think we want *pcObjectRanges = segmentsCount

I'd expect the current behavior will make a test case like this fail:

ULONG count
GetNonGCHeapBounds(0, &count, nullptr);
ranges = new ranges[count];
GetNonGCHeapBounds(count, nullptr, ranges);

Ah, makes sense, I thought it was "how many entries were written to ranges". Will fix

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I've seen it used that way too in other interfaces, but I checked GetGenerationBounds() for the GC case and it appears to have the segmentsCount semantics there so presumably we want to match the behavior.

@EgorBo EgorBo mentioned this pull request May 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 2, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants