Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inline resource strings in the compiler #80896

Merged
merged 6 commits into from
Jan 30, 2023
Merged

Inline resource strings in the compiler #80896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
22d8052
New approach
MichalStrehovsky Jan 25, 2023
6064206
Cleanups
MichalStrehovsky Jan 26, 2023
1280224
Update test
MichalStrehovsky Jan 26, 2023
d15a4a9
Apply suggestions from code review
MichalStrehovsky Jan 27, 2023
4878af6
Update InlineableStringsResourceNode.cs
MichalStrehovsky Jan 27, 2023
53ada6e
Update InlineableStringsResourceNode.cs
MichalStrehovsky Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down
77 changes: 77 additions & 0 deletions ...ools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// 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;
using Internal.TypeSystem;
using Internal.TypeSystem.Ecma;

namespace ILCompiler.DependencyAnalysis
{
/// <summary>
/// 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.
/// </summary>
internal sealed class InlineableStringsResourceNode : DependencyNodeCore<NodeFactory>
{
private readonly EcmaModule _module;

public const string ResourceAccessorTypeName = "SR";
public const string ResourceAccessorTypeNamespace = "System";
public const string ResourceAccessorGetStringMethodName = "GetResourceString";

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", StringComparison.Ordinal))
return false;

// 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";

if (resourceName != resourceName1 && resourceName != resourceName2)
return false;

MetadataType srType = module.GetType(ResourceAccessorTypeNamespace, ResourceAccessorTypeName, throwIfNotFound: false);
if (srType == null)
return false;

return srType.GetMethod(ResourceAccessorGetStringMethodName, null) != null;
}

public static void AddDependenciesDueToResourceStringUse(ref DependencyList dependencies, NodeFactory factory, MethodDesc method)
{
if (method.Name == ResourceAccessorGetStringMethodName && method.OwningType is MetadataType mdType
&& mdType.Name == ResourceAccessorTypeName && mdType.Namespace == ResourceAccessorTypeNamespace)
{
dependencies ??= new DependencyList();
dependencies.Add(factory.InlineableStringResource((EcmaModule)mdType.Module), "Using the System.SR class");
}
}

public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDependencies(NodeFactory context) => null;
public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFactory context) => null;
public override IEnumerable<CombinedDependencyListEntry> SearchDynamicDependencies(List<DependencyNodeCore<NodeFactory>> markedNodes, int firstNode, NodeFactory context) => null;
protected override string GetName(NodeFactory context)
=> $"String resources for {_module.Assembly.GetName().Name}";
}
}
11 changes: 11 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,11 @@ private void CreateNodeCaches()
return new ModuleMetadataNode(module);
});

_inlineableStringResources = new NodeCache<EcmaModule, InlineableStringsResourceNode>(module =>
{
return new InlineableStringsResourceNode(module);
});

_customAttributesWithMetadata = new NodeCache<ReflectableCustomAttribute, CustomAttributeMetadataNode>(ca =>
{
return new CustomAttributeMetadataNode(ca);
Expand Down Expand Up @@ -1139,6 +1144,12 @@ internal ModuleMetadataNode ModuleMetadata(ModuleDesc module)
return _modulesWithMetadata.GetOrAdd(module);
}

private NodeCache<EcmaModule, InlineableStringsResourceNode> _inlineableStringResources;
internal InlineableStringsResourceNode InlineableStringResource(EcmaModule module)
{
return _inlineableStringResources.GetOrAdd(module);
}

private NodeCache<ReflectableCustomAttribute, CustomAttributeMetadataNode> _customAttributesWithMetadata;

internal CustomAttributeMetadataNode CustomAttributeMetadata(ReflectableCustomAttribute ca)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ResourceDataNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public IReadOnlyList<ResourceIndexData> 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;
Expand Down
137 changes: 132 additions & 5 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@
using System.Collections.Generic;
using System.IO;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using System.Reflection.PortableExecutable;
using System.Resources;

using ILCompiler.DependencyAnalysis;

using Internal.IL;
using Internal.TypeSystem;
Expand Down Expand Up @@ -114,6 +118,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,
}
Expand All @@ -139,9 +146,17 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method)
// Last step is a sweep - we replace the tail of all unreachable blocks with "br $-2"
// and nop out the rest. If the basic block is smaller than 2 bytes, we don't touch it.
// We also eliminate any EH records that correspond to the stubbed out basic block.
//
// We also attempt to rewrite calls to SR.SomeResourceString accessors with string
// literals looked up from the managed resources.

Debug.Assert(method.GetMethodILDefinition() == method);

// Do not attempt to inline resource strings if we only want to use resource keys.
// The optimizations are not compatible.
bool shouldInlineResourceStrings =
MichalStrehovsky marked this conversation as resolved.
Show resolved Hide resolved
!_hashtable._switchValues.TryGetValue("System.Resources.UseSystemResourceKeys", out bool useResourceKeys) || !useResourceKeys;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any estimate on additional saving we would get if this feature also worked with UseSystemResourceKeys feature switch (some of the message are very long)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any estimate on additional saving we would get if this feature also worked with UseSystemResourceKeys feature switch (some of the message are very long)?

This will always be inlined - for the below app:

Console.WriteLine(new NullReferenceException().Message);
throw null;

If I compile this with .NET 7 as dotnet publish -c Release /p:PublishAot=true /p:UseSystemResourceKeys=true and step into the ctor, I see this (the first lea is loading the string literal of the resource key - inlined from the accessor):

