Reflection performance tweaks & cleanup #21737

jkotas · 2019-01-01T18:40:19Z

Delete internal "Unsafe" field getters/setters. They performance was very close (within 10%) of the regular getters/setters.
Made a few performance tweaks in reflection to make reflection faster overall. The regular field getters/setters are faster than what the unsafe ones used to be with this change.

jkotas · 2019-01-01T18:42:44Z

Related to discussion in dotnet/corert#6747 (comment)

jkotas · 2019-01-01T22:19:43Z

@dotnet-bot test Ubuntu x64 Checked CoreFX Tests please
@dotnet-bot test Windows_NT x64 Checked CoreFX Tests please

stephentoub · 2019-01-02T01:32:13Z

src/System.Private.CoreLib/src/System/Reflection/RtFieldInfo.cs


-                return m_fieldType;
+                Type Initialize() => m_fieldType = new Signature(this, m_declaringType).FieldType;


This doesn't need to be NoInlining (in which case it also currently can't be local)?

stephentoub · 2019-01-02T01:34:36Z

src/System.Private.CoreLib/src/System/RtType.cs


-                Debug.Assert(cache != null);
-                return cache;
+                    RuntimeTypeCache cache = Unsafe.As<RuntimeTypeCache>(GCHandle.InternalGet(m_cache));


Is the unsafe casting important for perf in this Initialize method?

- Delete internal "Unsafe" field getters/setters. They performance was very close (within 10%) of the regular getters/setters. - Made a few performance tweaks in reflection to make reflection faster overall. The regular field getters/setters are faster than what the unsafe ones used to be with this change.

When there is a race calling RuntimeType.InitializeCache, each of the racing threads creates a new GC handle using new RuntimeTypeHandle(this).GetGCHandle(GCHandleType.WeakTrackResurrection); This ends up calling RuntimeTypeHandle::GetGCHandle native method that adds the allocated handle into the handle cleanup list of the AssemblyLoaderAllocator specific for the runtime type. All but the winning thread then call GCHandle.InternalFree on the just allocated handle. That frees the handle, but leaves it on the cleanup list of the loader allocator. The same handle can be later allocated for some other purpose. When the AssemblyLoaderAllocator is being destroyed, all the handles on the cleanup list are destroyed too. So it destroys also the handle that was left on the cleanup list incorrectly. That can cause all kinds of hard to diagnose issues, like the dotnet/runtime#32171. This change fixes it by adding a FreeGCHandle method on the RuntimeTypeHandle that besides freeing the handle also removes it from the cleanup list of the related AssemblyLoadContext. ## Customer impact Hard to diagnose crashes in the runtime caused by closing of random GC handles. The customer that has reported this issue was using collectible assemblies and it was resulting in collecting LoaderAllocator that was still in use and it lead to crashes at various places. ## Regression? Yes, it was introduced in 3.0. In 2.1 and 2.2, the thread that loses the race destroys the handle only if the type was not in a collectible assembly. Since the non-collectible assemblies LoaderAllocator is never destroyed, the handles were never cleaned up and so no problem could occur. It was introduced in dotnet#21737 ##Testing Customer affected by the issue heavily has tested a fixed version and reported the issue doesn't occur anymore. ## Risk Low, the new code is executed at single place once per process runtine only when a thread races for allocating the GC handle with another one and loses the race.

* Port to 3.1: Fix allocation of RuntimeTypeCache GC handle When there is a race calling RuntimeType.InitializeCache, each of the racing threads creates a new GC handle using new RuntimeTypeHandle(this).GetGCHandle(GCHandleType.WeakTrackResurrection); This ends up calling RuntimeTypeHandle::GetGCHandle native method that adds the allocated handle into the handle cleanup list of the AssemblyLoaderAllocator specific for the runtime type. All but the winning thread then call GCHandle.InternalFree on the just allocated handle. That frees the handle, but leaves it on the cleanup list of the loader allocator. The same handle can be later allocated for some other purpose. When the AssemblyLoaderAllocator is being destroyed, all the handles on the cleanup list are destroyed too. So it destroys also the handle that was left on the cleanup list incorrectly. That can cause all kinds of hard to diagnose issues, like the dotnet/runtime#32171. This change fixes it by adding a FreeGCHandle method on the RuntimeTypeHandle that besides freeing the handle also removes it from the cleanup list of the related AssemblyLoadContext. ## Customer impact Hard to diagnose crashes in the runtime caused by closing of random GC handles. The customer that has reported this issue was using collectible assemblies and it was resulting in collecting LoaderAllocator that was still in use and it lead to crashes at various places. ## Regression? Yes, it was introduced in 3.0. In 2.1 and 2.2, the thread that loses the race destroys the handle only if the type was not in a collectible assembly. Since the non-collectible assemblies LoaderAllocator is never destroyed, the handles were never cleaned up and so no problem could occur. It was introduced in #21737 ##Testing Customer affected by the issue heavily has tested a fixed version and reported the issue doesn't occur anymore. ## Risk Low, the new code is executed at single place once per process runtine only when a thread races for allocating the GC handle with another one and loses the race. * Fix build break - subtle differences from runtime master

- Delete internal "Unsafe" field getters/setters. They performance was very close (within 10%) of the regular getters/setters. - Made a few performance tweaks in reflection to make reflection faster overall. The regular field getters/setters are faster than what the unsafe ones used to be with this change. Commit migrated from dotnet/coreclr@a626939

jkotas requested review from stephentoub and MichalStrehovsky January 1, 2019 18:42

stephentoub approved these changes Jan 2, 2019

View reviewed changes

jkotas added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 2, 2019

jkotas force-pushed the reflection-perf branch from 7dab022 to de1c866 Compare January 4, 2019 01:19

jkotas merged commit a626939 into dotnet:master Jan 4, 2019

jkotas removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 4, 2019

jkotas deleted the reflection-perf branch January 4, 2019 18:55

janvorli mentioned this pull request Mar 9, 2020

Port to 3.1: Fix allocation of RuntimeTypeCache GC handle #28025

Merged

jkotas mentioned this pull request Jul 29, 2020

Access violations around unmanaged LoaderAllocator when using dynamic (emitted with AssemblyBuilder) collectible assemblies dotnet/runtime#32171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reflection performance tweaks & cleanup #21737

Reflection performance tweaks & cleanup #21737

jkotas commented Jan 1, 2019

jkotas commented Jan 1, 2019

jkotas commented Jan 1, 2019

stephentoub Jan 2, 2019

stephentoub Jan 2, 2019


		return m_fieldType;
		Type Initialize() => m_fieldType = new Signature(this, m_declaringType).FieldType;

Reflection performance tweaks & cleanup #21737

Reflection performance tweaks & cleanup #21737

Conversation

jkotas commented Jan 1, 2019

jkotas commented Jan 1, 2019

jkotas commented Jan 1, 2019

stephentoub Jan 2, 2019

Choose a reason for hiding this comment

stephentoub Jan 2, 2019

Choose a reason for hiding this comment