From d51a8d5579f152ba8349c50d4b0310048f8bec7e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?=
 <MichalStrehovsky@users.noreply.github.com>
Date: Fri, 14 Jul 2023 13:33:11 +0900
Subject: [PATCH] Simplify ldftn reverse lookups (#88719)

This used a delegate because in the past we had multiple possible callbacks. Now the delegate callback is unnecessary. Saves 12 kB on Hello World with stack traces disabled (the delegate was the only thing keeping the `GetLdFtnReverseLookups_InvokeMap` method alive).
---
 ...EnvironmentImplementation.MappingTables.cs | 70 +++----------------
 ...System.Private.Reflection.Execution.csproj |  3 +
 2 files changed, 13 insertions(+), 60 deletions(-)

diff --git a/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.MappingTables.cs b/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.MappingTables.cs
index 35d9cde9222fa..7e3209b56e8f7 100644
--- a/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.MappingTables.cs
+++ b/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.MappingTables.cs
@@ -566,74 +566,24 @@ public bool TryGetOffsetsRange(IntPtr functionPointer, out int firstParserOffset
         // ldftn reverse lookup hash. Must be cleared and reset if the module list changes. (All sets to
         // this variable must happen under a lock)
         private volatile KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>[] _ldftnReverseLookup_InvokeMap;
-        private Func<NativeFormatModuleInfo, FunctionPointersToOffsets> _computeLdFtnLookupInvokeMapInvokeMap = ComputeLdftnReverseLookup_InvokeMap;
 
         /// <summary>
-        /// Initialize a lookup array of module to function pointer/parser offset pair arrays. Do so in a manner that will allow
-        /// future work which will invalidate the cache (by setting it to null)
+        /// Initialize a lookup array of module to function pointer/parser offset pair arrays.
         /// </summary>
-        /// <param name="ldftnReverseLookupStatic">pointer to static which holds cache value. This is treated as a volatile variable</param>
-        /// <param name="lookupComputer"></param>
-        /// <returns></returns>
-        private KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>[] GetLdFtnReverseLookups_Helper(ref KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>[] ldftnReverseLookupStatic, Func<NativeFormatModuleInfo, FunctionPointersToOffsets> lookupComputer)
+        private KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>[] GetLdFtnReverseLookups_InvokeMap()
         {
-            KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>[] ldFtnReverseLookup = Volatile.Read(ref ldftnReverseLookupStatic);
-
-            if (ldFtnReverseLookup != null)
-                return ldFtnReverseLookup;
-            else
+            KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>[] ldFtnReverseLookup = _ldftnReverseLookup_InvokeMap;
+            if (ldFtnReverseLookup == null)
             {
-                lock (this)
+                var ldFtnReverseLookupBuilder = new ArrayBuilder<KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>>();
+                foreach (NativeFormatModuleInfo module in ModuleList.EnumerateModules())
                 {
-                    ldFtnReverseLookup = Volatile.Read(ref ldftnReverseLookupStatic);
-
-                    // double checked lock, safe due to use of volatile on s_ldftnReverseHashes
-                    if (ldFtnReverseLookup != null)
-                        return ldFtnReverseLookup;
-
-                    // FUTURE: add a module load callback to invalidate this cache if a new module is loaded.
-                    while (true)
-                    {
-                        int size = 0;
-                        foreach (NativeFormatModuleInfo module in ModuleList.EnumerateModules())
-                        {
-                            size++;
-                        }
-
-                        ldFtnReverseLookup = new KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>[size];
-                        int index = 0;
-                        bool restart = false;
-                        foreach (NativeFormatModuleInfo module in ModuleList.EnumerateModules())
-                        {
-                            // If the module list changes during execution of this code, rebuild from scratch
-                            if (index >= ldFtnReverseLookup.Length)
-                            {
-                                restart = true;
-                                break;
-                            }
-
-                            ldFtnReverseLookup[index] = new KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>(module, lookupComputer(module));
-                            index++;
-                        }
-
-                        if (restart)
-                            continue;
-
-                        // unless we need to repeat the module enumeration, only execute the body of this while loop once.
-                        break;
-                    }
-
-                    Volatile.Write(ref ldftnReverseLookupStatic, ldFtnReverseLookup);
-                    return ldFtnReverseLookup;
+                    ldFtnReverseLookupBuilder.Add(new KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>(module, ComputeLdftnReverseLookup_InvokeMap(module)));
                 }
+                ldFtnReverseLookup = ldFtnReverseLookupBuilder.ToArray();
+                _ldftnReverseLookup_InvokeMap = ldFtnReverseLookup;
             }
-        }
-
-        private KeyValuePair<NativeFormatModuleInfo, FunctionPointersToOffsets>[] GetLdFtnReverseLookups_InvokeMap()
-        {
-#pragma warning disable 0420 // GetLdFtnReverseLookups_Helper treats its first parameter as volatile by using explicit Volatile operations
-            return GetLdFtnReverseLookups_Helper(ref _ldftnReverseLookup_InvokeMap, _computeLdFtnLookupInvokeMapInvokeMap);
-#pragma warning restore 0420
+            return ldFtnReverseLookup;
         }
 
         internal unsafe void GetFunctionPointerAndInstantiationArgumentForOriginalLdFtnResult(IntPtr originalLdFtnResult, out IntPtr canonOriginalLdFtnResult, out IntPtr instantiationArgument)
diff --git a/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/System.Private.Reflection.Execution.csproj b/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/System.Private.Reflection.Execution.csproj
index 76d3934794fd2..5527d5920f601 100644
--- a/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/System.Private.Reflection.Execution.csproj
+++ b/src/coreclr/nativeaot/System.Private.Reflection.Execution/src/System.Private.Reflection.Execution.csproj
@@ -65,6 +65,9 @@
     <Compile Include="$(AotCommonPath)\System\Collections\Generic\LowLevelDictionary.cs">
       <Link>System\Collections\Generic\LowLevelDictionary.cs</Link>
     </Compile>
+    <Compile Include="$(CompilerCommonPath)\System\Collections\Generic\ArrayBuilder.cs">
+      <Link>System\Collections\Generic\ArrayBuilder.cs</Link>
+    </Compile>
     <Compile Include="$(CompilerCommonPath)\Internal\LowLevelLinq\LowLevelEnumerable.cs">
       <Link>Internal\LowLevelLinq\LowLevelEnumerable.cs</Link>
     </Compile>