From 22d8052c4263280fc0787907479c627b249175f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 25 Jan 2023 18:33:19 +0900 Subject: [PATCH 1/6] New approach --- .../Diagnostics/StackFrame.NativeAot.cs | 7 +- .../InlineableStringsResourceNode.cs | 69 +++++++++++ .../DependencyAnalysis/NodeFactory.cs | 11 ++ .../DependencyAnalysis/ResourceDataNode.cs | 2 +- .../Compiler/FeatureSwitchManager.cs | 112 +++++++++++++++++- .../Compiler/MetadataManager.cs | 14 ++- .../ILCompiler.Compiler.csproj | 1 + src/libraries/Common/src/System/SR.cs | 2 +- .../src/System/Text/EncodingData.cs | 35 ------ .../src/System/Text/EncodingTable.cs | 31 +++-- 10 files changed, 228 insertions(+), 56 deletions(-) create mode 100644 src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs index 540f549ead2381..e920b34259fede 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs @@ -163,15 +163,16 @@ internal void AppendToStackTrace(StringBuilder builder) { // Passing a default string for "at" in case SR.UsingResourceKeys() is true // as this is a special case and we don't want to have "Word_At" on stack traces. - string word_At = SR.GetResourceString(nameof(SR.Word_At), defaultString: "at"); + string word_At = SR.UsingResourceKeys() ? "at" : SR.Word_At; builder.Append(" ").Append(word_At).Append(' '); builder.AppendLine(DeveloperExperience.Default.CreateStackTraceString(_ipAddress, _needFileInfo)); } if (_isLastFrameFromForeignExceptionStackTrace) { // Passing default for Exception_EndStackTraceFromPreviousThrow in case SR.UsingResourceKeys is set. - builder.AppendLine(SR.GetResourceString(nameof(SR.Exception_EndStackTraceFromPreviousThrow), - defaultString: "--- End of stack trace from previous location ---")); + builder.AppendLine(SR.UsingResourceKeys() ? + "--- End of stack trace from previous location ---" : + SR.Exception_EndStackTraceFromPreviousThrow); } } } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs new file mode 100644 index 00000000000000..5e7714a7f8ba91 --- /dev/null +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs @@ -0,0 +1,69 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; + +using ILCompiler.DependencyAnalysisFramework; +using Internal.TypeSystem; +using Internal.TypeSystem.Ecma; + +namespace ILCompiler.DependencyAnalysis +{ + /// + /// Represents a resource blob used by the SR class in the BCL. + /// If this node is present in the graph, it means we were not able to optimize its use away + /// and the blob has to be generated. + /// + internal sealed class InlineableStringsResourceNode : DependencyNodeCore + { + private readonly EcmaModule _module; + + public InlineableStringsResourceNode(EcmaModule module) + { + _module = module; + } + + public override bool InterestingForDynamicDependencyAnalysis => false; + + public override bool HasDynamicDependencies => false; + + public override bool HasConditionalStaticDependencies => false; + + public override bool StaticDependenciesAreComputed => true; + + public static bool IsInlineableStringsResource(EcmaModule module, string resourceName) + { + if (!resourceName.EndsWith(".resources")) + return false; + + // TODO: we should grab this name from the SR class + string resourceName1 = $"{module.Assembly.GetName().Name}.Strings.resources"; + string resourceName2 = $"FxResources.{module.Assembly.GetName().Name}.SR.resources"; + + if (resourceName != resourceName1 && resourceName != resourceName2) + return false; + + MetadataType srType = module.GetType("System", "SR", throwIfNotFound: false); + if (srType == null) + return false; + + return srType.GetMethod("GetResourceString", null) != null; + } + + public static void AddDependenciesDueToResourceStringUse(ref DependencyList dependencies, NodeFactory factory, MethodDesc method) + { + if (method.Name == "GetResourceString" && method.OwningType is MetadataType mdType + && mdType.Name == "SR" && mdType.Namespace == "System") + { + dependencies ??= new DependencyList(); + dependencies.Add(factory.InlineableStringResource((EcmaModule)mdType.Module), "Using the System.SR class"); + } + } + + public override IEnumerable GetConditionalStaticDependencies(NodeFactory context) => null; + public override IEnumerable GetStaticDependencies(NodeFactory context) => null; + public override IEnumerable SearchDynamicDependencies(List> markedNodes, int firstNode, NodeFactory context) => null; + protected override string GetName(NodeFactory context) + => $"String resources for {_module.Assembly.GetName().Name}"; + } +} diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs index 196a51ea2c31f5..ba0f5be86ea0f6 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs @@ -462,6 +462,11 @@ private void CreateNodeCaches() return new ModuleMetadataNode(module); }); + _inlineableStringResources = new NodeCache(module => + { + return new InlineableStringsResourceNode(module); + }); + _customAttributesWithMetadata = new NodeCache(ca => { return new CustomAttributeMetadataNode(ca); @@ -1139,6 +1144,12 @@ internal ModuleMetadataNode ModuleMetadata(ModuleDesc module) return _modulesWithMetadata.GetOrAdd(module); } + private NodeCache _inlineableStringResources; + internal InlineableStringsResourceNode InlineableStringResource(EcmaModule module) + { + return _inlineableStringResources.GetOrAdd(module); + } + private NodeCache _customAttributesWithMetadata; internal CustomAttributeMetadataNode CustomAttributeMetadata(ReflectableCustomAttribute ca) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ResourceDataNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ResourceDataNode.cs index 19fe3250804d76..903d4c21ffb9e1 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ResourceDataNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ResourceDataNode.cs @@ -98,7 +98,7 @@ public IReadOnlyList GetOrCreateIndexData(NodeFactory factory string resourceName = module.MetadataReader.GetString(resource.Name); // Check if emitting the manifest resource is blocked by policy. - if (factory.MetadataManager.IsManifestResourceBlocked(module, resourceName)) + if (factory.MetadataManager.IsManifestResourceBlocked(factory, module, resourceName)) continue; string assemblyName = module.GetName().FullName; diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs index cbe2c0adf25774..91c0ac45df2379 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs @@ -6,6 +6,9 @@ using System.IO; using System.Reflection.Metadata; using System.Reflection.PortableExecutable; +using System.Resources; + +using ILCompiler.DependencyAnalysis; using Internal.IL; using Internal.TypeSystem; @@ -114,6 +117,9 @@ private enum OpcodeFlags : byte // (Lets us avoid seeing lots of small basic blocks within eliminated chunks.) VisibleBasicBlockStart = 0x10, + // This is a potential SR.get_SomeResourceString call. + GetResourceStringCall = 0x20, + // The instruction at this offset is reachable Mark = 0x80, } @@ -142,6 +148,9 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method) Debug.Assert(method.GetMethodILDefinition() == method); + bool shouldInlineResourceStrings = + !_hashtable._switchValues.TryGetValue("System.Resources.UseSystemResourceKeys", out bool useResourceKeys) || !useResourceKeys; + ILExceptionRegion[] ehRegions = method.GetExceptionRegions(); byte[] methodBytes = method.GetILBytes(); OpcodeFlags[] flags = new OpcodeFlags[methodBytes.Length]; @@ -243,6 +252,8 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method) } } + bool hasGetResourceStringCall = false; + // Mark all reachable basic blocks // // We also do another round of basic block marking to mark beginning of visible basic blocks @@ -384,6 +395,18 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method) if (reader.HasNext) flags[reader.Offset] |= OpcodeFlags.VisibleBasicBlockStart; } + else if (shouldInlineResourceStrings && opcode == ILOpcode.call) + { + var callee = method.GetObject(reader.ReadILToken(), NotFoundBehavior.ReturnNull) as EcmaMethod; + if (callee != null && callee.IsSpecialName && callee.OwningType is EcmaType calleeType + && calleeType.Name == "SR" && calleeType.Namespace == "System" + && callee.Signature is { Length: 0, IsStatic: true } + && callee.Name.StartsWith("get_")) + { + flags[offset] |= OpcodeFlags.GetResourceStringCall; + hasGetResourceStringCall = true; + } + } else { reader.Skip(opcode); @@ -405,7 +428,7 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method) } } - if (!hasUnmarkedInstruction) + if (!hasUnmarkedInstruction && !hasGetResourceStringCall) return method; byte[] newBody = (byte[])methodBytes.Clone(); @@ -472,7 +495,33 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method) debugInfo = new SubstitutedDebugInformation(debugInfo, sequencePoints.ToArray()); } - return new SubstitutedMethodIL(method, newBody, newEHRegions.ToArray(), debugInfo); + ArrayBuilder newStrings = default; + if (hasGetResourceStringCall) + { + for (int offset = 0; offset < flags.Length; offset++) + { + if ((flags[offset] & OpcodeFlags.GetResourceStringCall) == 0) + continue; + + Debug.Assert(newBody[offset] == (byte)ILOpcode.call); + var getter = (EcmaMethod)method.GetObject(new ILReader(newBody, offset + 1).ReadILToken()); + + string resourceString = GetResourceStringForAccessor(getter); + if (resourceString == null) + continue; + + int tokenValue = NewStringTokenRidStart - newStrings.Count; + newStrings.Add(resourceString); + + newBody[offset] = (byte)ILOpcode.ldstr; + newBody[offset + 1] = (byte)tokenValue; + newBody[offset + 2] = (byte)(tokenValue >> 8); + newBody[offset + 3] = (byte)(tokenValue >> 16); + newBody[offset + 4] = 0x70; + } + } + + return new SubstitutedMethodIL(method, newBody, newEHRegions.ToArray(), debugInfo, newStrings.ToArray()); } private bool TryGetConstantArgument(MethodIL methodIL, byte[] body, OpcodeFlags[] flags, int offset, int argIndex, out int constant) @@ -641,19 +690,36 @@ private bool TryGetConstantArgument(MethodIL methodIL, byte[] body, OpcodeFlags[ return false; } + private string GetResourceStringForAccessor(EcmaMethod method) + { + Debug.Assert(method.Name.StartsWith("get_")); + string resourceStringName = method.Name.Substring(4); + + Dictionary dict = _hashtable.GetOrCreateValue(method.Module).InlineableResourceStrings; + if (dict != null + && dict.TryGetValue(resourceStringName, out string result)) + { + return result; + } + + return null; + } + private sealed class SubstitutedMethodIL : MethodIL { private readonly byte[] _body; private readonly ILExceptionRegion[] _ehRegions; private readonly MethodIL _wrappedMethodIL; private readonly MethodDebugInformation _debugInfo; + private readonly string[] _newStrings; - public SubstitutedMethodIL(MethodIL wrapped, byte[] body, ILExceptionRegion[] ehRegions, MethodDebugInformation debugInfo) + public SubstitutedMethodIL(MethodIL wrapped, byte[] body, ILExceptionRegion[] ehRegions, MethodDebugInformation debugInfo, string[] newStrings) { _wrappedMethodIL = wrapped; _body = body; _ehRegions = ehRegions; _debugInfo = debugInfo; + _newStrings = newStrings; } public override MethodDesc OwningMethod => _wrappedMethodIL.OwningMethod; @@ -662,7 +728,19 @@ public SubstitutedMethodIL(MethodIL wrapped, byte[] body, ILExceptionRegion[] eh public override ILExceptionRegion[] GetExceptionRegions() => _ehRegions; public override byte[] GetILBytes() => _body; public override LocalVariableDefinition[] GetLocals() => _wrappedMethodIL.GetLocals(); - public override object GetObject(int token, NotFoundBehavior notFoundBehavior) => _wrappedMethodIL.GetObject(token, notFoundBehavior); + public override object GetObject(int token, NotFoundBehavior notFoundBehavior) + { + if ((token >>> 24) == 0x70) + { + int rid = token & 0xFFFFFF; + if (rid > (NewStringTokenRidStart - _newStrings.Length)) + { + return _newStrings[-(rid - 0xFFFFFF)]; + } + } + + return _wrappedMethodIL.GetObject(token, notFoundBehavior); + } public override MethodDebugInformation GetDebugInfo() => _debugInfo; } @@ -682,9 +760,11 @@ public SubstitutedDebugInformation(MethodDebugInformation originalDebugInformati public override IEnumerable GetSequencePoints() => _sequencePoints; } + private const int NewStringTokenRidStart = 0xFFFFFF; + private sealed class FeatureSwitchHashtable : LockFreeReaderHashtable { - private readonly Dictionary _switchValues; + internal readonly Dictionary _switchValues; private readonly Logger _logger; public FeatureSwitchHashtable(Logger logger, Dictionary switchValues) @@ -710,6 +790,7 @@ private sealed class AssemblyFeatureInfo public Dictionary BodySubstitutions { get; } public Dictionary FieldSubstitutions { get; } + public Dictionary InlineableResourceStrings { get; } public AssemblyFeatureInfo(EcmaModule module, Logger logger, IReadOnlyDictionary featureSwitchValues) { @@ -741,6 +822,27 @@ public AssemblyFeatureInfo(EcmaModule module, Logger logger, IReadOnlyDictionary (BodySubstitutions, FieldSubstitutions) = BodySubstitutionsParser.GetSubstitutions(logger, module.Context, ms, resource, module, "name", featureSwitchValues); } + else if (InlineableStringsResourceNode.IsInlineableStringsResource(module, resourceName)) + { + BlobReader reader = resourceDirectory.GetReader((int)resource.Offset, resourceDirectory.Length - (int)resource.Offset); + int length = (int)reader.ReadUInt32(); + + UnmanagedMemoryStream ms; + unsafe + { + ms = new UnmanagedMemoryStream(reader.CurrentPointer, length); + } + + InlineableResourceStrings = new Dictionary(); + + using var resReader = new ResourceReader(ms); + var enumerator = resReader.GetEnumerator(); + while (enumerator.MoveNext()) + { + if (enumerator.Key is string key && enumerator.Value is string value) + InlineableResourceStrings[key] = value; + } + } } } } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs index d5d2d7bdfd3861..19d8a8a0f99a29 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MetadataManager.cs @@ -526,6 +526,8 @@ public void GetDependenciesDueToMethodCodePresence(ref DependencyList dependenci ExactMethodInstantiationsNode.GetExactMethodInstantiationDependenciesForMethod(ref dependencies, factory, method); } + InlineableStringsResourceNode.AddDependenciesDueToResourceStringUse(ref dependencies, factory, method); + GetDependenciesDueToMethodCodePresenceInternal(ref dependencies, factory, method, methodIL); } @@ -839,9 +841,17 @@ public bool IsReflectionBlocked(MethodDesc method) return _blockingPolicy.IsBlocked(typicalMethodDefinition); } - public bool IsManifestResourceBlocked(ModuleDesc module, string resourceName) + public bool IsManifestResourceBlocked(NodeFactory factory, Internal.TypeSystem.Ecma.EcmaModule module, string resourceName) { - return _resourceBlockingPolicy.IsManifestResourceBlocked(module, resourceName); + if (_resourceBlockingPolicy.IsManifestResourceBlocked(module, resourceName)) + return true; + + // If this is a resource strings resource but we don't actually need it, block it. + if (InlineableStringsResourceNode.IsInlineableStringsResource(module, resourceName) + && !factory.InlineableStringResource(module).Marked) + return true; + + return false; } public bool CanGenerateMetadata(MetadataType type) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj b/src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj index 33c4e33bde641b..db2e9308dc0a6b 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj @@ -393,6 +393,7 @@ + diff --git a/src/libraries/Common/src/System/SR.cs b/src/libraries/Common/src/System/SR.cs index ec25620f01b56b..96fcadafb5cda0 100644 --- a/src/libraries/Common/src/System/SR.cs +++ b/src/libraries/Common/src/System/SR.cs @@ -13,7 +13,7 @@ internal static partial class SR // by default it returns the value of System.Resources.UseSystemResourceKeys AppContext switch or false if not specified. // Native code generators can replace the value this returns based on user input at the time of native code generation. // The Linker is also capable of replacing the value of this method when the application is being trimmed. - private static bool UsingResourceKeys() => s_usingResourceKeys; + internal static bool UsingResourceKeys() => s_usingResourceKeys; internal static string GetResourceString(string resourceKey) { diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/EncodingData.cs b/src/libraries/System.Private.CoreLib/src/System/Text/EncodingData.cs index 2b621d6bce9fe1..88689f57d6a92f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/EncodingData.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/EncodingData.cs @@ -237,41 +237,6 @@ internal static partial class EncodingTable 56 }; - // - // EnglishNames is the concatenation of the English names for each codepage. - // It is used in retrieving the value for System.Text.Encoding.EncodingName - // given System.Text.Encoding.CodePage. - // This is done rather than using a large readonly array of strings to avoid - // generating a large amount of code in the static constructor. - // - private const string EnglishNames = - "Unicode" + // 1200 - "Unicode (Big-Endian)" + // 1201 - "Unicode (UTF-32)" + // 12000 - "Unicode (UTF-32 Big-Endian)" + // 12001 - "US-ASCII" + // 20127 - "Western European (ISO)" + // 28591 - "Unicode (UTF-7)" + // 65000 - "Unicode (UTF-8)"; // 65001 - - // - // EnglishNameIndices contains the start index of each code page's English - // name in the string EnglishNames. It is indexed by an index into - // MappedCodePages. - // - private static ReadOnlySpan EnglishNameIndices => new int[] - { - 0, // Unicode (1200) - 7, // Unicode (Big-Endian) (1201) - 27, // Unicode (UTF-32) (12000) - 43, // Unicode (UTF-32 Big-Endian) (12001) - 70, // US-ASCII (20127) - 78, // Western European (ISO) (28591) - 100, // Unicode (UTF-7) (65000) - 115, // Unicode (UTF-8) (65001) - 130 - }; - // redeclaring these constants here for readability below private const uint MIMECONTF_MAILNEWS = Encoding.MIMECONTF_MAILNEWS; private const uint MIMECONTF_BROWSER = Encoding.MIMECONTF_BROWSER; diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/EncodingTable.cs b/src/libraries/System.Private.CoreLib/src/System/Text/EncodingTable.cs index e404065b099bb9..0a98259f315379 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/EncodingTable.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/EncodingTable.cs @@ -124,7 +124,7 @@ internal static EncodingInfo[] GetEncodings() arrayEncodingInfo[arrayEncodingInfoIdx++] = new EncodingInfo( codePage, webNames[webNameIndices[i]..webNameIndices[i + 1]], - GetDisplayName(codePage, i) + GetDisplayName(codePage) ); } @@ -151,7 +151,7 @@ internal static EncodingInfo[] GetEncodings(Dictionary encodi if (codePage != Encoding.CodePageUTF7 || LocalAppContextSwitches.EnableUnsafeUTF7Encoding) { encodingInfoList[codePage] = new EncodingInfo(codePage, webNames[webNameIndices[i]..webNameIndices[i + 1]], - GetDisplayName(codePage, i)); + GetDisplayName(codePage)); } } } @@ -229,19 +229,32 @@ private static CodePageDataItem InternalGetCodePageDataItem(int codePage, int in // All supported code pages have identical header names, and body names. string headerName = webName; string bodyName = webName; - string displayName = GetDisplayName(codePage, index); + string displayName = GetDisplayName(codePage); uint flags = Flags[index]; return new CodePageDataItem(uiFamilyCodePage, webName, headerName, bodyName, displayName, flags); } - private static string GetDisplayName(int codePage, int englishNameIndex) + private static string GetDisplayName(int codePage) { - string? displayName = SR.GetResourceString("Globalization_cp_" + codePage.ToString()); - if (string.IsNullOrEmpty(displayName)) - displayName = EnglishNames[EnglishNameIndices[englishNameIndex]..EnglishNameIndices[englishNameIndex + 1]]; - - return displayName; + return codePage switch + { + 1200 => SR.Globalization_cp_1200, + 1201 => SR.Globalization_cp_1201, + 12000 => SR.Globalization_cp_12000, + 12001 => SR.Globalization_cp_12001, + 20127 => SR.Globalization_cp_20127, + 28591 => SR.Globalization_cp_28591, + 65000 => SR.Globalization_cp_65000, + 65001 => SR.Globalization_cp_65001, + _ => AssertFailAndEmptyString() + }; + + static string AssertFailAndEmptyString() + { + Debug.Fail("Unexpected code page"); + return ""; + } } } } From 60642063312535a9223daa3e198d813f0a8c9ff6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 26 Jan 2023 14:59:34 +0900 Subject: [PATCH 2/6] Cleanups --- .../InlineableStringsResourceNode.cs | 12 +++-- .../Compiler/FeatureSwitchManager.cs | 51 ++++++++++++++----- .../src/System/Text/EncodingTable.cs | 26 ++++------ 3 files changed, 57 insertions(+), 32 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs index 5e7714a7f8ba91..d133a4989e0aad 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs @@ -18,6 +18,10 @@ internal sealed class InlineableStringsResourceNode : DependencyNodeCore newStrings = default; - if (hasGetResourceStringCall) + if (hasGetResourceStringCall && method.GetMethodILDefinition() is EcmaMethodIL ecmaMethodIL) { + // We're going to inject new string tokens. Start where the last token of the module left off. + // We don't need this token to be globally unique because all token resolution happens in the context + // of a MethodIL and we're making a new one here. It just has to be unique to the MethodIL. + int tokenRid = ecmaMethodIL.Module.MetadataReader.GetHeapSize(HeapIndex.UserString); + for (int offset = 0; offset < flags.Length; offset++) { if ((flags[offset] & OpcodeFlags.GetResourceStringCall) == 0) @@ -506,18 +520,25 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method) Debug.Assert(newBody[offset] == (byte)ILOpcode.call); var getter = (EcmaMethod)method.GetObject(new ILReader(newBody, offset + 1).ReadILToken()); + // If we can't get the string, this might be something else. string resourceString = GetResourceStringForAccessor(getter); if (resourceString == null) continue; - int tokenValue = NewStringTokenRidStart - newStrings.Count; + // If we ran out of tokens, we can't optimize anymore. + if (tokenRid > 0xFFFFFF) + continue; + newStrings.Add(resourceString); + // call and ldstr are both 5-byte instructions: opcode followed by a token. newBody[offset] = (byte)ILOpcode.ldstr; - newBody[offset + 1] = (byte)tokenValue; - newBody[offset + 2] = (byte)(tokenValue >> 8); - newBody[offset + 3] = (byte)(tokenValue >> 16); - newBody[offset + 4] = 0x70; + newBody[offset + 1] = (byte)tokenRid; + newBody[offset + 2] = (byte)(tokenRid >> 8); + newBody[offset + 3] = (byte)(tokenRid >> 16); + newBody[offset + 4] = TokenTypeString; + + tokenRid++; } } @@ -692,7 +713,7 @@ private bool TryGetConstantArgument(MethodIL methodIL, byte[] body, OpcodeFlags[ private string GetResourceStringForAccessor(EcmaMethod method) { - Debug.Assert(method.Name.StartsWith("get_")); + Debug.Assert(method.Name.StartsWith("get_", StringComparison.Ordinal)); string resourceStringName = method.Name.Substring(4); Dictionary dict = _hashtable.GetOrCreateValue(method.Module).InlineableResourceStrings; @@ -730,12 +751,16 @@ public SubstitutedMethodIL(MethodIL wrapped, byte[] body, ILExceptionRegion[] eh public override LocalVariableDefinition[] GetLocals() => _wrappedMethodIL.GetLocals(); public override object GetObject(int token, NotFoundBehavior notFoundBehavior) { - if ((token >>> 24) == 0x70) + // If this is a string token, it could be one of the new string tokens we injected. + if ((token >>> 24) == TokenTypeString + && _wrappedMethodIL.GetMethodILDefinition() is EcmaMethodIL ecmaMethodIL) { int rid = token & 0xFFFFFF; - if (rid > (NewStringTokenRidStart - _newStrings.Length)) + int maxRealTokenRid = ecmaMethodIL.Module.MetadataReader.GetHeapSize(HeapIndex.UserString); + if (rid >= maxRealTokenRid) { - return _newStrings[-(rid - 0xFFFFFF)]; + // Yep, string injected by us. + return _newStrings[rid - maxRealTokenRid]; } } @@ -760,7 +785,7 @@ public SubstitutedDebugInformation(MethodDebugInformation originalDebugInformati public override IEnumerable GetSequencePoints() => _sequencePoints; } - private const int NewStringTokenRidStart = 0xFFFFFF; + private const int TokenTypeString = 0x70; // CorTokenType for strings private sealed class FeatureSwitchHashtable : LockFreeReaderHashtable { diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/EncodingTable.cs b/src/libraries/System.Private.CoreLib/src/System/Text/EncodingTable.cs index 0a98259f315379..c3b043af865cbd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/EncodingTable.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/EncodingTable.cs @@ -237,24 +237,20 @@ private static CodePageDataItem InternalGetCodePageDataItem(int codePage, int in private static string GetDisplayName(int codePage) { - return codePage switch + switch (codePage) { - 1200 => SR.Globalization_cp_1200, - 1201 => SR.Globalization_cp_1201, - 12000 => SR.Globalization_cp_12000, - 12001 => SR.Globalization_cp_12001, - 20127 => SR.Globalization_cp_20127, - 28591 => SR.Globalization_cp_28591, - 65000 => SR.Globalization_cp_65000, - 65001 => SR.Globalization_cp_65001, - _ => AssertFailAndEmptyString() + case 1200: return SR.Globalization_cp_1200; + case 1201: return SR.Globalization_cp_1201; + case 12000: return SR.Globalization_cp_12000; + case 12001: return SR.Globalization_cp_12001; + case 20127: return SR.Globalization_cp_20127; + case 28591: return SR.Globalization_cp_28591; + case 65000: return SR.Globalization_cp_65000; + case 65001: return SR.Globalization_cp_65001; }; - static string AssertFailAndEmptyString() - { - Debug.Fail("Unexpected code page"); - return ""; - } + Debug.Fail("Unexpected code page"); + return ""; } } } From 1280224705d6ac895ea67f847b481f08cb97c21d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 26 Jan 2023 16:19:59 +0900 Subject: [PATCH 3/6] Update test We can now optimize away the assembly even when resource strings are in use. --- src/tests/nativeaot/SmokeTests/FrameworkStrings/Program.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/tests/nativeaot/SmokeTests/FrameworkStrings/Program.cs b/src/tests/nativeaot/SmokeTests/FrameworkStrings/Program.cs index 6f4cb0fb10c566..73b0b99b29b6ee 100644 --- a/src/tests/nativeaot/SmokeTests/FrameworkStrings/Program.cs +++ b/src/tests/nativeaot/SmokeTests/FrameworkStrings/Program.cs @@ -56,7 +56,6 @@ Console.WriteLine("Resources in reflection library:"); string[] refNames; const string reflectionAssembly = "System.Private.Reflection.Execution"; -#if RESOURCE_KEYS try { refNames = System.Reflection.Assembly.Load(reflectionAssembly).GetManifestResourceNames(); @@ -65,9 +64,6 @@ { refNames = Array.Empty(); } -#else -refNames = System.Reflection.Assembly.Load(reflectionAssembly).GetManifestResourceNames(); -#endif foreach (var name in refNames) Console.WriteLine(name); From d15a4a99fe12576415903d9314fbf29a1cc2a551 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 27 Jan 2023 17:51:00 +0900 Subject: [PATCH 4/6] Apply suggestions from code review Co-authored-by: Jan Kotas --- .../DependencyAnalysis/InlineableStringsResourceNode.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs index d133a4989e0aad..e03791ec969bc5 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs @@ -37,12 +37,13 @@ public InlineableStringsResourceNode(EcmaModule module) public static bool IsInlineableStringsResource(EcmaModule module, string resourceName) { - if (!resourceName.EndsWith(".resources")) + if (!resourceName.EndsWith(".resources", StringComparison.Ordinal)) return false; // TODO: we should grab this name from the SR class - string resourceName1 = $"{module.Assembly.GetName().Name}.Strings.resources"; - string resourceName2 = $"FxResources.{module.Assembly.GetName().Name}.SR.resources"; + string simpleName = module.Assembly.GetName().Name; + string resourceName1 = $"{simpleName}.Strings.resources"; + string resourceName2 = $"FxResources.{simpleName}.SR.resources"; if (resourceName != resourceName1 && resourceName != resourceName2) return false; From 4878af6694537cbfc2a8c2c2b89acc40f4962e5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 27 Jan 2023 18:15:57 +0900 Subject: [PATCH 5/6] Update InlineableStringsResourceNode.cs --- .../Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs index e03791ec969bc5..018c73d5de9cd6 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using System.Collections.Generic; using ILCompiler.DependencyAnalysisFramework; From 53ada6ee2f4f0294a6b4fd2639c521f084a72c55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 31 Jan 2023 07:26:56 +0900 Subject: [PATCH 6/6] Update InlineableStringsResourceNode.cs --- .../DependencyAnalysis/InlineableStringsResourceNode.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs index 018c73d5de9cd6..b696088108bee8 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs @@ -41,7 +41,9 @@ public static bool IsInlineableStringsResource(EcmaModule module, string resourc if (!resourceName.EndsWith(".resources", StringComparison.Ordinal)) return false; - // TODO: we should grab this name from the SR class + // Make a guess at the name of the resource Arcade tooling generated for the resource + // strings. + // https://github.com/dotnet/runtime/issues/81385 tracks not having to guess this. string simpleName = module.Assembly.GetName().Name; string resourceName1 = $"{simpleName}.Strings.resources"; string resourceName2 = $"FxResources.{simpleName}.SR.resources";