Updated formatted GetResourceString code reduction by benaadams · Pull Request #7136 · dotnet/coreclr

benaadams · 2016-09-11T21:39:22Z

Update to: "Reduce code bloat around formatted GetResourceString calls" #7007

Addresses the issues raised in #7007 (comment)

Jit-Diff

Summary:
(Note: Lower is better)

Total bytes of diff: -17495 (-0.54 % of base)
    diff is an improvement.

Total byte diff includes 325 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    3 unique methods,      325 unique bytes

Top file improvements by size (bytes):
      -17495 : System.Private.CoreLib.dasm (-0.54 % of base)

1 total files with size differences.

Top method regessions by size (bytes):
         128 : System.Private.CoreLib.dasm - Environment:GetResourceString(ref,ref,ref,ref,ref):ref
         110 : System.Private.CoreLib.dasm - Environment:GetResourceString(ref,ref,ref,ref):ref
          91 : System.Private.CoreLib.dasm - Environment:GetResourceString(ref,ref):ref
          87 : System.Private.CoreLib.dasm - Environment:GetResourceString(ref,ref,ref):ref
          45 : System.Private.CoreLib.dasm - CustomAttributeTypedArgument:EncodedValueToRawValue(long,int):ref

Top method improvements by size (bytes):
        -845 : System.Private.CoreLib.dasm - CLRIPropertyValueImpl:CoerceArrayValue(int):ref:this
        -440 : System.Private.CoreLib.dasm - CLRIPropertyValueImpl:CoerceScalarValue(int,ref):int
        -426 : System.Private.CoreLib.dasm - CLRIPropertyValueImpl:CoerceScalarValue(int,ref):long
        -235 : System.Private.CoreLib.dasm - __Error:WinIOError(int,ref)
        -213 : System.Private.CoreLib.dasm - CLRIPropertyValueImpl:CoerceScalarValue(int,ref):ubyte

371 total methods with size differences.

/cc @jamesqo

benaadams · 2016-09-11T21:43:32Z

@jamesqo not sure if updating your PR breaks OSS protocol?

khellang · 2016-09-11T22:21:02Z

I'd probably do it the other way around though. Send a PR to @jamesqo's fork, targeting the less-resource-bloat branch and have it automatically included in #7007 when merged 😄

benaadams · 2016-09-11T22:31:59Z

Good idea

jamesqo · 2016-09-11T22:32:13Z

Hah, no, it's fine. I've closed my PR. I wasn't working on it since I'm planning to introduce a new attribute specifically to denote methods that use StackCrawlMark, so then NoInlining methods that do not use those types of variables will not be pessimized. But in the meantime, this is probably OK to go ahead with.

benaadams · 2016-09-11T22:32:15Z

Can't do that, branch has been deleted 😢

benaadams · 2016-09-11T22:33:08Z

Ah ok 😄

/cc @jkotas

jamesqo · 2016-09-11T22:35:19Z

+
+        // NoInlining causes the caller and callee to not be inlined in mscorlib as it is an assumption of StackCrawlMark use
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static String GetResourceString(string key, params object[] values)


This should be kept internal, otherwise things like

Environment.GetResourceString("Foo", new object[] { ... });

will break since they now call the (string, object) overload.

Ah, ok, then have to move the no-inlining about

benaadams · 2016-09-11T22:45:43Z

Re-analysing with code adjustment

jamesqo · 2016-09-11T22:47:18Z

        // NoInlining causes the caller and callee to not be inlined in mscorlib as it is an assumption of StackCrawlMark use
        [MethodImpl(MethodImplOptions.NoInlining)]
-        private static String GetResourceString(string key, params object[] values)
+        private static String GetResourceString(object[] values, string key)


Instead of shifting around all of the parameters which will make this confusing for readers + generates a larger git diff, you could just call this GetResourceStringCore or GetResourceStringInternal?

Shouldn't change the git diff? Anyway less confusion makes sense, made the change

benaadams · 2016-09-11T22:58:53Z

Still good

Summary:
(Note: Lower is better)

Total bytes of diff: -17495 (-0.54 % of base)
    diff is an improvement.

Total byte diff includes 325 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    3 unique methods,      325 unique bytes