00007FF787207950 56                   push        rsi  
00007FF787207951 48 83 EC 20          sub         rsp,20h  
00007FF787207955 48 8B F1             mov         rsi,rcx  
00007FF787207958 48 8D 0D 11 7B 17 00 lea         rcx,[__Str_Arg_NullReferenceException (07FF78737F470h)]  
00007FF78720795F E8 0C C2 00 00       call        S_P_CoreLib_System_SR__GetResourceString (07FF787213B70h)  
00007FF787207964 C7 46 48 00 15 13 80 mov         dword ptr [rsi+48h],80131500h  
00007FF78720796B 48 8D 4E 08          lea         rcx,[rsi+8]  
00007FF78720796F 48 8B D0             mov         rdx,rax  
00007FF787207972 E8 89 BE F4 FF       call        RhpAssignRefAVLocation (07FF787153800h)  
00007FF787207977 C7 46 48 01 15 13 80 mov         dword ptr [rsi+48h],80131501h  
00007FF78720797E C7 46 48 03 40 00 80 mov         dword ptr [rsi+48h],80004003h  
00007FF787207985 48 83 C4 20          add         rsp,20h  
00007FF787207989 5E                   pop         rsi  
00007FF78720798A C3                   ret

SR__GetResourceString didn't get inlined probably because the IL is messed up with too many nops and looks large even thought it isn't. It looks like this and there's only one of it, so not a huge deal size-wise, but we could leave fewer nops when substitutions happen to not to throw off inlining heuristics:

00007FF787213B70 48 8B C1             mov         rax,rcx  
00007FF787213B73 C3                   ret

vitek-karas marked this conversation as resolved.
Show resolved Hide resolved

ILExceptionRegion[] ehRegions = method.GetExceptionRegions();
byte[] methodBytes = method.GetILBytes();
OpcodeFlags[] flags = new OpcodeFlags[methodBytes.Length];
Expand Down Expand Up @@ -243,6 +258,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
Expand Down Expand Up @@ -384,6 +401,19 @@ 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 == InlineableStringsResourceNode.ResourceAccessorTypeName
&& calleeType.Namespace == InlineableStringsResourceNode.ResourceAccessorTypeNamespace
&& callee.Signature is { Length: 0, IsStatic: true }
&& callee.Name.StartsWith("get_", StringComparison.Ordinal))
{
flags[offset] |= OpcodeFlags.GetResourceStringCall;
hasGetResourceStringCall = true;
}
}
else
{
reader.Skip(opcode);
Expand All @@ -405,7 +435,7 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method)
}
}

if (!hasUnmarkedInstruction)
if (!hasUnmarkedInstruction && !hasGetResourceStringCall)
return method;

byte[] newBody = (byte[])methodBytes.Clone();
Expand Down Expand Up @@ -472,7 +502,47 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method)
debugInfo = new SubstitutedDebugInformation(debugInfo, sequencePoints.ToArray());
}

return new SubstitutedMethodIL(method, newBody, newEHRegions.ToArray(), debugInfo);
// We only optimize EcmaMethods because there we can find out the highest string token RID
// in use.
ArrayBuilder<string> newStrings = default;
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);
vitek-karas marked this conversation as resolved.
Show resolved Hide resolved

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());

// If we can't get the string, this might be something else.
string resourceString = GetResourceStringForAccessor(getter);
if (resourceString == null)
continue;

// 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)tokenRid;
newBody[offset + 2] = (byte)(tokenRid >> 8);
newBody[offset + 3] = (byte)(tokenRid >> 16);
newBody[offset + 4] = TokenTypeString;

tokenRid++;
}
}

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)
Expand Down Expand Up @@ -641,19 +711,36 @@ private bool TryGetConstantArgument(MethodIL methodIL, byte[] body, OpcodeFlags[
return false;
}

private string GetResourceStringForAccessor(EcmaMethod method)
{
Debug.Assert(method.Name.StartsWith("get_", StringComparison.Ordinal));
string resourceStringName = method.Name.Substring(4);

Dictionary<string, string> 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;
Expand All @@ -662,7 +749,23 @@ 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 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;
int maxRealTokenRid = ecmaMethodIL.Module.MetadataReader.GetHeapSize(HeapIndex.UserString);
if (rid >= maxRealTokenRid)
{
// Yep, string injected by us.
return _newStrings[rid - maxRealTokenRid];
}
}

return _wrappedMethodIL.GetObject(token, notFoundBehavior);
}
public override MethodDebugInformation GetDebugInfo() => _debugInfo;
}

Expand All @@ -682,9 +785,11 @@ public SubstitutedDebugInformation(MethodDebugInformation originalDebugInformati
public override IEnumerable<ILSequencePoint> GetSequencePoints() => _sequencePoints;
}

private const int TokenTypeString = 0x70; // CorTokenType for strings

private sealed class FeatureSwitchHashtable : LockFreeReaderHashtable<EcmaModule, AssemblyFeatureInfo>
{
private readonly Dictionary<string, bool> _switchValues;
internal readonly Dictionary<string, bool> _switchValues;
private readonly Logger _logger;

public FeatureSwitchHashtable(Logger logger, Dictionary<string, bool> switchValues)
Expand All @@ -710,6 +815,7 @@ private sealed class AssemblyFeatureInfo

public Dictionary<MethodDesc, BodySubstitution> BodySubstitutions { get; }
public Dictionary<FieldDesc, object> FieldSubstitutions { get; }
public Dictionary<string, string> InlineableResourceStrings { get; }

public AssemblyFeatureInfo(EcmaModule module, Logger logger, IReadOnlyDictionary<string, bool> featureSwitchValues)
{
Expand Down Expand Up @@ -741,6 +847,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<string, string>();

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;
}
}
}
}
}
Expand Down
Loading