[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks #14982

swgillespie · 2017-11-10T22:58:43Z

For better or for worse, both the runtime and the GC have deep knowledge about the runtime data structures behind overlapped I/O. This knowledge is centered around the existence and layout of the System.Threading.OverlappedData class. The handle table is aware of this class because the runtime creates "async pinned handles" that refer to them and they are treated specially when marking through the handle table.

OverlappedData objects are special in that the objects that they refer to are logically pinned, even though there are no pinning handles in the handle table. This is achieved by treating the m_userObject field specially when marking through the heap:

If m_userObject is an ordinary object, it is reported to the GC as pinned
If m_userObject is an array, all pointers within the array are reported to the GC as pinned, but not the array itself.

OverlappedData objects are special also in that their lifetime is based on an overlapped I/O request and as such the kernel may reappear at any time to signal the completion of the request. If an app domain is unloaded while such an I/O request is still in flight, the handle table must relocate these handles to a new app domain to avoid crashing when the I/O request does return. (Does not apply to CoreCLR, which only has one app domain).

This PR attempts to abstract these details behind four new additions to IGCToCLR:

WalkOverlappedObject - Given an object, if that object is System.Threading.OverlappedData, invoke a callback on all objects that would be considered pinned by this object.
WalkOverlappedObjectForPromotion - Similar to WalkOverlappedObject. Given an object, if that object is System.Threading.OverlappedData, invoke the promotion callback on all objects that would be considered pinned. This is different than the above callback because OverlappedData objects are extremely special and it is not correct to update the m_userObject field when doing a GC; this is taken care of after a GC is completed. The runtime accomplishes this with a second field m_userObjectInternal that records any relocations that may occur on the object being reported. (This can happen because, if m_userObject is an array, it is not pinned.)
OverlappedClearIfComplete - Once an Overlapped I/O request completes, the objects to which the OverlappedData object refer to no longer need to be pinned. This callback gives the EE a chance to unpin the data associated with an OverlappedData object if the I/O request has completed.
OverlappedSetPinnedHandle - When relocating an OverlappedData handle from a condemned app domain, the handle table allocates a new async pinned handle belonging to another app domain. This callback informs the EE of the existence of this new handle.

This set of callbacks is not pretty but it does cover all of the uses of OverlappedData within the GC that I could find. Not all of this code can be fully tested (e.g. app domain loading in CoreCLR) but I am in the process of validating the aspects of this change that I can test. A nice side-effect of this change is that it eliminates a number of places where overlapped data object traversal code was copy-pasted between different areas of the handle table.

This PR addresses the main blocker for https://github.com/dotnet/coreclr/issues/14701.

cc @jkotas @sergiy-k @Maoni0 PTAL?

four callbacks

jkotas · 2017-11-10T23:05:47Z

src/gc/gcinterface.ee.h

+    //
+    // This function is a no-op if "object" is not an OverlappedData object.
+    virtual
+    void WalkOverlappedObjectForPromotion(Object* object, ScanContext* sc, promote_func* callback) = 0;


I would call it AsyncPinned. AsyncPinned better describes the primitive, and it is what we call it on other places in the gcee interface (e.g. there is RelocateAsyncPinnedHandles method).

jkotas · 2017-11-10T23:12:07Z

src/gc/objecthandle.cpp

-        {
-            OverlappedDataObject::MarkCleanupNeededFromGC();
-        }
+        GCToEEInterface::WalkOverlappedObjectForPromotion(pPinnedObj, (ScanContext *)lp1, callback);


Is this going to be called even for regular pinned handles? It would be pretty unfortunate to call this for regular pinned handles. We should split this logic so that it is only called for the async pinned handles.

looks like it will. will do.

This looks potentially difficult. In this point of the code we are looking at an object reference only and have no idea what kind of handle it came from, so we don't know if it's an async pin handle. It seems the only surefire way to know if we are going to need to do additional work on a given object is to inspect its method table.

jkotas · 2017-11-10T23:16:18Z

src/gc/handletable.cpp

@@ -626,34 +622,31 @@ void HndLogSetEvent(OBJECTHANDLE handle, _UNCHECKED_OBJECTREF value)
        FireEtwSetGCHandle((void*) handle, value, hndType, generation, (int64_t) pAppDomain, GetClrInstanceId());
        FireEtwPrvSetGCHandle((void*) handle, value, hndType, generation, (int64_t) pAppDomain, GetClrInstanceId());

-#ifndef FEATURE_REDHAWK


You can delete other FEATURE_REDHAWK ifdefs around the overlapped stuff.

jkotas · 2017-11-10T23:22:01Z

src/gc/handletablecore.cpp

-
-                overlapped->m_pinSelf = HndCreateHandle((HHANDLETABLE)pTargetTable, HNDTYPE_ASYNCPINNED, ObjectToOBJECTREF(value));
-                if (!overlapped->m_pinSelf)
+                GCToEEInterface::OverlappedClearIfComplete((Object*)value);


Would it make more sense to have a callback passed into GCHandleStore::RelocateAsyncPinnedHandles instead of the OverlappedClearIfComplete + OverlappedSetPinnedHandle methods?

That would certainly be more elegant - I'll look into that.

swgillespie · 2017-11-11T01:19:57Z

Marking as no-merge for now since the CI is busted.

Maoni0 · 2017-11-13T08:32:45Z

I agree with Jan's comments.

1. Rename OverlappedData->AsyncPinned in interface methods 2. Remove additional FEATURE_REDHAWK defines around async pin relocation code

jkotas · 2017-11-13T22:24:38Z

src/gc/env/gcenv.base.h

@@ -320,6 +320,11 @@ inline void* ALIGN_DOWN(void* ptr, size_t alignment)
    return reinterpret_cast<void*>(ALIGN_DOWN(as_size_t, alignment));
 }

