Cleanup unused code from nativeaot and ilc #116480
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
...t/ILCompiler.MetadataTransform/Internal/Metadata/NativeFormat/Writer/NativeMetadataWriter.cs
Outdated
Show resolved
Hide resolved
…tadata/NativeFormat/Writer/NativeMetadataWriter.cs Co-authored-by: Austin Wise <[email protected]>
...t/ILCompiler.MetadataTransform/Internal/Metadata/NativeFormat/Writer/NativeMetadataWriter.cs
Show resolved
Hide resolved
| using System.Reflection; | ||
| using Debug = System.Diagnostics.Debug; | ||
| using ConditionalAttribute = System.Diagnostics.ConditionalAttribute; | ||
| using Internal.LowLevelLinq; |
There was a problem hiding this comment.
What's the impact of switching away from Internal.LowLevelLinq on ilc compiler binary size? Could you please share ilc.exe binary size before/after this PR?
There was a problem hiding this comment.
ilc is already using System.Linq in other components including object writer and dependency analysis. Will post the result later.
There was a problem hiding this comment.
12685KB -> 12735KB, it seems that size-optimized LINQ is still larger than naive implementation?
There was a problem hiding this comment.
It's now 12681KB in latest commit.
There was a problem hiding this comment.
Yes, the size-optimized LINQ is still a ton of code. It just avoids non-linear code size growth.
What would it take to delete LowLevelLinq completely? I think we either want to keep it; or delete it completely and take some size hit. Deleting it partially is kind of the worst of both worlds.
There was a problem hiding this comment.
What would it take to delete LowLevelLinq completely?
It could even improve size a bit. There are only a few use cases of it in both reflection runtime and ilc. Most of them are almost unnecessary and can be changed by exposing arrays instead of IEnumerable. Only very few cases need to be converted to manual for loops.
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Internal.LowLevelLinq; | ||
| using System.Linq; |
There was a problem hiding this comment.
We can avoid LINQ with a file local helper:
file static class FormatHelper
{
public static string Get<T>(T[] values, string label)
{
if (values == null || values.Length == 0)
return $"({label}[]) {{}}";
var result = $"({label}[]) {{{values[0]}";
for (int i = 1; i < values.Length; i++)
{
result += $", {values[i]}";
}
result += "}";
return result;
}
}
|
ILC size is 12685KB->12639KB for now. I think it's a reviewable state without too much change. |
| if ((value & SignatureCallingConvention.UnmanagedCallingConventionMask) != default) | ||
| { | ||
| value &= ~SignatureCallingConvention.UnmanagedCallingConventionMask; | ||
| values.Add((value & SignatureCallingConvention.UnmanagedCallingConventionMask).ToString()); |
There was a problem hiding this comment.
If I am reading this correctly, the previous line is going to clean all bits in UnmanagedCallingConventionMask and so this line is always going to print 0 or nothing.
There was a problem hiding this comment.
This is a copy-paste error. The string should be appended before clearing the flag.
| values.Add((value & SignatureCallingConvention.UnmanagedCallingConventionMask).ToString()); | ||
| } | ||
| values.Add(value.ToString()); | ||
| return string.Join(" | ", values); |
There was a problem hiding this comment.
Nit: This was wrapped in [ and ] before. Does the output look better without it?
There was a problem hiding this comment.
Well I totally missed that during refactor.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/ba-g known issue #116897 |
No description provided.