Avoid NativeOverlapped pinning by allocating unmanaged memory for it #18360

jkotas · 2018-06-07T22:44:24Z

Allocate NativeOverlapped on unmanaged heap
It made PinnableBufferCache unnecessary and allowed me to delete it
I have also deleted overlapped handle bashing that allowed pending overlapped operations to not block appdomain unloading. It is not used today and unlikely to be ever needed again.

Fixes #17909

jkotas · 2018-06-07T22:47:42Z

src/System.Private.CoreLib/src/System/Threading/Overlapped.cs

    {
-        private OverlappedData m_overlappedData;
-        private static PinnableBufferCache s_overlappedDataCache = new PinnableBufferCache("System.Threading.OverlappedData", () => new OverlappedData());
+        private OverlappedData _overlappedData;


The split between Overlapped and OvelappedData is not necessary either anymore. I may be able to eliminate the split as well.

jkotas · 2018-06-07T22:52:06Z

Perf results:

Synchronous alloc/free is 1.5x faster:

    for (int i = 0; i < 10000000; i++)
    {
        var o = new Overlapped();
        var p = o.UnsafePack(callback, data);
        Overlapped.Free(p);
    }

Asynchronous alloc/free is 1.1x faster:

    for (int i = 0; i < 10000000; i++)
    {
        var o = new Overlapped();
        var p = o.UnsafePack(callback, data);
        Task.Run(() => { Overlapped.Free(p); });
    }

jkotas · 2018-06-07T23:35:17Z

src/gc/objecthandle.cpp

    Object **pRef = (Object **)pObjRef;
    _ASSERTE(lp2);
    promote_func* callback = (promote_func*)lp2;
-    callback(pRef, (ScanContext *)lp1, GC_CALL_PINNED);


@Maoni0 @davmason This is changing contract of the local GC. Do I need to worry about it?

I don't see how this affects LocalGC... it's just making a callback that's implemented internally by the GC.

sorry I haven't had time to look at the rest of the changes - do you mind explaining why you are not passing the GC_CALL_PINNED flag anymore? if the object is now a native obj, it wouldn't need to call the callback at all - GC is not going to do anything with the object. do we still have Overlapped managed objects at all anymore (or did you split up the native part so the managed part doesn't need to be pinned)? if not then do we still need this async pinned handle type?

We still have Overlapped managed objects with this change, but the ASYNC handles do not pin them anymore - only the data arrays that they are pointing to are pinned by the ASYNC handles with this change.

It is the change in the GC contract: the ASYNC handles were pinning the overlapped objects before this change and they are not pinning the overlapped objects after this change.

ok then if you run with this version of clrgc.dll with an older version of clr.dll it wouldn't work. but this doesn't have to be a breaking change - we could communicate the version of the clr.dll to clrgc.dll and have async pinned handles do different things based on the minor version. but of course this requires some changes in LocalGC. what do you think, @davmason? otherwise you'd need to change the major version (GC_INTERFACE_MAJOR_VERSION) which is probably ok at this point if we think not many people will be using the 2.1 LocalGC anyway.

I know you see a big improvement in a microbenchmark - in real world scenarios the perf problem that these overlapped structures introduce is always about fragmentation and a lot of the fragmentation comes from the data that gets pinned so I am not sure how much improvement you'd see there.

We probably need to rev the major version for 2.2 anyways, so I think we can just accept that and make the 2.2 local gc incompatible with 2.1. There are some other changes too that likely need an api change for 2.2 (like the debugger notifications in #17769). If this were the only issue it might make sense to talk about ways to avoid it, but it seems inevitable at this point.

My primary motivation for this change has been code simplification. I have done the performance measurements just to ensure I am not regressing performance. The improvement was a nice surprise.

I have reved the GC interface version.

jkotas · 2018-06-07T23:36:22Z

src/vm/comthreadpool.cpp

+    lpOverlapped->Internal = 0;

    if (ETW_EVENT_ENABLED(MICROSOFT_WINDOWS_DOTNETRUNTIME_PROVIDER_Context, ThreadPoolIOEnqueue))
        FireEtwThreadPoolIOEnqueue(lpOverlapped, OBJECTREFToObject(overlapped), false, GetClrInstanceId());


@vancem @brianrob Are you aware of any tooling that uses the object in this event and that can get broken by this change? (The object is no longer pinned.)

I am not aware of any tooling that would be affected by moving the overlapped structure from the managed to native heap. Thanks for checking.

I suspect that in addition to improved throughput on alloc/free, this will also have a positive impact on the heap in cases where PinnableBufferCache wasn't able to do as well as we would have liked.

stephentoub · 2018-06-08T14:07:24Z

-2000 lines of code. Perf improvement. Awesome.

jkotas · 2018-06-08T17:06:33Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

stephentoub · 2018-06-09T02:20:00Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please ("java.nio.channels.ClosedChannelException")

jkotas · 2018-06-09T02:38:44Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

It makes PinnableBufferCache unnecessary

jkotas · 2018-06-09T05:36:57Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

Port dotnet/coreclr#18360 to CoreRT

Port #18360 to CoreRT Signed-off-by: dotnet-bot <[email protected]>

Port dotnet/coreclr#18360 to CoreRT Signed-off-by: dotnet-bot <[email protected]> Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>

stephentoub · 2019-04-02T14:27:29Z

src/System.Private.CoreLib/src/System/Threading/Overlapped.cs

+            // The split between Overlapped and OverlappedData should not be needed. It is required by the implementation of 
+            // async GC handles currently. It expects OverlappedData to be a sealed type.
+            _overlappedData = new OverlappedData();
+            _overlappedData._overlapped = this;


@jkotas, I'm annotating this for nullability... any reason I can't make OverlappedData's ctor take the Overlapped as an argument? It would allow for the _overlapped field to then be readonly rather than nullable.

OverlappedData's ctor take the Overlapped as an argument

This should be fine. I do not see any issues with it.

…otnet/coreclr#18360) It makes PinnableBufferCache unnecessary Commit migrated from dotnet/coreclr@911d332

Port dotnet/coreclr#18360 to CoreRT Signed-off-by: dotnet-bot <[email protected]> Commit migrated from dotnet/coreclr@108ead4

jkotas commented Jun 7, 2018

View reviewed changes

jkotas force-pushed the overlapped branch from 8b9d0ab to 69753da Compare June 7, 2018 23:39

jkotas mentioned this pull request Jun 8, 2018

Moved PinnableBufferCache.cs and PinnableBufferCacheEventSource.cs to shared #18293

Closed

jkotas requested a review from Anipik June 8, 2018 00:10

jkotas force-pushed the overlapped branch from 21f2624 to 4c00410 Compare June 8, 2018 00:15

stephentoub approved these changes Jun 8, 2018

View reviewed changes

jkotas force-pushed the overlapped branch from 4e13549 to af97b5d Compare June 9, 2018 00:12

Avoid NativeOverlapped pinning by allocating unmanaged memory for it

a165d60

It makes PinnableBufferCache unnecessary

jkotas force-pushed the overlapped branch from af97b5d to a165d60 Compare June 9, 2018 04:45

jkotas merged commit 911d332 into dotnet:master Jun 9, 2018

jkotas deleted the overlapped branch June 9, 2018 20:44

jkotas mentioned this pull request Jun 16, 2018

Delete PinnableBufferCache dotnet/corert#5950

Merged

jkotas added a commit to dotnet/corert that referenced this pull request Jun 18, 2018

Delete PinnableBufferCache (#5950)

d966f73

Port dotnet/coreclr#18360 to CoreRT

dotnet-bot pushed a commit that referenced this pull request Jun 18, 2018

Delete PinnableBufferCache (dotnet/corert#5950)

d420be1

Port #18360 to CoreRT Signed-off-by: dotnet-bot <[email protected]>

jkotas added a commit that referenced this pull request Jun 18, 2018

Delete PinnableBufferCache (dotnet/corert#5950)

280bdf1

Port #18360 to CoreRT Signed-off-by: dotnet-bot <[email protected]>

jkotas added a commit that referenced this pull request Jun 18, 2018

Delete PinnableBufferCache (dotnet/corert#5950)

108ead4

Port #18360 to CoreRT Signed-off-by: dotnet-bot <[email protected]>

dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Jun 18, 2018

Delete PinnableBufferCache (dotnet/corert#5950)

a73afac

Port dotnet/coreclr#18360 to CoreRT Signed-off-by: dotnet-bot <[email protected]> Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>

jkotas added a commit to dotnet/corefx that referenced this pull request Jun 18, 2018

Delete PinnableBufferCache (dotnet/corert#5950)

3594eb3

Port dotnet/coreclr#18360 to CoreRT Signed-off-by: dotnet-bot <[email protected]> Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>

jkotas mentioned this pull request Nov 11, 2018

[RFC] Reusable ThreadPool workitems #20936

Closed

This was referenced Dec 1, 2018

Only get Thread.CurrentThread once in PerformIOCompletionCallback #21320

Closed

Use clean contexts for IO Thread callbacks #21327

Closed

jkotas mentioned this pull request Mar 10, 2019

Move Overlapped implementation to managed code in CoreLib #23029

Closed

5 tasks

stephentoub reviewed Apr 2, 2019

View reviewed changes

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Avoid NativeOverlapped pinning by allocating unmanaged memory for it (d…

beb2113

…otnet/coreclr#18360) It makes PinnableBufferCache unnecessary Commit migrated from dotnet/coreclr@911d332

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Delete PinnableBufferCache (dotnet/corertdotnet/coreclr#5950)

474d3a7

Port dotnet/coreclr#18360 to CoreRT Signed-off-by: dotnet-bot <[email protected]> Commit migrated from dotnet/coreclr@108ead4

Avoid NativeOverlapped pinning by allocating unmanaged memory for it #18360

Avoid NativeOverlapped pinning by allocating unmanaged memory for it #18360

Uh oh!

Conversation

jkotas commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Jun 8, 2018

Uh oh!

jkotas commented Jun 8, 2018

Uh oh!

stephentoub commented Jun 9, 2018

Uh oh!

jkotas commented Jun 9, 2018

Uh oh!

jkotas commented Jun 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jkotas commented Jun 7, 2018 •

edited

Loading

jkotas Jun 7, 2018 •

edited

Loading