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.

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

JIT: Optimize "constant_string".Length #26000

Open
wants to merge 8 commits into
base: master
from

Conversation

@EgorBo
Copy link
Contributor

commented Aug 5, 2019

Replace "Hello".Length with just 5. Or

. See #3633

static int Test()
{
   return "Hello".Length;
}

Before:

; Method ConsoleApp177.Program:Test():int
G_M38358_IG01:
G_M38358_IG02:
       mov      rax, 0xD1FFAB1E
       mov      rax, gword ptr [rax]
       mov      eax, dword ptr [rax+8]
G_M38358_IG03:
       ret      
; Total bytes of code: 17

After:

; Method ConsoleApp177.Program:Test():int
G_M38358_IG01:
G_M38358_IG02:
       mov      eax, 5
G_M38358_IG03:
       ret      
; Total bytes of code: 6

Morph:

    *  RETURN    int   
    \--*  ARR_LENGTH int   
        \--*  CNS_STR   ref   <string constant>

    ↓ ↓

    *  RETURN    int   
    \--*  CNS_INT   int    5

Also, according to jit-diff (see below) it improves things like:

System.IO.Path:ChangeExtension("..");
System.IO.Path:GetFullPath("..");
System.Byte:ToString("X2");
System.Convert:ToBase64String("..");
System.DateTimeParse:ParseByFormat("..");
System.Enum:ValueToHexString("..");
...
@EgorBo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Jit-diff (-f --pmi):

PMI Diffs for System.Private.CoreLib.dll, framework assemblies for  default jit
Summary:
(Lower is better)
Total bytes of diff: -30626 (-0.07% of base)
    diff is an improvement.
Top file regressions by size (bytes):
          12 : NuGet.Versioning.dasm (0.04% of base)
          10 : NuGet.Frameworks.dasm (0.01% of base)
           6 : System.Private.Xml.Linq.dasm (0.00% of base)
           4 : System.Diagnostics.TextWriterTraceListener.dasm (0.03% of base)
Top file improvements by size (bytes):
       -6180 : System.Private.CoreLib.dasm (-0.11% of base)
       -4628 : System.Private.Xml.dasm (-0.13% of base)
       -4375 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.14% of base)
       -3470 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.06% of base)
       -1985 : System.Net.HttpListener.dasm (-0.82% of base)
