GcHandle Perf Tweaks by benaadams · Pull Request #9473 · dotnet/coreclr

benaadams · 2017-02-09T23:33:11Z

GCHandle is either mostly a thin wrapper around VM functions; or simple bitshifts and casts. However its current implementation has significant overhead as it is called a huge amount and its functions are marked by NGen as permanently uninlinable.

Previously 2,028.497 GCHandle calls for 158,475 libuv write calls (in Marshal)

After 814 GCHandle calls for 2,760,965 libuv write calls (in Marshal)

Mostly they are fairly simple, so for example while GCHandle:SetIsPinned is unprofitable and permanently no inline; is asm ends up extremely simple

Marking GCHandle:SetIsPinned():this as NOINLINE because of unprofitable inline
Successfully inlined IntPtr:op_Explicit(long):long (9 IL bytes) (depth 1) [below ALWAYS_INLINE size]
Successfully inlined IntPtr:.ctor(long):this (9 IL bytes) (depth 1) [below ALWAYS_INLINE size]
**************** Inline Tree
Inlines into 0600369E GCHandle:SetIsPinned():this
  [1 IL=0007 TR=000004 06000C06] [below ALWAYS_INLINE size] IntPtr:op_Explicit(long):long
  [2 IL=0015 TR=000017 06000BF6] [below ALWAYS_INLINE size] IntPtr:.ctor(long):this
Budget: initialTime=138, finalTime=146, initialBudget=1380, currentBudget=1380
Budget: initialSize=724, finalSize=724
; Assembly listing for method GCHandle:SetIsPinned():this
; Emitting BLENDED_CODE for X64 CPU with SSE2
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 this         [V00,T00] (  4,   4  )   byref  ->  rcx         this
;* V01 tmp0         [V01    ] (  0,   0  )    long  ->  zero-ref   
;  V02 tmp1         [V02,T01] (  2,   4  )    long  ->  rax         ld-addr-op
;  V03 tmp2         [V03,T02] (  2,   4  )    long  ->  rax        
;# V04 OutArgs      [V04    ] (  1,   1  )  lclBlk ( 0) [rsp+0x00]  
;
; Lcl frame size = 0

G_M19266_IG01:
       0F1F440000           nop      

G_M19266_IG02:
       488B01               mov      rax, qword ptr [rcx]
       4883C801             or       rax, 1
       488901               mov      qword ptr [rcx], rax

G_M19266_IG03:
       C3                   ret      

; Total bytes of code 16, prolog size 5 for method GCHandle:SetIsPinned():this

benaadams · 2017-02-09T23:44:15Z

            // Free the handle if it hasn't already been freed.
-            if (handle != IntPtr.Zero && Interlocked.CompareExchange(ref m_handle, IntPtr.Zero, handle) == handle)
-            {
+            IntPtr handle = Interlocked.Exchange(ref m_handle, IntPtr.Zero);


Doesn't need CompareExchange as m_handle always ends up as IntPtr.Zero; so can just Exchange it with IntPtr.Zero and the perform the tests on the result

Yes, agree.

jkotas · 2017-02-09T23:56:03Z

+        }

-            return new GCHandle(handle);
+        internal static object GetTargetFromIntPtr(IntPtr value)


Why do we need this?

AssemblyLoadContext does a set of handle double verifies with the target property: GCHandle.FromIntPtr(...).Target as seemed to be a particular area where some of the functions where getting marked as no-inline. So I collapsed the two calls and gave its own internal function.

However, I think the most significant change was m_handle == IntPtr.Zero to m_handle.IsNull() so I could probably revert this and retest?

The AssemblyLoadContext is not performance critical. The double verification is not a problem there.

benaadams · 2017-02-10T00:17:05Z

Updated summary

benaadams · 2017-02-10T00:22:16Z

GCHandle:SetIsPinned is probably a bad example as it still doesn't inline :)

jkotas · 2017-02-10T00:27:18Z

SetIsPinned

This is how you can get fix it:

            IntPtr handle = InternalAlloc(value, type);

            // Record if the handle is pinned.
            if (type == GCHandleType.Pinned)
            {
#if BIT64
                handle = new IntPtr(((long)handle) | 1L);
#else // !BIT64 (32)
                handle = new IntPtr(((int)handle) | 1);
#endif
            }

            m_handle = handle;

jkotas · 2017-02-10T00:30:09Z

Or even better - add this at the top:

#if BIT64
using nint = System.Int64;
#else
using nint = System.Int32;
#endif

And then you can write it as handle = (IntPtr)((nint)handle | 1) without ifdefs.

benaadams · 2017-02-10T01:00:24Z

Yep, that seems to do the trick, also removed GetTargetFromIntPtr

jkotas · 2017-02-10T05:20:14Z

+    using nuint = System.UInt64;
+    using nint = System.Int64;
+#else
+    using nuint = System.UInt32;


Can we use signed nint consistently everywhere - since the handle is signed IntPtr?

jkotas · 2017-02-10T05:21:25Z

-        // IMPORTANT: This must be kept in sync with the GCHandleType enum.
-        private const GCHandleType MaxHandleType = GCHandleType.Pinned;
+#if BIT64
+    private const long NoFlag = ~1L;


Can this be just ~(nint)1 without ifdef ? Also, since it is used in just one place, it may not be worth a constant.

jkotas · 2017-02-10T05:22:22Z

LGTM modulo few nits.

jkotas

Thanks!

* GcHandle Perf Tweaks

* GcHandle Perf Tweaks Commit migrated from dotnet/coreclr@b85d71f

GcHandle Perf Tweaks

77c36ba

dnfclas added the cla-already-signed label Feb 9, 2017

benaadams commented Feb 9, 2017

View reviewed changes

jkotas reviewed Feb 9, 2017

View reviewed changes

Feedback

5e9d3ec

benaadams mentioned this pull request Feb 10, 2017

Reduce Task and async method overheads #9471

Merged

jkotas reviewed Feb 10, 2017

View reviewed changes

Feedback

180f1ba

jkotas approved these changes Feb 10, 2017

View reviewed changes

jkotas merged commit b85d71f into dotnet:master Feb 10, 2017

benaadams deleted the gchandle branch February 10, 2017 13:56

jkotas mentioned this pull request Feb 10, 2017

Port GCHandle perf tweaks from CoreCLR dotnet/corert#2726

Closed

sdmaclea pushed a commit to sdmaclea/coreclr that referenced this pull request Feb 15, 2017

GcHandle Perf Tweaks (dotnet#9473)

539b054

* GcHandle Perf Tweaks

This was referenced Mar 17, 2017

Renaming IndexOfVectorized to IndexOf dotnet/corefxlab#1320

Merged

Use for rather than foreach on List aspnet/KestrelHttpServer#1523

Merged

jorive pushed a commit to guhuro/coreclr that referenced this pull request May 4, 2017

GcHandle Perf Tweaks (dotnet#9473)

2beed9d

* GcHandle Perf Tweaks

karelz modified the milestone: 2.0.0 Aug 28, 2017

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

GcHandle Perf Tweaks (dotnet/coreclr#9473)

4a2c0f2

* GcHandle Perf Tweaks Commit migrated from dotnet/coreclr@b85d71f

Conversation

benaadams commented Feb 9, 2017 • 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!

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!

benaadams commented Feb 10, 2017

Uh oh!

benaadams commented Feb 10, 2017

Uh oh!

jkotas commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Feb 10, 2017

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 commented Feb 10, 2017

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benaadams commented Feb 9, 2017 •

edited

Loading

jkotas commented Feb 10, 2017 •

edited

Loading

jkotas commented Feb 10, 2017 •

edited

Loading