Report -1 for GC.GetGeneration(nongc_obj) and same for GetObjectGeneration #85017
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9141,6 +9141,15 @@ HRESULT ProfToEEInterfaceImpl::GetObjectGeneration(ObjectID objectId, | |
|
|
||
| IGCHeap *hp = GCHeapUtilities::GetGCHeap(); | ||
|
|
||
| if (hp->IsInFrozenSegment((Object*)objectId)) | ||
| { | ||
| range->generation = (COR_PRF_GC_GENERATION)INT32_MAX; | ||
| range->rangeStart = 0; | ||
|
There was a problem hiding this comment. We can return memory range for the given frozen segments here. There was a problem hiding this comment.
I agree, but I thought that the idea was to not provide support for apis with explicit GC prefix/name/suffix for nongc objects (cc @noahfalk) There was a problem hiding this comment. Right, the plan of record is to avoid treating Non-GC objects as if they are on the GC heap, and that also included not treating the non-GC segments as if they are some type of special GC generation. We could smuggle the range here at the cost of blurring the lines and potentially creating some confusion. If profiler vendors said it would be specifically helpful to them I don't think it bends the rules too badly though. From the limited profiler feedback I've heard so far folks wanted to enumerate the Non-GC region boundaries directly and resolve the pointers themsevles offline. I'm expecting @davmason to broadcast the changes we are doing to profiler vendors and then depending on their feedback we can make adjustments. |
||
| range->rangeLength = 0; | ||
| range->rangeLengthReserved = 0; | ||
| return CORPROF_E_NOT_GC_OBJECT; | ||
|
There was a problem hiding this comment. Returning error here makes this more breaking than necessary. Why can't we just return There was a problem hiding this comment. Returning -1 seemed like it was going to be a breaking change no matter what so I figured we may as well make it more obvious rather than try to minimize it and increase risk that tools break more subtly. However we don't yet have feedback from profiler authors one way or another and this behavior is just a best guess on my part. If you think we'd be better off defaulting to keeping S_OK @jkotas I don't have a strong opinion on it. There was a problem hiding this comment. Traditionally (in textbook sense) GC generations imply that genX can be collected without collecting generations X+.
That seems to imply that frozen objects logically belong to generation > MAX_GEN. There was a problem hiding this comment.
It looks odd to me to return failure error code and still try to return meaningful values. I see that there are other profiler APIs that work like that, so it should be fine to return error code. You can ignore my feedback. There was a problem hiding this comment. I kind of agree with @VSadov that e.g. INT_MAX value would be safer, e.g. I already had to apply a hack in code because GC/VM usages of There was a problem hiding this comment.
I have no particular preference between MAXINT and -1. I don't anticipate its going to make a difference myself but I also have no concern changing it. 3 I would not use - in the right context 3 is used as a legitimate generation number and the goal is that nobody should treat this value as if it is a generation. There was a problem hiding this comment. There are not that many purposes for Assigning immortal objects to gen There was a problem hiding this comment. I tried INT_MAX and there was only one place that I had to change (some NativeAOT test expecting frozen objects to be in Gen2). Also, So any strong opinions for keeping -1 ? There was a problem hiding this comment. I think |
||
| } | ||
|
|
||
| uint8_t* pStart; | ||
| uint8_t* pAllocated; | ||
| uint8_t* pReserved; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Threading; | ||
|
|
||
| namespace Profiler.Tests | ||
| { | ||
| class NonGCHeapTests | ||
| { | ||
| static readonly Guid GcAllocateEventsProfilerGuid = new Guid("EF0D191C-3FC7-4311-88AF-E474CBEB2859"); | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static void AllocateNonGcHeapObjects() | ||
| { | ||
| // When this method is invoked, JIT is expected to trigger allocations for these | ||
|
There was a problem hiding this comment. This might not work for ReadyToRun? There was a problem hiding this comment. Thanks, disabled the test for crossgen There was a problem hiding this comment. I do not think that this is correct. Did you see the test actually failing with crossgen? If I am reading the code correctly, this test should work with crossgen just fine. The string literals referenced by R2R code must be allocated on frozen heap. Otherwise, the tiering from R2R to Tier1 would work poorly - the tiering would be stuck with non-frozen string literals. There was a problem hiding this comment. Thanks, you're absolutely right |
||
| // string literals and they're expected to end up in a nongc segment (also known as frozen) | ||
| Consume("string1"); | ||
| Consume("string2"); | ||
| Consume("string3"); | ||
| Consume("string4"); | ||
| Consume("string5"); | ||
| Consume("string6"); | ||
|
|
||
| int gen = GC.GetGeneration("string7"); | ||
| if (gen != int.MaxValue) | ||
| throw new Exception("object is expected to be in a non-gc heaps"); | ||
|
|
||
| GC.Collect(gen); | ||
|
There was a problem hiding this comment. The current docs for GC.Collect() and GC.MaxGeneration states:
I don't think we need to promise anyone that GC.Collect(int.MaxValue) will work even if it happens to do so in the current implementation. Instead I think we would document something like:
There was a problem hiding this comment. We should only call GC.Collect(Int32.MaxValue) in the test if we consider it a contract and are willing to fix it if it breaks. If we decide to not view that as a contract we shouldn't have it in the test There was a problem hiding this comment.
I do not think the test coverage for it belongs to this test. A better place to add a test coverage for it would be https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/tests/System/GCTests.cs . There was a problem hiding this comment. Move to GCTests.cs, thanks! There was a problem hiding this comment.
In case there was any confusion I was not proposing we change GC.Collect() behavior. I wanted to avoid declaring in our docs that GC.Collect(int.MaxValue) was supported and well-defined. |
||
| GC.Collect(gen, GCCollectionMode.Optimized, true); | ||
| GC.Collect(gen, GCCollectionMode.Forced, true, true); | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static void Consume(object o) {} | ||
|
|
||
| public static int RunTest(String[] args) | ||
| { | ||
| AllocateNonGcHeapObjects(); | ||
| Console.WriteLine("Test Passed"); | ||
| return 100; | ||
| } | ||
|
|
||
| public static int Main(string[] args) | ||
| { | ||
| if (args.Length > 0 && args[0].Equals("RunTest", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return RunTest(args); | ||
| } | ||
|
|
||
| return ProfilerTestRunner.Run(profileePath: System.Reflection.Assembly.GetExecutingAssembly().Location, | ||
| testName: "NonGCHeapAllocate", | ||
| profilerClsid: GcAllocateEventsProfilerGuid); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <TargetFrameworkIdentifier>.NETCoreApp</TargetFrameworkIdentifier> | ||
| <OutputType>exe</OutputType> | ||
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
| <Optimize>true</Optimize> | ||
| <!-- This test provides no interesting scenarios for GCStress --> | ||
| <GCStressIncompatible>true</GCStressIncompatible> | ||
| <!-- The test launches a secondary process and process launch creates | ||
| an infinite event loop in the SocketAsyncEngine on Linux. Since | ||
| runincontext loads even framework assemblies into the unloadable | ||
| context, locals in this loop prevent unloading --> | ||
| <UnloadabilityIncompatible>true</UnloadabilityIncompatible> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Include="$(MSBuildProjectName).cs" /> | ||
| <ProjectReference Include="$(TestSourceDir)Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" /> | ||
| <ProjectReference Include="../common/profiler_common.csproj" /> | ||
| <CMakeProjectReference Include="$(MSBuildThisFileDirectory)/../native/CMakeLists.txt" /> | ||
| </ItemGroup> | ||
| </Project> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| #include "nongcheap.h" | ||
|
|
||
| GUID NonGcHeapProfiler::GetClsid() | ||
| { | ||
| // {EF0D191C-3FC7-4311-88AF-E474CBEB2859} | ||
| GUID clsid = { 0xef0d191c, 0x3fc7, 0x4311, { 0x88, 0xaf, 0xe4, 0x74, 0xcb, 0xeb, 0x28, 0x59 } }; | ||
| return clsid; | ||
| } | ||
|
|
||
| HRESULT NonGcHeapProfiler::Initialize(IUnknown* pICorProfilerInfoUnk) | ||
| { | ||
| Profiler::Initialize(pICorProfilerInfoUnk); | ||
|
|
||
| HRESULT hr = S_OK; | ||
| if (FAILED(hr = pCorProfilerInfo->SetEventMask2( | ||
| COR_PRF_ENABLE_OBJECT_ALLOCATED | COR_PRF_MONITOR_OBJECT_ALLOCATED, | ||
cshung marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| COR_PRF_HIGH_BASIC_GC))) | ||
| { | ||
| printf("FAIL: ICorProfilerInfo::SetEventMask2() failed hr=0x%x", hr); | ||
| return hr; | ||
| } | ||
|
|
||
| return S_OK; | ||
| } | ||
|
|
||
| HRESULT STDMETHODCALLTYPE NonGcHeapProfiler::ObjectAllocated(ObjectID objectId, ClassID classId) | ||
| { | ||
| COR_PRF_GC_GENERATION_RANGE gen; | ||
| HRESULT hr = pCorProfilerInfo->GetObjectGeneration(objectId, &gen); | ||
|
|
||
| // non-GC objects (same for GC.GetGeneration() API) have generation = -1 | ||
| if (gen.generation == (COR_PRF_GC_GENERATION)INT32_MAX) | ||
| { | ||
| if (!FAILED(hr)) | ||
|
There was a problem hiding this comment. How about calling this API to verify that it is a frozen object as well? There was a problem hiding this comment. From my understanding we sort of ignore these old Frozen* profiler APIs as we re-branded Frozen as NonGC. For the same reason we introduce a new API to traverse frozen objects and ignore existing There was a problem hiding this comment. Ignore is probably fine when we were still designing, but it is really lousy from a testing perspective. It should either supposed to work or it doesn't. If it is supposed to work, and we are not testing it, it would be a test hole. If it is not supposed to work, then we should update the code to make it fails. If we wanted to depreciate old APIs in favor of new ones, we should make that clear in the documentation. Either way, something is missing. |
||
| { | ||
| // We expect GetObjectGeneration to return an error (CORPROF_E_NOT_GC_OBJECT) | ||
| // for non-GC objects. | ||
| _failures++; | ||
| } | ||
| _nonGcHeapObjects++; | ||
| if (gen.rangeLength != 0 || gen.rangeLengthReserved != 0 || gen.rangeStart != 0) | ||
| { | ||
| _failures++; | ||
| } | ||
| } | ||
| else if (FAILED(hr)) | ||
| { | ||
| _failures++; | ||
| } | ||
| return S_OK; | ||
| } | ||
|
|
||
| HRESULT NonGcHeapProfiler::Shutdown() | ||
| { | ||
| if (_failures > 0) | ||
| { | ||
| printf("PROFILER TEST FAILS\n"); | ||
| } | ||
| else if (_nonGcHeapObjects == 0) | ||
| { | ||
| printf("PROFILER TEST FAILS: non-GC heap objects were not allocated\n"); | ||
| } | ||
| else | ||
| { | ||
| printf("PROFILER TEST PASSES\n"); | ||
| } | ||
| printf("Non-GC objects allocated: %d\n", (int)_nonGcHeapObjects); | ||
| printf("PROFILER TEST PASSES\n"); | ||
| fflush(stdout); | ||
| return S_OK; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| #pragma once | ||
|
|
||
| #include "../profiler.h" | ||
|
|
||
| class NonGcHeapProfiler : public Profiler | ||
| { | ||
| public: | ||
| NonGcHeapProfiler() : Profiler(), | ||
| _nonGcHeapObjects(0), | ||
| _failures(0) | ||
| {} | ||
|
|
||
| static GUID GetClsid(); | ||
| virtual HRESULT STDMETHODCALLTYPE Initialize(IUnknown* pICorProfilerInfoUnk); | ||
| virtual HRESULT STDMETHODCALLTYPE ObjectAllocated(ObjectID objectId, ClassID classId); | ||
| virtual HRESULT STDMETHODCALLTYPE Shutdown(); | ||
|
|
||
| private: | ||
| std::atomic<int> _nonGcHeapObjects; | ||
| std::atomic<int> _failures; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Maoni0 - heads up, you probably want to look at any changes that touch the GC.
I'm fine with INT32_MAX as the value that the profiling API returns back to 3rd party code in GetGenerationBounds but I leave it to you to decide if that is also an acceptable behavior for this GC code to have.