Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Use const internally in corelib for Environment.NewLine #27013

Merged
merged 1 commit into from
Oct 3, 2019

Conversation

stephentoub
Copy link
Member

Anywhere Environment.NewLine was being used internal to corelib, instead changes it to use an internal Environment.NewLineConst, which is just a const string. The main benefit of that is places where it's then concatenated with another const allows the C# compiler to do the concat at compile time rather than at run time.

Where I was already touching the code, fixed a few places where additional strings were being created unnecessarily, e.g. s += a; s += b; will do two string.Concat calls each with two arguments, whereas s += a + b; will do just one string.Concat call with three arguments.

@EgorBo
Copy link
Member

EgorBo commented Oct 3, 2019

Shouldn't Environment.NewLine be an intrinsic instead? Like BitConverter.IsLittleEndian or string.Empty?

UPD: ah, I guess JIT is not able to concat const strings yet (but would be nice to be able to fold CNS_STR nodes, I think it's not difficult to implement: #26000 (comment))

@stephentoub
Copy link
Member Author

I guess JIT is not able to concat const strings yet (but would be nice to be able to fold CNS_STR nodes, I think it's not difficult to implement

It of course could be done, but it'd be more than just handling constant strings... the JIT would need to unwind decisions the C# compiler makes about what overloads of string.Concat to use.

@EgorBo
Copy link
Member

EgorBo commented Oct 3, 2019

I guess JIT is not able to concat const strings yet (but would be nice to be able to fold CNS_STR nodes, I think it's not difficult to implement

It of course could be done, but it'd be more than just handling constant strings... the JIT would need to unwind decisions the C# compiler makes about what overloads of string.Concat to use.

I mean at least simple cases, e.g. (from your changes):

text.Append(Environment.NewLine + InnerExceptionPrefix);

So JIT in runtime should replace Environment.NewLine with CNS_STR (const string) because its value never changes. Environment.NewLine + InnerExceptionPrefix is:

\--*  CALL      ref    String.Concat
   +--*  CALL      ref    Environment.get_NewLine
   \--*  CNS_STR   ref   <string constant>

then is converted into:

\--*  CALL      ref    String.Concat
   +--*  CNS_STR   ref   <string constant>   ;; it's now a const
   \--*  CNS_STR   ref   <string constant>

then JIT should fold it into just

\--*  CNS_STR   ref   <string constant>

@stephentoub stephentoub merged commit 073ad7e into dotnet:master Oct 3, 2019
@stephentoub stephentoub deleted the newline branch October 3, 2019 20:40
SrivastavaAnubhav pushed a commit to SrivastavaAnubhav/coreclr that referenced this pull request Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants