Better lazy initialization for Encoding static properties

[jamesqo]

Previously the Encoding properties were not being inlined and made use of volatile fields, which are particularly costly on ARM (I think). This changes them to use nested static classes instead, which are a better way to perform lazy initialization (#1193) since we bypass checks if the cctor has already been run.

Partially addresses #6759

Unfortunately this only made a small impact (< 10%, maybe 6-8%) for cases like

Encoding.UTF8.GetString(Encoding.UTF8.GetBytes(""));

since Encoding is basically a landmine of virtual method calls. However the difference is still there, and the diff is mainly red.

cc: @jkotas @omariom

[jkotas]

It is wasteful to create new holder types for these - even trivial type costs 100's bytes at runtime. It would better to make these readonly variables in the respective encoding types:

public class ASCIIEncoding
{
    internal readonly ASCIIEncoding Instance = new ASCIIEncoding();
...

Similar pattern is used in number of places in coreclr and corefx.

[jamesqo]

@jkotas I saw your suggestion in the other issue, however I did not follow it for reasons of code cleanliness (people might wonder why there is a static field being used nowhere in the file).

Perhaps what I'll do is make all the Encoding subclasses partial, and then have a file like Encoding.StaticProperties.cs (probably with a better name) and keep all of the static property-initialization logic in there. That way, the comment will not have to be repeated per-Encoding, and there won't be a need for all of these new classes.

[jkotas]

I do not see a problem with internal static field that is not used anywhere else in the file. It is pretty common with the existing uses of this pattern.

One example out of many:

coreclr/src/mscorlib/src/System/Diagnostics/Eventing/TraceLogging/TraceLoggingDataCollector.cs

Line 27 in 775003a

internal static readonly TraceLoggingDataCollector Instance = new TraceLoggingDataCollector();

[jamesqo]

@jkotas Yeah, but the example you pointed out is a singleton. UTF8Encoding on the other hand has a public constructor, so naming it Instance or s_instance will be confusing since you can create multiple instances of it.

[jkotas]

It can be called DefaultInstance

[jamesqo]

@jkotas I have made the changes you suggested; thanks for reviewing. (I used Interlocked.CompareExchange pattern to initialize the Default property.)

[jkotas]

This generates warnings:

warning CS0108: 'ASCIIEncoding.Default' hides inherited member 'Encoding.Default'. Use the new keyword if hiding was intended.

[jkotas]

readonly field would have same effect, with more compact IL...

[jamesqo]

Done.

[jkotas]

This is not right

[jamesqo]

Test Windows_NT x64 Release Priority 1 Build and Test
Test Windows_NT x86 ryujit Checked Build and Test
Test Linux ARM Emulator Cross Debug Build

[jkotas]

Thanks!