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

Add TextReader and TextWriter Span/Buffer-based APIs #22840

Closed
3 tasks done
stephentoub opened this issue Jul 18, 2017 · 10 comments
Closed
3 tasks done

Add TextReader and TextWriter Span/Buffer-based APIs #22840

stephentoub opened this issue Jul 18, 2017 · 10 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@stephentoub
Copy link
Member

Separated out of https://github.com/dotnet/corefx/issues/21281 for tracking purposes.

  • Implement in System.Runtime.Extensions
  • Expose from System.Runtime.Extensions contract
  • Add tests to System.Runtime.Extensions.Tests
namespace System.IO
{
    public class TextReader
    {
        public virtual int Read(Span<char> destination);
        public virtual ValueTask<int> ReadAsync(Buffer<char> destination, CancellationToken cancellationToken = default(CancellationToken));
        public virtual int ReadBlock(Span<char> destination);
        public virtual ValueTask<int> ReadBlockAsync(Buffer<char> destination, CancellationToken cancellationToken = default(CancellationToken));}

    public class TextWriter
    {
        public virtual void Write(ReadOnlySpan<char> source);
        public virtual Task WriteAsync(ReadOnlyBuffer<char> source, CancellationToken cancellationToken = default(CancellationToken));
        public virtual void WriteLine(ReadOnlySpan<char> source);
        public virtual Task WriteLineAsync(ReadOnlyBuffer<char> source, CancellationToken cancellationToken = default(CancellationToken));}
}

EDIT 7/25/2017: Updated with CancellationTokens per API review.

@WinCPP
Copy link
Contributor

WinCPP commented Aug 15, 2017

Thanks @stephentoub. Will work on this.

@WinCPP
Copy link
Contributor

WinCPP commented Aug 15, 2017

Just to make sure I understand correctly... In the opening comment for API to be added, Read and ReadBlock work with Span<char> whereas ReadAsync and ReadBlockAsync work with Buffer<char>. I guess these differences are just for illustrative purpose and actually all four methods are to be provided for Span<char> as well as Buffer<char>. Likewise for Write* methods...?

@stephentoub
Copy link
Member Author

I guess these differences are just for illustrative purpose and actually all four methods are to be provided for Span as well as Buffer

No, these are the actual APIs. The synchronous ones take spans, the asynchronous ones take buffers.

@WinCPP
Copy link
Contributor

WinCPP commented Aug 16, 2017

Ok. I believe that is out of the fact that spans are on stack (?) and hence can be used in sync APIs only whereas buffers are on heap and hence can be used in async APIs. But then buffers could be used in sync versions as well right... But one could create a span on buffer and use it in sync API...

I'm still not clear about spans and buffers, the new ways... Where can I get to read up more? Google doesn't show up much...

@stephentoub
Copy link
Member Author

I believe that is out of the fact that spans are on stack

Yes. Spans are a ref-like type, which comes with various constraints, including that it can't live on the heap. And that means it can't be an argument to an asynchronous method that might need to capture that argument to live on the heap for the duration of the operation.

But then buffers could be used in sync versions as well right

Yes. But a) spans are cheaper than buffers, and b) if you have a span and a method takes a buffer, you'd have no way to call it. In that sense spans are sort of a universal receiver: if you have a method that takes a span, you can call it if you have either a span or a buffer, but if you have a method that takes a buffer, you can't call it with a span. Thus, wherever possible, we'd ideally like methods to take spans.

@WinCPP
Copy link
Contributor

WinCPP commented Aug 20, 2017

@stephentoub I'm unable to locate the tests / performance tests for TextReader / TextWriter. Please let me know where I can find them...

I'm doing this work in parts. First TextReader and TextWriter, followed by StringReader/StringWriter/StreamReader/StreamWriterand then theBuffer` based overloads...

@WinCPP
Copy link
Contributor

WinCPP commented Aug 26, 2017

@stephentoub bumping up the thread...

Between I am working on adding new overloads in StreamReader which has the apis working on char buffer all the way down to (abstract) System.Text.Decoder and its various implementations in System.Private.CoreLib. Likewise is the case for Write* overloads that use Encoder implementations. So do we want to add Span based implementations in the Encoder / Decoder interface and implementations (CoreLib) as well such that,
a. Span<char> based overloads in Text/Stream/String Reader/Writers use the span based overloads in Decoders and Encoders.
b. char based overloads in Text/Stream/String Reader/Writers start using the span based overloads in Decoders and Encoders.

Being a big change, wanted to check if I am thinking right.

Quick dirty alternative may be to call the existing char buffer based overloads in Text/Stream/String Readers/Writers inside the Span<char> based overloads with copy to-and-fro span and buffer. That would introduce the Span<char> based overloads in right places but would be super inefficient.

