Improve LargeEncodedText performance

[pharring]

Issue #516 talks about a performance problem with LargeEncodedText.
I fixed that, but along the way I had to reconcile the duplication of code between SourceText, EncodedStringText and LargeEncodedText.
LargeEncodedText is entirely portable, so it was easily moved into the Portable compiler assembly.
Most of EncodedStringText is portable. The class itself is just a re-implementation of StringText. The non-portable stuff remains: Using Encoding.Default as a fallback, throwing if a binary file is detected and special-casing FileStream and MemoryStream.

[tmeschter]

Don't forget to run the API change by the compatibility council.

[tmat]

I don't think we need any new public surface nor change the current.

[pharring]

Allow me to explain the public API changes. They're all in SourceText.

1. De-virtualize the "Lines" property and add a protected virtual GetLinesCore method. This is the typical pattern. It's not expected that derived types have to call the base implementation. I could revert this change if it seems frivolous.
2. Add an optional throwIfBinaryDetected parameter to SourceText.From. Defaults to false to maintain the original behavior. This is required because EncodedStringText and SourceText had deviated in their behaviors around binary files.
3. Introduce a new SourceText.From overload that takes a byte array instead of a stream. This is required because the optimizations for (small) MemoryStream and FileStream read the original Stream into a byte[] buffer.

You might argue that 2 and 3 could have been done with internal methods. However, the wrinkle is that EncodedStringText (the Desktop entry point that contains optimizations for FileStream and MemoryStream, has the "throwIfBinaryDetected" behavior and uses Encoding.Default as the fallback instead of Utf8) is also used in the Workspaces.Desktop layer (there's a linked file reference). It therefore can't use any compiler internal APIs).

[pharring]

@AnthonyDGreen For the attention of the compat council. Please let me know how you'd like me to proceed.

[tmat]

My comment re the parameter throwIfBinary is lost somewhere in the commits. Why couldn't the caller do the check for binary content after SourceText.From returns the SourceText instance?

[pharring]

@tmat Re:"throwIfBinary". It's for the LargeEncodedText case. The 'throwIsBinary' implementation works on the chunks as they are being decoded. It would be inefficient to do the binary check after the fact because LargeEncodedText's indexer is slow.

[mattwar]

👍

[pharring]

@AnthonyDGreen I'm still waiting for a comment from the compat council. Please take a look at the public API changes and let me know what you think.

[tmat]

@pharring I still don't like the flag and throwing an exception. It would be better if we added a method IsBinary() on SourceText

[AnthonyDGreen]

@pharring, is this a complete and accurate description of the compat impact of the change?

Source

1. Source that extends SourceText and attempts to override the Lines property will now fail to compile.
2. Source that called SourceText.From(Stream) will continue to compile but will silently call a new overload with a default value which preserves the current behavior.
   Binaries
3. Binaries that extended SourceText and override the Lines property will now fail are runtime when running against the next version of Microsoft.CodeAnalysis.dll
4. Binaries that call SourceText.From(Stream) will fail at runtime with MissingMethodException when running against the next version of Microsoft.CodeAnalysis.dll

Have I overlooked anything that you're aware of?

[pharring]

@AnthonyDGreen Yes, that's complete and accurate.

[pharring]

@tmat I tried to explain that already. SourceText.IsBinary() would have to be public and virtual.
Public so it could be called from the Workspaces Desktop assembly and virtual so it could be overridden in LargeEncodedText for efficiency.
We'd also lose the benefit of checking during decoding that LargeEncodedText does. If you have a giant binary file (e.g. you accidentally pass a 1GB binary file to SourceText.From), then we can detect "IsBinary" and throw before we've decoded the entire stream.

[tmat]

@pharring I see. OK. (Public IsBinary would be better imo, but I didn't realize we throw before we decode the entire stream, in that case sounds good).

[AnthonyDGreen]

+1. I think this is a simple enough change with low risk. Go for it.