+inline int GetRandomInt(int max)


Why not call rand() directly? This is used just in one place, in debug-only code.

I heeded this warning that I got when I tried to do that:

e:\github\coreclr\src\gc\handletablecore.cpp(1054): error C3861: 'Do_not_use_rand': identifier not found [E:\GitHub\co reclr\bin\obj\Windows_NT.x64.Debug\src\vm\wks\cee_wks.vcxproj]

I can always undef rand but it seemed to go against the spirit of the warning 😄 .

…ctly as arguments to a method on IGCHandleStore

swgillespie · 2017-11-18T00:44:43Z

The most recent commit eliminates OverlappedClearIfComplete and OverlappedSetPinnedHandle by passing callbacks directly to IGCHandleStore::RelocateAsyncPinnedHandles, which ultimately will invoke the callbacks passed to it when relocating handles in the handle table. This brings us down to the two interface methods that "walk" an async pinned handle with a callback.

I am still unsure how to avoid the indirection in PinHandle - we only need to take the indirection when we know that we are looking at an async pin, but at that point in the code we have no idea what kind of code we are looking at.

jkotas · 2017-11-18T06:58:17Z

I am still unsure how to avoid the indirection in PinHandle

It would need to handled in Ref_TracePinningRoots: We would need to have two scans: one over HNDTYPE_PINNED handles that use a pure pinning callback (and also potentially used more efficient iteration function that does not deal with the extra data), and second over HNDTYPE_ASYNCPINNED handles that calls more expensive overlapped callback in the VM. It would be useful to measure this: Whether it is better to have two specialized scans, or one scan with less efficient callback.

swgillespie · 2017-11-20T20:11:08Z

@jkotas Done in the latest commit - thanks! It doesn't look like the coreclr test suite tests this area very much so I'm doing some additional manual testing now on the parts that I can test.

jkotas · 2017-11-20T23:36:15Z

src/.nuget/init/init.csproj.nuget.cache