43 total files with size differences (39 improved, 4 regressed), 86 unchanged.
Top method regressions by size (bytes):
          70 ( 3.78% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.ActivityComputer:ResolveWellKnownSymbols():this
          60 ( 4.44% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.Binder:BindIntegralMinValConstants(ref,ref,ref):ref:this
          20 ( 2.40% of base) : System.Private.Xml.dasm - System.Xml.HtmlEncodedRawTextWriter:WriteDocType(ref,ref,ref,ref):this
          16 ( 2.02% of base) : System.Private.Xml.dasm - System.Xml.HtmlUtf8RawTextWriter:WriteDocType(ref,ref,ref,ref):this
          16 ( 2.41% of base) : System.Private.Xml.dasm - System.Xml.XmlEncodedRawTextWriter:WriteDocType(ref,ref,ref,ref):this
Top method improvements by size (bytes):
        -829 (-29.20% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.Parsers.Kernel.MemoryProcessMemInfoTraceData:ToXml(ref):ref:this
        -787 (-29.16% of base) : System.Private.Xml.dasm - System.Xml.XmlConvert:EncodeName(ref,bool,bool):ref
        -780 (-39.14% of base) : System.Private.CoreLib.dasm - System.Enum:ValueToHexString(ref):ref
        -776 (-16.32% of base) : System.Diagnostics.Process.dasm - System.Diagnostics.NtProcessManager:GetProcessInfos(ref,int,int,struct):ref
        -556 (-34.79% of base) : System.Net.Http.dasm - System.Net.Http.HttpWindowsProxy:ParseProxyConfigPart(struct,byref,byref)
Top method regressions by size (percentage):
          60 ( 4.44% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.Binder:BindIntegralMinValConstants(ref,ref,ref):ref:this
          70 ( 3.78% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.ActivityComputer:ResolveWellKnownSymbols():this
          16 ( 2.41% of base) : System.Private.Xml.dasm - System.Xml.XmlEncodedRawTextWriter:WriteDocType(ref,ref,ref,ref):this
          20 ( 2.40% of base) : System.Private.Xml.dasm - System.Xml.HtmlEncodedRawTextWriter:WriteDocType(ref,ref,ref,ref):this
          16 ( 2.02% of base) : System.Private.Xml.dasm - System.Xml.HtmlUtf8RawTextWriter:WriteDocType(ref,ref,ref,ref):this
Top method improvements by size (percentage):
         -14 (-56.00% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.Cci.PeWriter:get_SizeOfNameTable():int
         -14 (-56.00% of base) : System.Reflection.Metadata.dasm - System.Reflection.PortableExecutable.ManagedTextSection:get_SizeOfNameTable():int
         -85 (-49.71% of base) : System.Private.CoreLib.dasm - System.Char8:ToString():ref:this
         -86 (-47.51% of base) : System.Diagnostics.FileVersionInfo.dasm - System.Diagnostics.FileVersionInfo:ConvertTo8DigitHex(int):ref
         -92 (-45.10% of base) : System.Private.Xml.dasm - System.Xml.XmlTextEncoder:WriteCharEntityImpl(ushort):this
483 total methods with size differences (451 improved, 32 regressed), 249702 unchanged.
2 files had text diffs but not size diffs.
System.Resources.Writer.dasm had 135 diffs
NuGet.Protocol.Core.v3.dasm had 22 diffs
Completed analysis in 21.82s
@dzmitry-lahoda

This comment has been minimized.

Copy link

commented Aug 5, 2019

may be patch roslyn and fsharp?

GenTree* strCon = op1->AsOp()->gtGetOp1();
if (strCon->OperIs(GT_CNS_INT) && strCon->IsIconHandle(GTF_ICON_STR_HDL))
{
CORINFO_String* asString = *reinterpret_cast<CORINFO_String**>(strCon->AsIntCon()->IconValue());

This comment has been minimized.

Copy link
@mikedn

mikedn Aug 5, 2019

Contributor

Hmm, I remember looking into this in the past and thinking that the JIT-VM interface would need to be extended to obtain this information. But I don't remember why, perhaps it wasn't clear if it's appropriate to use CORINFO_String like this.

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 5, 2019

Member

It is not ok to use CORINFO_String like this. The string is not pinned and the GC can move it while the JIT is looking at the Length field. It would result into GC hole / intermittent silent bad codegen.

This comment has been minimized.

Copy link
@EgorBo

EgorBo Aug 5, 2019

Author Contributor

Oh I suspected that, I hope I won't have to modify vm API for it, looking at the importation now to find out if I can save string length to GenTreeStrCon (new field)

This comment has been minimized.

Copy link
@jkotas

jkotas Aug 5, 2019

Member

I do not think you would be able to do this without modifying JIT/EE interface

This comment has been minimized.

Copy link
@EgorBo

EgorBo Aug 5, 2019

Author Contributor

@jkotas ah I see, I guess it should be something similar to isValidStringRef

This comment has been minimized.

Copy link
@EgorBo

EgorBo Aug 5, 2019

Author Contributor

@jkotas I tried to copy-paste isValidStringRef method and introduced getStringLength should work now I hope...

This comment has been minimized.

Copy link
@mikedn

mikedn Aug 5, 2019

Contributor

If a new function is added to the JIT-VM interface would it make sense to also return the string characters somehow? Perhaps in the future we could add some other optimizations around constant strings such as generating special code for a.Equals("ABCD").

src/jit/morph.cpp Outdated Show resolved Hide resolved
@@ -12829,6 +12829,26 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
break;
#endif

case GT_ARR_LENGTH:
// Morph "constant_string".Length to CNS_INT(15)
if (op1->OperIs(GT_IND) && fgGlobalMorph)

This comment has been minimized.

Copy link
@mikedn

mikedn Aug 5, 2019

Contributor

Does this really have to wait until morph? Is there any way to do this during importation?

This comment has been minimized.

Copy link
@mikedn

mikedn Aug 5, 2019

Contributor

I wonder if this is enough to catch all cases. Does this work if the string is a static readonly field? Does it work if you have conditional code like (c ? "abc" : "def").Length? The later, while probably not common, would likely require updating VN to work.

This comment has been minimized.

Copy link
@EgorBo

EgorBo Aug 5, 2019

Author Contributor

@mikedn if I do it in the importation phase will it handle e.g. stringBuilder.Append("."); ? (Append will be inlined and it checks for Length < 2 inside afair for fast insert -- so with the current change it's improved).

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Aug 5, 2019

Member

The inliner will directly substitute constants like this, so checking in the importer should get most cases.

@mikedn

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Where do code size regressions come from? Seems odd for this to introduce any regressions, unless we have cases where the characters of the constant string are accessed via the indexer and this breaks range check elimination. Though I don't see how, if anyhting it should be the other way around.

@mikedn

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

may be patch roslyn and fsharp?

What do you mean by that?

@dzmitry-lahoda

This comment has been minimized.

Copy link

commented Aug 5, 2019

What do you mean by that?
why should JIT do what compilers can? put number instead of call may be done by roslyn.

@mikedn

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

why JIT should do what compilers can? put 5 instead of call may be done by roslyn.

In general managed compilers do not make optimizations for various reasons. The fact that there is more than one managed compiler is one of them. Another reason is that some optimizations may require access to more code than the managed compiler has access to (e.g. what happens if the constant string is returned by a method in another assembly). IL optimizations may also affect various IL tools out there (e.g. some IL tool may care about knowing when the length of a particular string is obtained).

EgorBo added some commits Aug 5, 2019

src/vm/jitinterface.cpp Outdated Show resolved Hide resolved
@AndyAyersMS

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

I would move the transformation into gtFoldExprConst and move the check into the importer.

Also you'll need a new jit GUID.

@mikedn

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

I would move the transformation into gtFoldExprConst and move the check into the importer.

Right, perhaps with some care that would also take care of the unnecessary null check from #22511

@EgorBo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@AndyAyersMS @mikedn @jkotas

Where do code size regressions come from?

An example of the regression:
was: cmp r14d, dword ptr [r15+8] now: cmp r14d, 16 (same with mov)
Full jit-analyze report https://gist.github.com/EgorBo/0784f2d475b371a2005c928ca8305d17

Does this work if the string is a static readonly field? Does it work if you have conditional code like (c ? "abc" : "def").Length

Is not handled yet but the static readonly case sounds like a good candidate for TODO

The inliner will directly substitute constants like this, so checking in the importer should get most cases.

So will it handle this case: sharplab.io ? Current implementation handles it.

I would move the transformation into gtFoldExprConst and move the check into the importer.

Um.. GT_ARR_LENGTH visits gtFoldExpr after the place it's currently located in my PR, furthermore someone changes GT_CNS_STR to GT_IND+GT_CNS_INT before it visits gtFoldExpr. Or did you mean to handle GT_CNS_STR? Will I be able to remove its parent there then?

    *  RETURN    int   
    \--*  ARR_LENGTH int   
        \--*  CNS_STR   ref   <string constant>

Also you'll need a new jit GUID.

Could you please show me where it's located?

Also, I was wrong about StringBuilder.Append("const str") is not inlined (obviously it's too big) but it's quite popular to use it with literals so I suggest to make a special version which will be inline friendly for literals (e.g. hide the slow paths under a call, see impl).

@jkotas

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

I suggest to make a special version which will be inline friendly for literals

It does not sound like a good idea to me. It would just make the code bigger and slower.

@AndyAyersMS

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

The JIT GUID is found in corinfo.h.

Once you change it, running jit-diffs will become more complicated as the base/diff jits will no longer be plugin compatible. So maybe change that as a last step.

You should invoke the suitably updated gtFoldExpr in the CEE_LDLEN handling clause in the importer, under the optimization enabled clause. I would leave it general in the importer (just always pass the new tree as an arg, don't check up front for constness there) and we can have the implementation check for the various special cases.

@AndyAyersMS

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

I wonder if we can lose bounds check eliminations from this, eg a loop like

string s = "a string";
for (int i = 0; i < s.Length; i++)
{
        ... s[i]
}

Can you double-check that the internally generated string length access also gets optimized?

@tannergooding

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

An example of the regression:
was: cmp r14d, dword ptr [r15+8] now: cmp r14d, 16 (same with mov)

@EgorBo, why is that a regression? Both should be 4 bytes as there are cmp reg32, imm8 encodings (it should change, if I'm not mistaken, from 45 3b 77 08 to 41 83 fe 10).

@ArtBlnd

This comment has been minimized.

Copy link

commented Aug 9, 2019

@tannergooding Maybe because of constant transmissions? just like @AndyAyersMS 's example. It will make sense some of regressions causes from inline small codes into large code. and most of codes will be improved from other optimization side effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.