Appreciate your inputs.

@stephentoub
Copy link
Member Author

I'm unable to locate the tests / performance tests for TextReader / TextWriter.

It's possible there's a test gap here. They should be in System.IO\tests in corefx. @JeremyKuhne? @ianhays?

So do we want to add Span based implementations in the Encoder / Decoder interface

They already exist. Double check that your repo is up-to-date. They were added a week or two ago.

@WinCPP
Copy link
Contributor

WinCPP commented Aug 27, 2017

@stephentoub Thanks. Yup. Updating the repo, I got the Encoder / Decoder interface implementation. Mine was a bit more stale than two weeks. Also I got the Stream/String Reader/Writer tests in the Test project in System.IO. I don't think I find for TextReader/Writer...

@ianhays
Copy link
Contributor

ianhays commented Aug 28, 2017

It's possible there's a test gap here. They should be in System.IO\tests in corefx. @JeremyKuhne? @ianhays?

iirc there aren't any specifically for TextREader/TextWriter and the justification was that StreamReader/StreamWriter tests provide coverage for them. Really we should have some tests specifically for TextReader/TextWriter that use a basic/dummy implementation.

beniamin-airapetian referenced this issue in beniamin-airapetian/corefx Sep 23, 2017
* Microsoft.ServiceModel.Syndication skeleton project

* Adding the existing classes of SyndicationFeed from .net fx

* Added the needed references to get the code to compile

* Changed some namespaces

* Fixed errors when reading feeds and replaced some buffers

* Cleaning code for PR

* Deleted some unused files

* Added the posibility that the user creates his own date parser when reading a feed from outside, also as part of our default date parser, if it can't parse the date it will just assign a default date to avoid the crash

* Added correct testnames and Copyright

* Initial changes to add custom parsers

* Added more delegates as parsers

* Added custom parser delegates

* Save changes

* Initial SyndicationFeed

* Removed the dependence of the old SR class

* Cleaned the code, deleted Diagnostics, fixed throwing resources

* Cleaned code from most of the unnecesary comments

* Formatted the code with CodeFormatter Tool

* Moved the call of itemParser to be called with each item

* Test using custom parsers

* Fixed image issue where image tag was using original title and url, for RSS formatting
Image issue fixed and added some tests

* Test clase jorge

* Save changes with Jorge cass

* Initial Jorge

* saving changes

* Save changes

* Fixed image and items issue

* Fixed disjoint items in atom

* Run codeFormatter

* Adding parsing to optional elements int the spec
added unittesting

* Added Icon parsing to Atom10 feedFormatter. Added unit test

* Adding Async Writing
RssFormater writes new optional spec elements

* Added Icon writing on Arom Writer

* Fixed some warnings

* Improved custom parsing for RSS

* Added custom parsing for Atom feed formatter, added test

* added nameof() to all exceptions when needed.

* Adding Extension Methods for XmlReader

* Fixing code for PR

* Fixing code for PR

* Added check for skip days to allow only accepted days.

* Improved flexibility for Default dateparser

* Add wrong dates example

* Fixing code review

* Fixed warnings on some unawaited methods.

* Added async extension methods for XmlReader

* Add XmlWriterWrapper method extensions

* Changed ReadCategoryFromAsync to return a SyndicationCategory

* Fixed sync method exposed GetReader.

* Edited XmlReaderWrapper, moved methods to extension methods.

* Removed unnecesary variables from Wrappers.

* Fixed bug from ServiceModel.Syndication

* Make BigInteger ctor opt path for == int.MinValue reachable.

The BigInteger(ReadOnlySpan<byte> value) ctor contains an optimised
path for value being larger than 4 bytes and the result being equal to
int.MinValue.

However this path is inside a test that precludes that value, so can
never be reached.

Restructure so that the path is reachable.

* Fix ServiceModel.SyndicationFeed project dependencies
- Change M.SM.SyndicationFeed to be a .NET Standard 2.0 library
- Change tests to use official .NET Core 2.0 release from preview