@@ -0,0 +1,5 @@
+{
+  "version": 1,


I do not think you meant to add all these files.

oh goodness... i certainly did not. I'll fix up the last commit.

swgillespie · 2017-11-21T20:50:47Z

@dotnet-bot test this please

swgillespie · 2017-11-27T20:21:54Z

I think I've addressed all feedback in this PR and this code has passed all functional testing I've done locally. I think it's ready to merge unless there are any objections.

CentOS failure looks unrelated:

13:33:16 FAILED   - JIT/Performance/CodeQuality/Span/SpanBench/SpanBench.sh
13:33:16                BEGIN EXECUTION
13:33:16                /mnt/resource/j/workspace/dotnet_coreclr/master/checked_centos7.1_tst_prtest/bin/tests/Windows_NT.x64.Checked/Tests/coreoverlay/corerun SpanBench.exe
13:33:16                TestSpanClear<Byte>(100): 0.5357ms
13:33:16                TestSpanClear<String>(100): 0.0069ms
13:33:16                TestArrayClear<Byte>(100): 0.0079ms
13:33:16                TestArrayClear<String>(100): 0.0084ms
13:33:16                
13:33:16                Unhandled Exception: System.TypeLoadException: Could not load type 'System.MemoryExtensions' from assembly 'System.Memory, Version=4.0.1.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'.
13:33:16                   at Span.SpanBench.TestSpanAsBytes[T](T[] array, Int32 iterationCount, Boolean untrue)
13:33:16                   at Span.SpanBench.Invoke(Action`1 innerLoop, String nameFormat, Object[] nameArgs)
13:33:16                   at Span.SpanBench.Main(String[] args)
13:33:16                ./SpanBench.sh: line 243: 75810 Aborted                 (core dumped) $_DebuggerFullPath "$CORE_ROOT/corerun" $ExePath $CLRTestExecutionArguments
13:33:16                Expected: 100
13:33:16                Actual: 134
13:33:16                END EXECUTION - FAILED

swgillespie · 2017-11-27T23:41:13Z

thanks!

… through four callbacks (dotnet#14982)" This reverts commit c755e3b.

Revert "[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks (#14982)"

…to the EE through four callbacks (#14982)""

Revert " Revert "[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks (#14982)""

Reverting the change to Enumerator Current for now Fix file formatting Enable Alpine CI (dotnet#15502) * Enable Alpine CI This enables Alpine CI leg on every PR using the pipelines. compare type size instead of var_types get rid of TYP_CHAR Adding support for Acosh, Asinh, Atanh, and Cbrt to Math and MathF Updating the PAL layer to support acosh, asinh, atanh, and cbrt Adding some PAL tests for acosh, asinh, atanh, and cbrt Adding valuenum support for acosh, asinh, atanh, and cbrt Lsra Documentation Update LinearScan section of ryujit-overview.md, and add lsra-detail.md Refactor Unsafe.cs to get it more in sync with CoreRT. (dotnet#15510) * Refactor Unsafe.cs to get it more in sync with CoreRT. * Format the document. * Unifying the copies of Unsafe using ifdefs * Change exception thrown to PlatformNotSupportedException * Addressing PR feedback and moving Unsafe to shared. * Addressing PR feedback * Addressing PR review - adding intrinsic attribute Update CoreClr, CoreFx to preview1-26014-01, preview1-26013-12, respectively (dotnet#15513) Revert "Add optional integer offset to OwnedMemory Pin (dotnet#15410)" This reverts commit 8931cfa. Get rid of old -altjitcrossgen argument now that CI has been updated Merge pull request dotnet/corert#5109 from dotnet/nmirror (dotnet#15518) Merge nmirror to master Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> Revert " Revert "[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks (dotnet#14982)"" Fix typo `_TARGET_ARM` to `_TARGET_ARM_`. This happens mostly in comments except lsra.cpp. Update CoreClr, CoreFx, PgoData to preview1-26014-04, preview1-26014-03, master-20171214-0043, respectively (dotnet#15520)

* Change ReadOnlySpan indexer to return ref readonly. Update JIT to handle changes to ReadOnlySpan indexer Resolving merge conflict and fixing jit importer Update ReadOnlySpan Enumerator Current to use indexer. Removing readonly keyword. * Temporarily disabling Span perf and other tests that use ReadOnlySpan * Isolating the ref readonly indexer change only to CoreCLR for now. Reverting the change to Enumerator Current for now Fix file formatting Enable Alpine CI (#15502) * Enable Alpine CI This enables Alpine CI leg on every PR using the pipelines. compare type size instead of var_types get rid of TYP_CHAR Adding support for Acosh, Asinh, Atanh, and Cbrt to Math and MathF Updating the PAL layer to support acosh, asinh, atanh, and cbrt Adding some PAL tests for acosh, asinh, atanh, and cbrt Adding valuenum support for acosh, asinh, atanh, and cbrt Lsra Documentation Update LinearScan section of ryujit-overview.md, and add lsra-detail.md Refactor Unsafe.cs to get it more in sync with CoreRT. (#15510) * Refactor Unsafe.cs to get it more in sync with CoreRT. * Format the document. * Unifying the copies of Unsafe using ifdefs * Change exception thrown to PlatformNotSupportedException * Addressing PR feedback and moving Unsafe to shared. * Addressing PR feedback * Addressing PR review - adding intrinsic attribute Update CoreClr, CoreFx to preview1-26014-01, preview1-26013-12, respectively (#15513) Revert "Add optional integer offset to OwnedMemory Pin (#15410)" This reverts commit 8931cfa. Get rid of old -altjitcrossgen argument now that CI has been updated Merge pull request dotnet/corert#5109 from dotnet/nmirror (#15518) Merge nmirror to master Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> Revert " Revert "[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks (#14982)"" Fix typo `_TARGET_ARM` to `_TARGET_ARM_`. This happens mostly in comments except lsra.cpp. Update CoreClr, CoreFx, PgoData to preview1-26014-04, preview1-26014-03, master-20171214-0043, respectively (#15520) * Disabling a test that uses ReadOnlySpan indexer * Temporarily disabling the superpmi test and fixing nit * Remove debug statements.

[Local GC] Move knowledge of overlapped I/O objects to the EE through

7bcdb00

four callbacks

swgillespie added the area-GC label Nov 10, 2017

swgillespie added this to the 2.1.0 milestone Nov 10, 2017

jkotas reviewed Nov 10, 2017

View reviewed changes

swgillespie added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 11, 2017

Code review feedback:

03843f9

1. Rename OverlappedData->AsyncPinned in interface methods 2. Remove additional FEATURE_REDHAWK defines around async pin relocation code

jkotas reviewed Nov 13, 2017

View reviewed changes

Eliminate two GCToEEInterface callbacks by passing the callbacks dire…

6b8b010

…ctly as arguments to a method on IGCHandleStore

swgillespie removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 18, 2017

swgillespie added 2 commits November 20, 2017 10:36

Repair clang build

fb0a76c

Split pin and async pin handle scans into two separate callbacks

92e225f

jkotas reviewed Nov 20, 2017

View reviewed changes

Fix the clang and non-Windows builds

af847d2

swgillespie force-pushed the nativeoverlapped-handle-table branch from 5adfff6 to af847d2 Compare November 20, 2017 23:41

jkotas approved these changes Nov 27, 2017

View reviewed changes

swgillespie merged commit c755e3b into dotnet:master Nov 27, 2017

swgillespie added a commit to swgillespie/coreclr that referenced this pull request Dec 13, 2017

Revert "[Local GC] Move knowledge of overlapped I/O objects to the EE…

8df01f5

… through four callbacks (dotnet#14982)" This reverts commit c755e3b.

jkotas added a commit that referenced this pull request Dec 13, 2017

Merge pull request #15498 from swgillespie/asyncpin-revert

439ec81

Revert "[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks (#14982)"

jkotas added a commit that referenced this pull request Dec 14, 2017

Revert " Revert "[Local GC] Move knowledge of overlapped I/O objects …

bc5a76d

…to the EE through four callbacks (#14982)""

jkotas added a commit that referenced this pull request Dec 14, 2017

Merge pull request #15517 from dotnet/revert-15498-asyncpin-revert

6755fcc

Revert " Revert "[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks (#14982)""

swgillespie mentioned this pull request Jan 31, 2020

[Local GC] AV in handle table when promoting overlapped data object dotnet/runtime#9410

Closed

crui3er mentioned this pull request Dec 25, 2020

ClrMD 2.0 reports less number of pinned handles than ClrMD 1.1 microsoft/clrmd#881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks #14982

[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks #14982

swgillespie commented Nov 10, 2017

jkotas Nov 10, 2017

swgillespie Nov 10, 2017

jkotas Nov 10, 2017

swgillespie Nov 10, 2017

swgillespie Nov 13, 2017

jkotas Nov 10, 2017

swgillespie Nov 10, 2017

jkotas Nov 10, 2017

swgillespie Nov 10, 2017

swgillespie commented Nov 11, 2017

Maoni0 commented Nov 13, 2017

jkotas Nov 13, 2017

swgillespie Nov 13, 2017 •

edited

Loading

swgillespie commented Nov 18, 2017

jkotas commented Nov 18, 2017 •

edited

Loading

swgillespie commented Nov 20, 2017

jkotas Nov 20, 2017

swgillespie Nov 20, 2017

swgillespie commented Nov 21, 2017

swgillespie commented Nov 27, 2017

swgillespie commented Nov 27, 2017

[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks #14982

[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks #14982

Conversation

swgillespie commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie commented Nov 11, 2017

Maoni0 commented Nov 13, 2017

Choose a reason for hiding this comment

swgillespie Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

swgillespie commented Nov 18, 2017

jkotas commented Nov 18, 2017 • edited Loading

swgillespie commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie commented Nov 21, 2017

swgillespie commented Nov 27, 2017

swgillespie commented Nov 27, 2017

swgillespie Nov 13, 2017 •

edited

Loading

jkotas commented Nov 18, 2017 •

edited

Loading