Top file improvements by size (bytes):
      -17495 : System.Private.CoreLib.dasm (-0.54 % of base)

1 total files with size differences.

Top method regessions by size (bytes):
         128 : System.Private.CoreLib.dasm - Environment:GetResourceString(ref,ref,ref,ref,ref):ref
         110 : System.Private.CoreLib.dasm - Environment:GetResourceString(ref,ref,ref,ref):ref
          91 : System.Private.CoreLib.dasm - Environment:GetResourceString(ref,ref):ref
          87 : System.Private.CoreLib.dasm - Environment:GetResourceString(ref,ref,ref):ref
          45 : System.Private.CoreLib.dasm - CustomAttributeTypedArgument:EncodedValueToRawValue(long,int):ref

Top method improvements by size (bytes):
        -845 : System.Private.CoreLib.dasm - CLRIPropertyValueImpl:CoerceArrayValue(int):ref:this
        -440 : System.Private.CoreLib.dasm - CLRIPropertyValueImpl:CoerceScalarValue(int,ref):int
        -426 : System.Private.CoreLib.dasm - CLRIPropertyValueImpl:CoerceScalarValue(int,ref):long
        -235 : System.Private.CoreLib.dasm - __Error:WinIOError(int,ref)
        -213 : System.Private.CoreLib.dasm - CLRIPropertyValueImpl:CoerceScalarValue(int,ref):ubyte

371 total methods with size differences.

jamesqo · 2016-09-11T23:14:44Z

+            return GetResourceStringFormatted(key, new object[] { val0, val1, val2, val3, val4, val5 });
        }

+        internal static String GetResourceStrings(string key, params object[] values)


GetResourceString

sigh... was so it didn't get caught in my renaming,

Fixed, good spot.

benaadams · 2016-09-11T23:25:36Z

Need to add [System.Security.SecuritySafeCritical] to all the methods?

jkotas · 2016-09-12T01:27:57Z

Need to add [System.Security.SecuritySafeCritical] to all the methods?

It should not be needed.

benaadams · 2016-09-12T03:45:36Z

@dotnet-bot test Linux ARM Emulator Cross Debug Build

jkotas · 2016-09-12T20:39:19Z

Thank you!

Reduce code bloat around formatted GetResourceString calls

b28dc79

dnfclas added the cla-already-signed label Sep 11, 2016

jamesqo reviewed Sep 11, 2016
View reviewed changes

benaadams force-pushed the enviro branch from cd49917 to ab9d2e1 Compare September 11, 2016 23:06

jamesqo reviewed Sep 11, 2016
View reviewed changes

Fix Environment no-inlining for mscorlib

85f2216

benaadams force-pushed the enviro branch from ab9d2e1 to 85f2216 Compare September 11, 2016 23:22

jkotas merged commit 762833d into dotnet:master Sep 12, 2016

benaadams deleted the enviro branch October 13, 2016 08:49

Conversation

benaadams commented Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khellang commented Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Sep 11, 2016

Uh oh!

jamesqo commented Sep 11, 2016

Uh oh!

benaadams commented Sep 11, 2016

Uh oh!

benaadams commented Sep 11, 2016

Uh oh!

jamesqo Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams Sep 11, 2016

Choose a reason for hiding this comment

Uh oh!

benaadams commented Sep 11, 2016

Uh oh!

jamesqo Sep 11, 2016

Choose a reason for hiding this comment

Uh oh!

benaadams Sep 11, 2016

Choose a reason for hiding this comment

Uh oh!

benaadams commented Sep 11, 2016

Uh oh!

jamesqo Sep 11, 2016

Choose a reason for hiding this comment

Uh oh!

benaadams Sep 11, 2016

Choose a reason for hiding this comment

Uh oh!

benaadams Sep 11, 2016

Choose a reason for hiding this comment

Uh oh!

benaadams commented Sep 11, 2016

Uh oh!

jkotas commented Sep 12, 2016

Uh oh!

benaadams commented Sep 12, 2016

Uh oh!

jkotas commented Sep 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benaadams commented Sep 11, 2016 •

edited

Loading

benaadams commented Sep 11, 2016 •

edited

Loading

khellang commented Sep 11, 2016 •

edited

Loading

jamesqo Sep 11, 2016 •

edited

Loading