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

Use C# compiler's static data support in Encoding.Preamble #20768

Merged
merged 2 commits into from Nov 3, 2018

Conversation

Projects
None yet
3 participants
@stephentoub
Copy link
Member

commented Nov 2, 2018

Also avoid Array.Empty and just use default span for an empty preamble.

Example IL (for UTF8Encoding.PreambleSpan):

  IL_0000:  ldsflda    valuetype '<PrivateImplementationDetails>'/'__StaticArrayInitTypeSize=3' '<PrivateImplementationDetails>'::'57218C316B6921E2CD61027A2387EDC31A2D9471'
  IL_0005:  ldc.i4.3
  IL_0006:  newobj     instance void valuetype System.ReadOnlySpan`1<uint8>::.ctor(void*, int32)
  IL_000b:  ret

cc: @jkotas, @tarekgh

Use C# compiler's static data support in Encoding.Preamble
Also avoid Array.Empty and just use default span for an empty preamble.
@@ -39,8 +39,8 @@ public sealed class UTF32Encoding : Encoding
internal static readonly UTF32Encoding s_default = new UTF32Encoding(bigEndian: false, byteOrderMark: true);
internal static readonly UTF32Encoding s_bigEndianDefault = new UTF32Encoding(bigEndian: true, byteOrderMark: true);

private static readonly byte[] s_bigEndianPreamble = new byte[4] { 0x00, 0x00, 0xFE, 0xFF };
private static readonly byte[] s_littleEndianPreamble = new byte[4] { 0xFF, 0xFE, 0x00, 0x00 };
private static ReadOnlySpan<byte> BigEndianPreamble => new byte[4] { 0x00, 0x00, 0xFE, 0xFF }; // uses C# compiler's optimization for static byte[] data

This comment has been minimized.

Copy link
@jkotas

jkotas Nov 2, 2018

Member

Can this just be inline in the Preamble property, similar to how GetPreamble() method is done? No need to define trivial properties for this.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Nov 2, 2018

Author Member

Sure.

@jkotas

jkotas approved these changes Nov 2, 2018

Copy link
Member

left a comment

Nice

@ahsonkhan

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

I had gone through and made changes across the board within the coreclr repo some time ago but never submitted it. Is it worth pursuing and submitting a PR?

2832a0d

I had similar changes for corefx in some branch that I need to find on my old machine. Let me know if I should dig it up.

For example: ahsonkhan/corefx@21c3abb

@jkotas

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

Is it worth pursuing and submitting a PR?

Sounds good to me. (BTW: The Hungarian p prefix should be deleted when you are turning it into property, not upper cased.)

@stephentoub

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2018

Is it worth pursuing and submitting a PR?

This particular case is one where the consumer was already using the array as a span, so there was very little downside (though I did measure just to be sure, and microbenchmarks on, e.g. Encoding.UTF8.Preamble.Length, saw nice bumps). The other cases I found in corelib, though, which show up in your changes, too, all had a consumer expecting a byte[], which meant there could in theory be a greater chance of regression. Since there is a downside to the churn, and IMO a readability hit (it really looks like these call sites allocate), I think it's only worth doing for cases where you believe there will be measurable throughout or startup wins, or some other measurable benefit, and can then validate that. And in most of these cases, I think that's probably unlikely. But if you want to prove otherwise, great :)

@stephentoub

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2018

@dotnet-bot test Windows_NT x86 Release Innerloop Build and Test please
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked Innerloop Build and Test please

@stephentoub

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2018

ps Just saw Jan's response. If he thinks it's worthwhile, I defer to him :)

@jkotas

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

This should be pretty much always worthwhile, in particular for large data blobs like the ones found in globalization. We should spot check the code generated by the JIT for this in several places to make sure that it works as expected and it is not hitting JIT optimization bug as it is often case with new constructs.

@stephentoub stephentoub merged commit faa4c87 into dotnet:master Nov 3, 2018

31 checks passed

CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
Linux-musl x64 Debug Build Build finished.
Details
OSX10.12 x64 Checked Innerloop Build and Test Build finished.
Details
Tizen armel Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked crossgen_comparison Build and Test Build finished.
Details
Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Release crossgen_comparison Build and Test Build finished.
Details
Ubuntu x64 Checked CoreFX Tests Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Ubuntu16.04 arm64 Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu16.04 arm64 Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
WIP Ready for review
Details
Windows_NT arm Cross Debug Innerloop Build Build finished.
Details
Windows_NT arm64 Cross Debug Innerloop Build Build finished.
Details
Windows_NT x64 Checked CoreFX Tests Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release CoreFX Tests Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x86 Release Innerloop Build and Test Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
license/cla All CLA requirements met.
Details

@stephentoub stephentoub deleted the stephentoub:encodingspans branch Nov 3, 2018

A-And added a commit to A-And/coreclr that referenced this pull request Nov 20, 2018

Use C# compiler's static data support in Encoding.Preamble (dotnet#20768
)

* Use C# compiler's static data support in Encoding.Preamble

Also avoid Array.Empty and just use default span for an empty preamble.

* Address PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.