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

Jump to bottom

[RyuJIT] Import String.Empty as CNS_STR #44847

Closed
wants to merge 4 commits into from
Closed

[RyuJIT] Import String.Empty as CNS_STR #44847

wants to merge 4 commits into from

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Nov 18, 2020

Fixes #42694

Jit-diff (-f -pmi):

Total bytes of base: 49550624
Total bytes of diff: 49553223
Total bytes of delta: 2599 (0.01% of base)
    diff is a regression.

Top file regressions (bytes):
        3057 : System.Private.Xml.dasm (0.09% of base)
         282 : System.Configuration.ConfigurationManager.dasm (0.08% of base)
          80 : System.Net.Primitives.dasm (0.13% of base)
          77 : System.Data.Odbc.dasm (0.04% of base)
          47 : System.Transactions.Local.dasm (0.04% of base)
          41 : System.IO.Packaging.dasm (0.05% of base)
          38 : System.Management.dasm (0.01% of base)
          25 : System.Data.OleDb.dasm (0.01% of base)
          15 : System.Data.Common.dasm (0.00% of base)
          11 : xunit.console.dasm (0.01% of base)
           7 : System.Private.Uri.dasm (0.01% of base)
           4 : System.ServiceModel.Syndication.dasm (0.00% of base)
           3 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (0.00% of base)

Top file improvements (bytes):
        -381 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.01% of base)
        -150 : Microsoft.CodeAnalysis.CSharp.dasm (-0.00% of base)
        -125 : System.DirectoryServices.dasm (-0.03% of base)
        -123 : System.DirectoryServices.AccountManagement.dasm (-0.03% of base)
         -83 : System.Diagnostics.EventLog.dasm (-0.09% of base)
         -53 : System.Private.CoreLib.dasm (-0.00% of base)
         -49 : CommandLine.dasm (-0.01% of base)
         -29 : System.Security.Cryptography.Xml.dasm (-0.02% of base)
         -26 : System.Private.Xml.Linq.dasm (-0.02% of base)
         -20 : Microsoft.CodeAnalysis.dasm (-0.00% of base)
         -13 : Microsoft.Extensions.FileSystemGlobbing.dasm (-0.05% of base)
         -11 : Microsoft.Extensions.Configuration.Xml.dasm (-0.20% of base)
         -10 : Microsoft.Extensions.DependencyModel.dasm (-0.02% of base)
          -5 : Microsoft.Extensions.Configuration.EnvironmentVariables.dasm (-0.20% of base)
          -3 : System.Diagnostics.Process.dasm (-0.00% of base)
          -3 : System.Net.Requests.dasm (-0.00% of base)
          -2 : System.Private.DataContractSerialization.dasm (-0.00% of base)
          -1 : System.Net.HttpListener.dasm (-0.00% of base)
          -1 : System.Security.Claims.dasm (-0.00% of base)

32 total files with Code Size differences (19 improved, 13 regressed), 236 unchanged.

