Avoid unnecessary copy in TextWriter.Write(string)

[logiclrd]

This PR updates the Write(string?) overload in TextWriter.cs to avoid copying the string data.

[logiclrd]

TextWriter's implementation funnels writes to a small number of common implementations that operate on individual characters. If you pass a string to Write, it hands it off to another overload. I was poking around and noticed that Write(string) -- likely the most commonly-called overload and an underlying implementation for many of the other overloads -- is currently implemented using a call to .ToCharArray(), which allocates a new char[] and copies the string data into it. Perhaps there's a good reason for doing this, but I notice there is a Write(ReadOnlySpan<char>) overload available and string converts to that argument type in O(1) with no allocation or copying.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

[logiclrd]

I notice that the Write(ReadOnlySpan<char>) overload actually copies the data as well. But it does avoid allocation by using ArrayPool<char>. Why is it done this way? Couldn't it write the characters straight out of the span??

[logiclrd]

I dug into one of those failures and found this:

/tmp/helix/working/B269099C/p/build/apple/AppleBuild.targets(260,5): error : LLVM ERROR: IO failure on output stream: No space left on device 

😛

[huoyaoyuan]

This is a behavioral change. It will change the virtual method called in derived class. Similar to #68800.

[huoyaoyuan]

Perhaps there's a good reason for doing this, but I notice there is a Write(ReadOnlySpan<char>) overload available and string converts to that argument type in O(1) with no allocation or copying.

Any derived class that cares about performance can override all these virtual methods. The base class has to be fully compatible that Write(char[]) is considered lower level.

Why is it done this way? Couldn't it write the characters straight out of the span??

It's the same as Span methods of Stream, which are added later and can't be used as source of truth in base class.

[logiclrd]

Mm, okay. The fact that the char[] overload is expected to be called by other overloads doesn't appear to be documented. The documentation says only that everything funnels down to the char overload which thus, minimally, be implemented.

In my opinion, either it shouldn't be guaranteed by the implementation, or it should be documented. Unless there's documentation I'm unaware of (quite possible), the current state feels kind of hand-wavy.

Is switching from char[] to char[], index, offset also not allowed for the same reason? If not, then perhaps it would be possible to eliminate the allocation, if not the copying, by using ArrayPool<char>.Shared...

[huoyaoyuan]

In my opinion, either it shouldn't be guaranteed by the implementation, or it should be documented.

There are more contracts implied by other rules beyond specific documentations. Given the history and other decisions in the area, the behavior around virtual methods is considered important.

Is switching from char[] to char[], index, offset also not allowed for the same reason?

It's probably because the behavior was such since .NET Framework 1.0.
Any derived class cares about performance should override these different methods, like Streams.

[vcsjones]

Mm, okay. The fact that the char[] overload is expected to be called by other overloads doesn't appear to be documented.

It's documented as a breaking change here:

Removing a call to a virtual/abstract member

Documentation or not, changing things like this is more or less guaranteed to break someone. Breaks for performance improvements are, in my opinion, hard to justify, especially when a derived type can simply override the implementation themselves if they are sure of the desired behavior.

[logiclrd]

Fair enough :-) Thanks for taking the time to elaborate.