-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Reduce code bloat around formatted GetResourceString calls #7007
Conversation
LGTM. Thanks |
I just realized it is possible for the caller to have a pre-initialized array and pass it to object[] objects = { ... };
Environment.GetResourceString("fmt", objects); // before called GetResourceString(object[]), now calls GetResouceString(object) which is incorrect To mitigate any risk of this happening, I'll change the |
Actually, the noinlining attribute has extended meaning in corlib because of this check https://github.com/dotnet/coreclr/blob/master/src/vm/jitinterface.cpp#L6657. This change may be preventing inlinining in a lot more places than desired. It would be a good idea to check asm-diffs for bad deltas. @AndyAyersMS Are the instructions on how to run asm-diffs somewhere? |
There are some notes on getting diffs here. Please direct any follow up on diffing to @russellhadley. |
You'd end up with two no-lined methods in a row for all the ThrowHelper calls; but that's probably dwarfed by the exception being thrown (as a function that only throws does not inline - except in the cases that should be fixed by #6742) Would lessen code size for the functions that don't use ThrowHelper as |
@jkotas @jamesqo just run the diffs on the current coreclr (so it has the ThrowHelper reductions already in it) and this change looks good from a diff point of view?
|
Ah... should check the callers... |
@jkotas no its a bad change for the item you highlighted as it marks everything using Before
After
|
@benaadams Yeah, it's unfortunate that all methods in CoreLib marked NoInlining are automatically assumed to use StackCrawlMark (as pointed out by the code @jkotas linked to). Maybe we should consider introducing a new attribute, e.g. |
That's probably not the best example function as that actually might be better; as its a lot of exception stuff that's not inlined 😄 Will find something more mainstream |
Ok, something that isn't an exception .ctor, that shows the issue Before
After
So fairly major 😢 |
@jamesqo as So just adding it to [MethodImpl(MethodImplOptions.NoInlining)]
internal static String GetResourceStringLocal(String key) and [MethodImpl(MethodImplOptions.NoInlining)]
internal static String GetResourceString(String key, params Object[] values) Jit-Diff
|
Then the callers of the callers then go back to being inlined
@jkotas is this worth pursuing? Its the right results, but for the wrong reasons... |
__Error:WinIOError as is a big changer moves from
to
|
If there is a comment explaining it, it is fine with me. |
Currently formatted calls to
Environment.GetResourceString
, i.e.call the
GetResourceString(string, params object[])
overload. This requires allocating a new object array, which is done in the caller and unnecessarily bloats the code (since this is basically only called when we throw exceptions). I've made that overload private, and instead introduced a bunch of methods taggedNoInlining
which just forward to the params overload. This reduces theSystem.Private.CoreLib
size from 2376 -> 2372 (4 kB difference), and the size of the native image from 11060 -> 11037 (23 kB).Related pull request: #6634
@benaadams @jkotas @AndyAyersMS PTAL