Top method regressions (bytes):
        1865 (19.19% of base) : System.Private.Xml.dasm - SchemaNames:.ctor(XmlNameTable):this
         125 (416.67% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:.ctor(Stream,XmlNameTable):this
         115 (45.63% of base) : System.Private.Xml.dasm - XmlDocument:Load(Stream):this
         105 (101.94% of base) : System.Private.Xml.dasm - XmlTextReader:.ctor(Stream,XmlNameTable):this
         105 (350.00% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:.ctor(TextReader,XmlNameTable):this
         100 (128.21% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:.ctor(Stream):this
          98 (23.96% of base) : System.Configuration.ConfigurationManager.dasm - ConfigXmlDocument:LoadSingleElement(String,XmlTextReader):this
          98 (73.68% of base) : System.Private.Xml.dasm - XmlTextReader:.ctor(Stream):this
          98 (31.41% of base) : System.Private.Xml.dasm - XPathDocument:.ctor(Stream):this
          89 (35.32% of base) : System.Private.Xml.dasm - XmlDocument:Load(TextReader):this
          89 (30.80% of base) : System.Private.Xml.dasm - XmlDocument:LoadXml(String):this
          85 (82.52% of base) : System.Private.Xml.dasm - XmlTextReader:.ctor(TextReader,XmlNameTable):this
          80 ( 9.58% of base) : System.Configuration.ConfigurationManager.dasm - BaseConfigurationRecord:FindSectionRecursive(ref,int,XmlUtil,byref):ConfigXmlReader:this
          80 (102.56% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:.ctor(TextReader):this
          78 (58.65% of base) : System.Private.Xml.dasm - XmlTextReader:.ctor(TextReader):this
          78 (25.00% of base) : System.Private.Xml.dasm - XPathDocument:.ctor(TextReader):this
          77 (427.78% of base) : System.Private.Xml.dasm - XmlAttributeOverrides:get_Item(Type):XmlAttributes:this
          73 (58.87% of base) : System.Private.Xml.dasm - XmlReflectionImporter:GetAttributes(Type,bool):XmlAttributes:this
          62 ( 3.04% of base) : System.Private.Xml.dasm - XsltLoader:LoadDecimalFormat(NsDecl):this
          61 ( 8.47% of base) : System.Private.Xml.dasm - DtdParser:InitializeFreeFloatingDtd(String,String,String,String,String,IDtdParserAdapter):this

Top method improvements (bytes):
         -68 (-1.95% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:ParseAttributeValueSlow(int,ushort,NodeData):this
         -55 (-7.49% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Scanner:ScanInterpolatedStringPunctuation():SyntaxToken:this
         -52 (-4.11% of base) : System.Diagnostics.EventLog.dasm - EventLog:Delete(String,String)
         -52 (-2.38% of base) : System.Private.Xml.dasm - DtdParser:ParseElementMixedContent(ParticleContentValidator,int):this
         -44 (-4.45% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LambdaSymbol:MakeParameters(CSharpCompilation,UnboundLambda,ImmutableArray`1):ImmutableArray`1:this
         -44 (-2.23% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:BindXmlElementWithoutAddingNamespaces(XmlNodeSyntax,XmlNodeSyntax,byref,ArrayBuilder`1,ArrayBuilder`1,SyntaxList`1,XmlElementRootInfo,DiagnosticBag):BoundExpression:this
         -44 (-2.43% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Scanner:ScanXmlElementInXmlDoc(int):SyntaxToken:this
         -44 (-1.86% of base) : System.DirectoryServices.AccountManagement.dasm - PrincipalContext:DoLDAPDirectoryInitNoContainer():this
         -43 (-5.43% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LocalRewriter:RewriteStringConcatenation(CSharpSyntaxNode,int,BoundExpression,BoundExpression,TypeSymbol):BoundExpression:this
         -43 (-8.13% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - LocalRewriter:VisitInterpolatedStringExpression(BoundInterpolatedStringExpression):BoundNode:this
         -39 (-1.23% of base) : System.Private.Xml.dasm - <ParseElementMixedContentAsync>d__166:MoveNext():this
         -38 (-2.10% of base) : System.Configuration.ConfigurationManager.dasm - ConfigurationElementCollection:BaseAdd(ConfigurationElement,bool,bool):this
         -38 (-1.53% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:ReadAsync():Task`1:this
         -36 (-1.90% of base) : System.Configuration.ConfigurationManager.dasm - ConfigurationElementCollection:BaseAdd(int,ConfigurationElement,bool):this
         -33 (-3.58% of base) : System.Private.Xml.dasm - XmlSqlBinaryReader:ReadContentAsDecimal():Decimal:this
         -33 (-3.78% of base) : System.Private.Xml.dasm - XmlSqlBinaryReader:ReadContentAsLong():long:this
         -29 (-2.13% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Scanner:ScanXmlPIDataInXmlDoc(int):SyntaxToken:this
         -28 (-50.00% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Scanner:MakeEndOfInterpolatedStringToken():SyntaxToken:this
         -28 (-6.33% of base) : System.Private.Xml.dasm - Datatype_QName:TryParseValue(String,XmlNameTable,IXmlNamespaceResolver,byref):Exception:this
         -28 (-6.33% of base) : System.Private.Xml.dasm - Datatype_NOTATION:TryParseValue(String,XmlNameTable,IXmlNamespaceResolver,byref):Exception:this

Top method regressions (percentages):
          77 (427.78% of base) : System.Private.Xml.dasm - XmlAttributeOverrides:get_Item(Type):XmlAttributes:this
         125 (416.67% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:.ctor(Stream,XmlNameTable):this
         105 (350.00% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:.ctor(TextReader,XmlNameTable):this
          49 (272.22% of base) : System.Private.Xml.dasm - XmlQualifiedName:.ctor(String):this
          41 (195.24% of base) : System.Private.Xml.dasm - XmlQualifiedName:.ctor():this
         100 (128.21% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:.ctor(Stream):this
          80 (102.56% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:.ctor(TextReader):this
         105 (101.94% of base) : System.Private.Xml.dasm - XmlTextReader:.ctor(Stream,XmlNameTable):this
          85 (82.52% of base) : System.Private.Xml.dasm - XmlTextReader:.ctor(TextReader,XmlNameTable):this
          98 (73.68% of base) : System.Private.Xml.dasm - XmlTextReader:.ctor(Stream):this
          73 (58.87% of base) : System.Private.Xml.dasm - XmlReflectionImporter:GetAttributes(Type,bool):XmlAttributes:this
          78 (58.65% of base) : System.Private.Xml.dasm - XmlTextReader:.ctor(TextReader):this
          47 (49.47% of base) : System.Net.Primitives.dasm - SystemNetworkCredential:.ctor():this
          47 (49.47% of base) : System.Net.Primitives.dasm - NetworkCredential:.ctor():this
         115 (45.63% of base) : System.Private.Xml.dasm - XmlDocument:Load(Stream):this
          89 (35.32% of base) : System.Private.Xml.dasm - XmlDocument:Load(TextReader):this
          98 (31.41% of base) : System.Private.Xml.dasm - XPathDocument:.ctor(Stream):this
          89 (30.80% of base) : System.Private.Xml.dasm - XmlDocument:LoadXml(String):this
          78 (25.00% of base) : System.Private.Xml.dasm - XPathDocument:.ctor(TextReader):this
          33 (25.00% of base) : System.Private.Xml.dasm - SchemaInfo:System.Xml.IDtdInfo.LookupEntity(String):IDtdEntityInfo:this

Top method improvements (percentages):
         -28 (-50.00% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Scanner:MakeEndOfInterpolatedStringToken():SyntaxToken:this
         -25 (-23.81% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Scanner:MakeEofToken(SyntaxList`1):SyntaxToken
         -13 (-9.49% of base) : Microsoft.CodeAnalysis.CSharp.dasm - UnboundArgumentErrorTypeSymbol:.cctor()
         -17 (-9.09% of base) : System.Private.Xml.dasm - XmlReader:ReadElementContentAsInt():int:this
         -17 (-8.99% of base) : System.Private.Xml.dasm - XmlReader:ReadElementContentAsLong():long:this
         -17 (-8.50% of base) : System.Private.Xml.dasm - XmlReader:ReadElementContentAsDecimal():Decimal:this
         -13 (-8.50% of base) : Microsoft.Extensions.FileSystemGlobbing.dasm - WildcardPathSegment:.cctor()
         -43 (-8.13% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - LocalRewriter:VisitInterpolatedStringExpression(BoundInterpolatedStringExpression):BoundNode:this
          -8 (-7.77% of base) : System.Data.OleDb.dasm - OleDbConnectionString:get_DataSource():String:this
          -8 (-7.77% of base) : System.Data.OleDb.dasm - OleDbConnectionString:get_InitialCatalog():String:this
         -55 (-7.49% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Scanner:ScanInterpolatedStringPunctuation():SyntaxToken:this
          -5 (-7.35% of base) : Microsoft.Extensions.Configuration.EnvironmentVariables.dasm - EnvironmentVariablesConfigurationProvider:.ctor():this
          -7 (-7.29% of base) : System.Diagnostics.EventLog.dasm - EventLog:.ctor(String,String):this
          -7 (-6.36% of base) : System.Net.Primitives.dasm - NetworkCredential:.ctor(String,String):this
          -7 (-6.36% of base) : System.Net.Primitives.dasm - NetworkCredential:.ctor(String,SecureString):this
         -28 (-6.33% of base) : System.Private.Xml.dasm - Datatype_QName:TryParseValue(String,XmlNameTable,IXmlNamespaceResolver,byref):Exception:this
         -28 (-6.33% of base) : System.Private.Xml.dasm - Datatype_NOTATION:TryParseValue(String,XmlNameTable,IXmlNamespaceResolver,byref):Exception:this
          -5 (-5.88% of base) : System.Private.Xml.dasm - XmlReader:Create(TextReader,XmlReaderSettings):XmlReader
         -16 (-5.71% of base) : CommandLine.dasm - MultilineTextAttribute:get_Value():String:this
         -23 (-5.68% of base) : System.Private.DataContractSerialization.dasm - XmlDictionaryReader:ReadElementContentAsDateTime():DateTime:this

337 total methods with Code Size differences (225 improved, 112 regressed), 258500 unchanged.

Diff is a regression because GT_CNS_STR (const) instead of GT_IND leads to bigger benefit multipliers.

I decided to use a fake SconCPX (0) instead of a real one for "" because as far as I understand I'll have to add a new jit-API method to get one and I'm not sure it worth it.

I'm planning to try to optimize x == string.Empty to x?.Length == 0 and it's better to have GT_CNS_STR there.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 18, 2020
@EgorBo
Copy link
Member Author

EgorBo commented Nov 18, 2020

per v3.1 codegen

Would optimizing to x == "" be better?

X in your case is the fastest one because Y and Z just call String.Equals 🙂

@runfoapp runfoapp bot mentioned this pull request Nov 18, 2020
Closed
@ViktorHofer
Copy link
Member

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

AndyAyersMS
AndyAyersMS reviewed Dec 21, 2020
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

I wonder if we should instead cache the value returned by emptyStringLiteral on the jit side and recognize that in IsStringEmptyField()?

Say add a gtNewEmptyStringLiteral that sets the cache (or confirms the value if already set).

Then (I think) the changes in morph wouldn't be needed.

@EgorBo
Copy link
Member Author

EgorBo commented Dec 21, 2020 •
edited
Loading

I wonder if we should instead cache the value returned by emptyStringLiteral on the jit side and recognize that in IsStringEmptyField()?

Say add a gtNewEmptyStringLiteral that sets the cache (or confirms the value if already set).

Then (I think) the changes in morph wouldn't be needed.

@AndyAyersMS Can you please re-review? I am now saving SconCPX to a global variable before constructStringLiteral (if it's from corlib module)

AndyAyersMS
AndyAyersMS reviewed Dec 21, 2020
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Would prefer not to have important mutable static state, so I'd suggest caching on the compiler instance.

@runfoapp runfoapp bot mentioned this pull request Dec 22, 2020
Closed
@EgorBo
Copy link
Member Author

EgorBo commented Dec 22, 2020 •
edited
Loading

Would prefer not to have important mutable static state, so I'd suggest caching on the compiler instance.

Hm.. isn't compiler instance re-created (inited) for each method so caching on the instance level won't make any sense?

@AndyAyersMS
Copy link
Member

Jit hosts may not always return the same value over the lifetime of a process, so caching the result in a static on the jit side won't work. But the value should be stable for a particular jit request.

@AndyAyersMS
Copy link
Member

Jit hosts may not always return the same value over the lifetime of a process

In particular, that may explain your SPMI and Crossgen2 failures...

@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Feb 8, 2021
@EgorBo
Copy link
Member Author

EgorBo commented Feb 19, 2021

Will revise later, closing to keep amount of active PRs smaller.
Caching SconCPX in a compiler instance makes the optimization useful only if we hit "" and string.Eqauls as part of the same jit request (in the same method?)

@EgorBo EgorBo closed this Feb 19, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndyAyersMS AndyAyersMS AndyAyersMS left review comments

Assignees

@EgorBo EgorBo

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

RyuJIT: String.Empty vs ""
5 participants
@EgorBo @ViktorHofer @AndyAyersMS @Dotnet-GitSync-Bot @JulieLeeMSFT