* Add btrfs and other missing file system types (dotnet#24102)

* Uppercase

* Add more filesystems from stat.c. Change to stat names where possible

* Add some more local friendly names

* Add some more remote file types from stat.c

* Add entry to switch present in enum

* Build errors

* Remove Fixed entries that should be RAM

* comment

* Change case of 0X to 0x

* Move cramfs to Fixed

* ConcurrentQueue 128-byte cache line (dotnet#22724)

ConcurrentQueue 128-byte cache line

* Add warning by default in SGEN (dotnet#24054)

* Output warning by default if run the tool directly without /quiet parameter.

* add quiet parameter in the command.

* fix parameter error.

* Update the warning.

* Add the target to copy the serializer to publish folder. (dotnet#24096)

* Wrap cert callback leaked exceptions in WinHttpHandler (dotnet#24107)

Similar to the fix done for CurlHandler (dotnet#21938), fix WinHttpHandler so
that leaked exceptions from the user-provided certificate callback are
properly wrapped.

The ManagedHandler still needs to be fixed since it is not wrapping
properly.

Contributes to #21904

* Ensure ProcessModule for main executable is first in modules list (dotnet#24106)

* Disable NegotiateStreamTest fixture for distros without working Kerberos (dotnet#24098)

Disable NegotiateStreamTest fixture entirely because setup-kdc.sh is broken on some distros

* Expose and add tests for Guid.ctor(Span)/TryWriteBytes/TryFormat

* Address remaining PR feedback

* #24112 Replaced documentation summary of TryPop with text similar to TryDequeue. (dotnet#24113)

* Wrap exceptions from ManagedHandler's server validation callback (dotnet#24111)

To match other handlers' behaviors.

* Fix libgdiplus function loading on OSX.

* Prevent ServiceControllerTests from dispose when already disposed (dotnet#24042)

* Disable ServiceProcessTest that has been failing on CI and official builds

* Prevent dispose from been called when already disposed

* Add logging to repro if RemoveService is been called twice on the same ServiceController

* Data-annotations length fix (dotnet#24101)

* Updated in MinLengthAttribute and MaxLengthAttribute to support ICollection<T>

* Added tests

* Fixed typo

* Trying to address two failing checks:
- Linux x64 Release Build
- UWP CoreCLR x64 Debug Build

* Implemented changes requested in review
- Extracted Count checking to an external helper to obey DRY
- Removed dependency of ICollection<T> and changed to simple reflection Count property lookup

* Added requested tests

* Added catch for MissingMetadataException.

* Extracted code from try-catch.

* Added comment as requested.

* Typo correction

* Remove System.Drawing.Common from the netfx compat package (temporary).

* Updating corefx to use new roslyn compiler (dotnet#24076)

* Updating corefx to use new roslyn compiler

* Updating to new version of the compiler and use the switch so tests pass on desktop

* Fix System.Reflection.Metadata tests

* Stop running Math.Clamp tests on UAP as API is not there (dotnet#24125)

* Stop running Math.Clamp tests on UAP as API is not there

* Disable only for AOT

* Fix instantiating PrincipleContext  with Domain type (dotnet#24122)

* Fix instantiating PrincipleContext  with Domain type

* Enhancement

* Disable Test_Write_Metric_eventListener on UWP. (dotnet#24127)

* Revert "Remove System.Drawing.Common from the netfx compat package (temporary)."

This reverts commit f6b0fbd.

* Update BuildTools, CoreClr, CoreFx, CoreSetup, Standard to prerelease-02019-01, preview1-25718-02, preview1-25718-03, preview1-25717-02, preview1-25718-01, respectively (dotnet#24075)

* Fixed compile warning/error on FreeBSD (dotnet#24141)

* Marking {ReadOnly}Span as readonly structs (dotnet#23908)

* Marking {ReadOnly}Span as readonly structs, fixing issue #23809

* Adding readonly attributes to reference assemblies.

* Using "readonly ref" keyword instead of attributes.

* Adding a LangVersion 7.2 property

* System.Drawing: Throw ArgumentNullException on Unix as well (dotnet#24140)

* Throw ArgumentNullException when stream is null

* Throw ArgumentNullException when stream is null

* Remove invalid NameResolution tests (dotnet#24147)

The Dns_GetHostEntryAsync_* are fundamentially invalid because it's
possible for the broadcast address, 255.255.255.255, to have an DNS
mapping via manually modifying the hosts file.  This was actually
happening on Mac systems as well as an virtual environment running on
top of that (i.e. Windows on Parallels).

Ref:
https://github.com/dotnet/corefx/issues/23992#issuecomment-330250642

Contributes to #23992

* Bump system.drawing.common.testdata to 1.0.6

* Update BuildTools to prerelease-02019-02 (dotnet#24146)

* Fix path to test data

* Fix whitespace

* Add GraphicsTests based on Mono's test suite.

* Consolidate more code in the "System.Drawing" namespace.

* Remove all remaining Win32 codepaths from the mono codebase. All of this
  code now implicitly assumes that it will be run on a Unix platform.
* Consolidate the rest of the gdipFunctions.cs file into Gdip.cs and
  GdipNative.Unix.cs
* Consolidate the GraphicsUnit and ImageType enumerations -- they were
  duplicated.
* Remove the mono Status enum and use the Windows constants instead in all
  Unix code.
* Move all files into the regular directory structure. Suffix them with
  ".Unix" and ".Windows" when there are collisions.

* Tiny bit of code cleanup

* Add conditionals for recent versions of mono

* Remove duplicate tests.

* Fix multiplying TextureBrush with a disposed matrix on Unix (dotnet#24109)

* Fix multiplying TextureBrush with a disposed matrix on Unix

* Enable another passing test

* Fix accidentally committed file

* Add an error code fixup to Bitmap.Unix.cs to match Windows.

* Bump System.Drawing.Common.TestData to 1.0.6 (dotnet#24149)

* Bump system.drawing.common.testdata to 1.0.6

* Fix path to test data

* Fix whitespace

* Cleanup - Add/simplify using statements of disposable resources (Graphics, Bitmap)

* Validate HatchStyle passed to HatchBrush ctor

* Delete accidentally duplicated HatchBrush tests in LinearGradientBrushTests

* Remove references to historical Mono bug IDs

* Renable some already passing tests

* Remove Thread.Sleep workaround

* Update BuildTools to prerelease-02020-01 (dotnet#24172)

* Add thread-local based switch to opt-in to ManagedHandler

* Address PR feedback

* PR feedback

* Fix memory map imports (dotnet#24176)

* Fix memory map imports

Imports lost the last error attribute. Add it back and change the results to be the more correct "bool". Tweak the usage based on the new return type.

#24159

* Move spin wait

* Remove unused FEATURE_RANDOMIZED_STRING_HASHING (dotnet#24178)

* Adding System.Data.Odbc package and including in metapackage

* Fix MultiplyTransform with a disposed brush

* Fix for 2nd Connection hangs in Mirroring Environment (dotnet#24181)

* Switch tests to use thread-local switch for ManagedHandler

And as a result re-enable parallelism of the test suite, which on my machine reduces the running time of the outerloop tests from 150s to 45s.

* Update CoreClr, CoreSetup, ProjectNTfs, ProjectNTfsTestILC, Standard to preview1-25720-03, preview1-25719-04, beta-25721-01, beta-25721-01, preview1-25721-01, respectively

* CoreFx #22406 Span based APIs - Text Reader Writer (dotnet#23786)

* [Drawing] Move remaining "Unix" files into regular directory structure

* As part of this, the "Windows" version of ImageFormat.cs is now being
  used everywhere. This file just contains a bunch of GUID's that are not
  platform-specific in any way.

* Change AsSpan to property Span and rename AsMemory to Memory

* Add Metafile and MetaHeader tests based on Mono's System.Drawing unit tests

* Update project file

* MetafileTests: Remove duplicates, and re-enable tests on Unix which are now working.
Delete duplicate MetaHeadertests

* Rationalize using statements

* Update project file

* Fix Metafile exception behavior

* Simplify tests, remove duplicate test

* Don't catch an ArgumentException just to get a NullReferenceException instead.

* Assert.False(Object.ReferenceEquals => Assert.NotSame

* PR feedback

* Remove duplicate tests, fix typo

* Remove X11 dependency for tests which are enabled on Unix.

* Workaround libgdiplus glitches

Don't test metafileHeader.Bounds when using libgdiplus.
Don't test metafile.GetBounds on Unix, force MetafileHeader.Bounds to return an empty rectangle when MetafileSize == 0
Don't validate graphicsUnit on Unix.

* Move Syndication to root/src

* Disable Build of S.SM.Syndication.

* Increase file descriptor limit in S.N.Http tests on macOS

* Expose/test String.Create span-based method (dotnet#23872)

* Expose/test String.Create span-based method

* Address PR feedback

* Update build to clang/llvm 3.9 (dotnet#24177)

Update scripts, docs and build pipeline docker images to clang/llvm/lldb 3.9

* Update ProjectNTfs, ProjectNTfsTestILC, Standard to beta-25722-00, beta-25722-00, preview1-25722-01, respectively (dotnet#24207)

* Remove the line that will copy the generated serializer to the pack. (dotnet#24199)

* Revert "Update build to clang/llvm 3.9 (dotnet#24177)"

This reverts commit 21e008a.

* Remove stale SetStateMachine call in test

* Ssl Stream Async Write  (dotnet#23715)

* Change from APM to Async/Await for write side

* Added nameof

* Reacting to review

* Reacting to review

* SSLStream : Fixed spelling mistake in file name (extra a) (dotnet#24221)

* Fixed spelling mistake in file name

* Fix csproj

* Update ProjectNTfs, ProjectNTfsTestILC to beta-25723-00, beta-25723-00, respectively (dotnet#24222)
@stephentoub stephentoub assigned stephentoub and unassigned WinCPP Oct 4, 2017
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

No branches or pull requests

5 participants