Re-design Utf8JsonWriter as a regular class rather than a ref struct. #36961

ahsonkhan · 2019-04-17T12:16:41Z

The first commit (i.e. a19d268 - Add an in-box array-backed IBufferWriter), fixes https://github.com/dotnet/corefx/issues/34894.
Related PR: #35094

That commit can be reviewed independently of the Utf8JsonWriter changes but since there was a strong dependency between these components, I submitted them together.

The remaining commits update the Utf8JsonWriter design based on recent usability and reliability feedback: https://gist.github.com/ahsonkhan/f6f30656717548212693e5eaa49cece5

API Review Feedback TL;DR:

We should aim to provide built-in stream support and simplify its usage even more than proposed.
The Utf8JsonWriter should implement IDisposable and IAsyncDisposable (on latest platform).
BytesWritten should be renamed to BytesPending
Auto-flush on dispose and remove isFinalBlock bool on flush (the user would need to validate correctness themselves).
Add Reset() and Reset(...) overloads that allow re-using the Utf8JsonWriter instance.
Remove Clear() and clear the memory field on flush to avoid forcing the caller to reset after flushing.
Consider adding leaveOpen/dispose bools on ctor to close/dispose the stream on Utf8JsonWriter.Dispose (?).
Keep the JsonWriterOptions a struct. It cannot be modified once passed in to the constructor.
Internally create an ArrayBufferWriter to have the same code path for both Stream and IBufferWriter modes.

Left TODO:

Remove duplicate code by refactoring common functionality: https://github.com/dotnet/corefx/issues/36987
Investigate writing large string values in 4 KB chunks within Utf8JsonWriter instead all at once: https://github.com/dotnet/corefx/issues/36958

This change also fixes https://github.com/dotnet/corefx/issues/35182

Note: Fixed sized IBW without slack capacity are generally not supported since the writer tends to eagerly ask for space and an exact, fixed-sized IBW would fail the GetSpan(minimumSize) contract.

cc @davidfowl, @KrzysztofCwalina, @stephentoub, @steveharter, @bartonjs, @layomia, @JeremyKuhne, @GrabYourPitchforks, @BrennanConroy, @rynowak, @pranavkm, @pakrym, @terrajobst, @glennc, @joshfree, @JamesNK

delimiter

…gnUtf8JsonWriter

src/Common/src/System/Buffers/ArrayBufferWriter.cs

steveharter · 2019-04-17T15:45:20Z

src/Common/src/System/Buffers/ArrayBufferWriter.cs

+#else
+    public
+#endif
+    sealed class ArrayBufferWriter<T> : IBufferWriter<T>


Add where T:Struct to avoid mixing objects and memory.

I would like to match the IBufferWriter<T> constraint here (i.e. none). If there is strong opposition, we should discuss both outside of this PR in an API review.

I'd rather not artificially constrain it, unless there's good reason to.

Calling GetMemory() would be a bit odd with an array of objects. I'm assuming we don't do anything unsafe with these object pointers like mem copying (in the middle of a gc).

Calling GetMemory() would be a bit odd with an array of objects.

Why?

The scenario is a bit edge case IMO - when would you pass a writeable buffer of objects? There's an indirection in memory (element->object) and "ownership" now becomes a bit more ambiguous - i.e. what if the objects implement IDispose -- who's going to call that?

It's no different than storing objects into an array; when do you pass arrays of things, like strings? You also can have the exact same ownership question with structs, which might themselves have resources that have lifetime, or wrap any number of reference types. Limiting it is just an artificial restriction.

I agree that limiting isn't a good thing if we don't need to.

Since Memory has lifetime semantics (don't hold onto it after your method returns etc) it is not the same as an array.

Consider a buffer type that is only for data (or primitives like char or byte - not objects) and in that case there could be some memory clear and copy-like efficiencies regarding clearing after use and shuffling unprocessed elements to the beginning when making room for more data. Originally I was thinking we may be doing some of these operations here which would make it unsafe for objects...

src/System.Text.Json/src/System/Text/Json/Serialization/PooledBufferWriter.cs

src/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Byte.cs

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs

src/Common/src/System/Buffers/ArrayBufferWriter.cs

ahsonkhan

Given this is a large, time sensitive change, and to stir up some more targeted review, I have highlighted/commented on a few things that I would like feedback on (or at least a second set of eyes).

ahsonkhan · 2019-04-17T21:59:54Z

src/Common/src/System/Buffers/ArrayBufferWriter.cs

+    /// Represents a heap-based, array-backed output sink into which <typeparam name="T"/> data can be written.
+    /// </summary>
+#if USE_ABW_INTERNALLY
+    internal


Is this pattern acceptable to use? If not, I am open to suggestions.

We do this in other places as well, but I think it would be better to invert the condition, such that the type is only public if you explicitly set a compilation constant... otherwise, if a project includes this file, by default it's going to be exposing a new type publicly.

ahsonkhan · 2019-04-17T22:00:45Z

src/Common/src/System/Buffers/ArrayBufferWriter.cs

+            if (initialCapacity <= 0)
+                throw new ArgumentException(nameof(initialCapacity));
+
+            _buffer = new T[initialCapacity];


Do we want to bound initialCapacity to MinimumBufferSize from the bottom or is this fine?

ahsonkhan · 2019-04-17T22:02:21Z

src/Common/src/System/Buffers/ArrayBufferWriter.cs

+
+            if (sizeHint == 0)
+            {
+                sizeHint = MinimumBufferSize;


Any feedback/concerns on this growth strategy? Any call to GetSpan/GetMemory will return at least 256 bytes, if sizeHint is not specified.

This has consequences when used with an ArrayBufferWriter ctor that has initialCapacity.

I was contemplating this when looking earlier. I'd say that sizeHint of 0 should imply sizeHint of 1 (simply "not empty" but avoids growing if it can)

I had considered this, but in practice, most workloads would suffer from this since they can't make forward progress with 1 byte. It effectively makes calling GetSpan without an argument useless. I am not sure returning 1 is better. If there are no strong opinions here, I would let scenario-driven user feedback guide the change, and leave it as is for now.

in practice, most workloads would suffer from this since they can't make forward progress with 1 byte. It effectively makes calling GetSpan without an argument useless.

I dunno, once the regrow happens it'll double in size, so you'll end up with one call between 1 and 256, then the "oh, I need more than that..." would get a bigger buffer.

I would let scenario-driven user feedback guide the change, and leave it as is for now.

Also seems valid, but if the caller really doesn't have any notion of a hint then it seems like any size is acceptable. Otherwise passing in min/max/accurate seems like the best bet.

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleValue.cs

ahsonkhan · 2019-04-17T22:07:23Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.cs

                }

-                if (flushThreshold >= 0 && writer.BytesWritten > flushThreshold)
+                if (flushThreshold >= 0 && writer.BytesPending > flushThreshold)


Note that writer.BytesWritten previously showcased the total written (i.e. BytesCommitted + whats still pending to be flushed). BytesPending now only returns the pending data. Not sure if the semantics were broken before, or now (given that we previously were creating a new Utf8JsonWriter every time, but now are not, which likely resolves the issue).

BytesPending seems right. Basically we want to call the writer before we fill the buffer too much and force it to grow if the next write pushes it over the edge (can still happen, but shouldn't be common).

ahsonkhan · 2019-04-17T22:20:07Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private void FlushHelper()
+        {
+            _output.Advance(BytesPending);


For the stream mode, should I use the concrete type _arrayBufferWriter.Advance() rather than going through an interface call like this?

Why do we have this helper at all? It's a single line in a method (with a non-descriptive name) marked AggressiveInlining.

Artifact of merging two concepts/terminology. With stream/IBW mode, we have "advance IBW" and we have "write/flush to stream", which effectively have the same semantics as far as the JsonWriter is concerned so I normalized it. This just helped me make sense of what operations I am doing while reading the source (normalizes to call Flush... for both cases).

This goes back to my original question of should I call _output.Advance in the stream case or call _arrayBufferWriter.Advance?

Do you think I should remove it regardless?

I suggest you remove it and then at the call sites instead do whatever is the more efficient call.

ahsonkhan · 2019-04-17T22:22:37Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs

-            GrowAndEnsure(minimumSize);
-            alreadyWritten = 0;
-        }
+            int sizeHint = Math.Max(DefaultGrowthSize, requiredSize);


Is this a reasonable growth-scheme?

ahsonkhan · 2019-04-17T22:23:03Z

src/System.Text.Json/tests/JsonElementWriteTests.cs

+        [Theory]
+        [InlineData(false)]
+        [InlineData(true)]
+        public static void WriteNumberAsPropertyWithLargeName(bool indented)


Improves some test coverage.

ahsonkhan · 2019-04-17T22:24:58Z

src/System.Text.Json/tests/Utf8JsonWriterTests.cs

+            Assert.Equal(expectedString, actualString);
+        }
+
+        private static async Task WriteLargeToStreamHelper(Stream stream, JsonWriterOptions options)


This test captures the expected usage pattern for most users.

ahsonkhan · 2019-04-17T22:26:50Z

src/System.Text.Json/tests/Utf8JsonWriterTests.cs

+            jsonUtf8.Flush();
+
+            // Explicitly skipping flushing here
+            var invalidUtf8 = new byte[2] { 0xc3, 0x28 };


This test captures the current semantics for writing comments that contain invalid data. Let me know if its fine as is.

ahsonkhan · 2019-04-17T22:30:43Z

@mairaw, @rpetrusha - please feel free to review/provide feedback on some of the xml comments (particularly for newer APIs like Reset and the change from ref struct to class). This has implications on the recent docs change which will need to be updated: dotnet/dotnet-api-docs#1880

Your feedback/insights here would be much appreciated :)

steveharter · 2019-04-17T23:08:31Z

@ahson it's not clear to me if we can skip the escaping logic. Previously the serializer would ensure the property name is already escaped and skip the expensive check.

I believe we discussed creating a new "resolved string" type that could be leveraged to avoid the perf hit.

ahsonkhan · 2019-04-17T23:13:05Z

it's not clear to me if we can skip the escaping logic. Previously the serializer would ensure the property name is already escaped and skip the expensive check.

I believe we discussed creating a new "resolved string" type that could be leveraged to avoid the perf hit.

That's TBD atm. I will submit a proposal for this outside this PR and provide that functionality. Is there anything in the serializer code (in master) that used bool escape: false? I didn't see any.

steveharter · 2019-04-17T23:23:01Z

src/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.String.cs

+
+namespace System.Buffers.Tests
+{
+    public static partial class ArrayBufferWriterTests_String


I didn't realize we want to support this on non-primitive types; string is normally immutable as well.

I didn't realize we want to support this on non-primitive types

This is a question on the interface, tbh:

corefx/src/System.Memory/src/System/Buffers/IBufferWriter.cs

Line 10 in 818355a

public interface IBufferWriter<T>

The test using string is just to verify that T : reference type works fine. As you mentioned, strings are immutable, so I don't expect anyone to create an ABW<string>.

I can remove this test all together.

It doesn't have any constraints on T. Maybe it should be T: struct or T: unmanaged.

cc @davidfowl, @stephentoub

Also cc @jkotas, if you have any thoughts on the issue.

IBufferWriter shipped without any constraints. Do we consider it to be a mistake?

Since IBufferWriter does not have any constraints, ArrayBufferWriter should not have any constraints either for consistency.

steveharter · 2019-04-17T23:30:41Z

it's not clear to me if we can skip the escaping logic. Previously the serializer would ensure the property name is already escaped and skip the expensive check.
I believe we discussed creating a new "resolved string" type that could be leveraged to avoid the perf hit.

That's TBD atm. I will submit a proposal for this outside this PR and provide that functionality. Is there anything in the serializer code (in master) that used bool escape: false? I didn't see any.

Not currently. However all of the write method in the serializer need to be changed to support pre-escaping the property name (pass false) for performance. I was planning to address this when we add the data type extensibility (needs tests).

…gnUtf8JsonWriter

ahsonkhan · 2019-04-18T00:10:45Z

src/System.Text.Json/pkg/Microsoft.Bcl.Json.Sources.pkgproj

  <Target Name="GetSourcesToPackage" BeforeTargets="ExpandProjectReferences">
    <ItemGroup>
-      <_ProjectsToBuild Include="../src/System.Text.Json.csproj" UndefineProperties="Configuration" />
+      <_ProjectsToBuild Include="../src/System.Text.Json.csproj" UndefineProperties="Configuration" AdditionalProperties="TargetGroup=netstandard"/>


@joperezr, please review the packaging change :)

Left a comment regarding the define constant that you declare for netstandard, since you probably don't want to require a constant to be defined in order for the source package to work. Other than that, this particular change on the pkgproj looks good to me.

ahsonkhan · 2019-04-18T04:05:34Z

If there is no other critical feedback, I would like to merge this in to give upstream repositories time to react accordingly (at least to the API breaking changes). Other changes, like code refactoring, etc., can be made in a subsequent PR.

stephentoub · 2019-04-18T11:58:34Z

src/Common/src/System/Buffers/ArrayBufferWriter.cs

+        private T[] _buffer;
+        private int _index;
+
+        private const int MinimumBufferSize = 256;


This isn't actually MinimumBufferSize, at least not how it's currently used. A better name for the current usage would be DefaultInitialBufferSize, right?

Yes, I was thinking the same thing. The only concern is that I use this to define the minimum size GetMemory/GetSpan can return as well.

The only concern is that I use this to define the minimum size GetMemory/GetSpan can return as well.

It isn't the minimum, though. It's just the value that you pretend sizeHint was if it's 0. (GetSpan(1) is allowed to return a span of Length 1)

Fair enough. I meant minimum in the "default" case, but you brought up a useful clarification.

stephentoub · 2019-04-18T12:03:13Z

src/Common/src/System/Buffers/ArrayBufferWriter.cs

+        /// </summary>
+        public ArrayBufferWriter()
+        {
+            _buffer = new T[MinimumBufferSize];


Why wouldn't we start with Array.Empty<T>()? Then if it's not actually written to, we don't allocate, and if it is written to, the first allocation will be based on the supplied sizeHint. If a typical usage is creating one of these and then passing it off to something that uses a sizeHint larger than 256, this is going to unnecessarily allocate a throw-away 256-byte buffer.

stephentoub · 2019-04-18T12:03:52Z

src/Common/src/System/Buffers/ArrayBufferWriter.cs

+            Debug.Assert(FreeCapacity > 0 && FreeCapacity >= sizeHint);
+        }
+
+        private static void ThrowInvalidOperationException(int capacity)


Nit: Seems like this should be named ThrowAdvancedTooFarException, or something like that.

stephentoub · 2019-04-18T12:05:17Z

src/Common/src/System/Buffers/ArrayBufferWriter.cs

+        /// <summary>
+        /// Returns the amount of space available that can still be written into without forcing the underlying buffer to grow.
+        /// </summary>
+        public int FreeCapacity => _buffer.Length - _index;


Do we really need this? Someone can just do Capacity - WrittenCount.

Also, WrittenCount is an uncommon name. How about Position?

The serializer uses this but you are right it is primarily a convenience thing. Do you think its exposing too much or do you just think it is unnecessary and not worth the benefit it provides?

Regarding the naming/API changes like this, how do you feel about discussing it all up in a follow up review (post preview 5)? I prefer not to make these changes in PR (in isolation). WrittenCount fits nicer with WrittenMemory/Span (especially when passed in as "offset/count" to stream.write), imo, but I am not super attached to the name.

stephentoub · 2019-04-18T12:07:36Z

src/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Char.cs

+
+namespace System.Buffers.Tests
+{
+    public static partial class ArrayBufferWriterTests_Char


Why are we duplicating all of these tests for different data types? Wouldn't it be better to just have a generic base test type parameterized on T, and then have multiple derived types that provide T==char, T==string, etc.? This is a ton of what appears to be largely duplicated code.

Yep, definitely. The only difference is that I have stream-based tests for T=byte, but that can be extracted out.

I, generally, don't mind code duplication in tests (for clarity/debugging failed tests), but in this case I agree it goes too far.

stephentoub · 2019-04-18T12:10:39Z

src/System.Text.Json/src/Resources/Strings.resx

    <value>Serializer options cannot be changed once serialization or deserialization has occurred.</value>
  </data>
+  <data name="StreamNotWritable" xml:space="preserve">
+    <value>Stream was not writable.</value>


Nit: was => is ?

Looks like I borrowed it from the wrong place:

corefx/src/System.Security.Cryptography.Primitives/src/Resources/Strings.resx

Line 73 in c390ce7

<value>Stream was not writable.</value>

We aren't consistent here: https://github.com/dotnet/corefx/search?utf8=%E2%9C%93&q=stream+was+not+writable&type=

However, I will follow what StreamWriter does and change to is :)

https://docs.microsoft.com/en-us/dotnet/api/system.io.streamwriter.-ctor?view=netframework-4.8#System_IO_StreamWriter__ctor_System_IO_Stream_

stephentoub · 2019-04-18T12:12:43Z

src/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonValueConverterChar.cs

 #if BUILDING_INBOX_LIBRARY
-            Span<char> temp = MemoryMarshal.CreateSpan<char>(ref value, 1);
+            Span<char> temp = MemoryMarshal.CreateSpan(ref value, 1);
            writer.WriteStringValue(temp);


Nit: you don't actually need a temp here; it can be a single line to parallel the other else case, e.g.

writer.WriteStringValue( #if BUILDING_INBOX_LIBRARY MemoryMarshal.CreateSpan(ref value, 1) #else value.ToString() #endif );

or something like that.

I decided to keep changes outside of the writer minimal, but that would work.

However, your suggestion does make it a bit harder to read (but VS coloring would help).

cc @steveharter

stephentoub · 2019-04-18T12:15:25Z

src/System.Text.Json/src/System/Text/Json/Serialization/PooledBufferWriter.cs

        {
            if (_rentedBuffer == null)
-                ThrowHelper.ThrowObjectDisposedException(nameof(ArrayBufferWriter<T>));
+                ThrowHelper.ThrowObjectDisposedException(nameof(PooledBufferWriter<T>));


When can this exception actually occur? And if it can occur and can leak out to callers, since "This is an implementation detail", do we really want the internal type's name in the exception message?

This was copy/pasted from the existing API that was going to be public. Given this is totally internal now, we don't need such checks/exceptions (and can be Debug.Asserts). We should be using it correctly, internally.

stephentoub · 2019-04-18T12:19:30Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs

            {
-                throw ThrowHelper.GetInvalidOperationException_CallFlushFirst(_buffered);
+                _arrayBufferWriter.Clear();
            }


Nit: _arrayBufferWriter?.Clear();

stephentoub · 2019-04-18T12:21:26Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs

-            _tokenType = state._tokenType;
-            _writerOptions = state._writerOptions;
-            _bitStack = state._bitStack;
+            _arrayBufferWriter = default;


Why do we clear it if we're just going to drop it?

And if we must clear it for some reason (we don't do this in most other situations where we just let the GC clean up), this line should be moved into the if block. It should also use null instead of default.

stephentoub · 2019-04-18T12:29:18Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs

+                Debug.Assert(_arrayBufferWriter.WrittenSpan.Length == _arrayBufferWriter.WrittenCount);
+                _stream.Write(_arrayBufferWriter.WrittenSpan.ToArray(), 0, _arrayBufferWriter.WrittenCount);
+#endif
+                _arrayBufferWriter.Clear();


We end up clearing the writer, which means zero'ing all bytes written, every time we flush. That seems unnecessary. Shouldn't we be able to just reset the position to 0 and not clear at all? When we're done using the buffer writer we'll just drop it and let the GC clean up.

We don't have a "reset" method on the ArrayBufferWriter that just resets the position 0. Should we make the WrittenCount property settable, or just change Clear() to Reset() which doesn't clear?

We don't have a "reset" method on the ArrayBufferWriter that just resets the position 0. Should we make the WrittenCount property settable, or just change Clear() to Reset() which doesn't clear?

That would be my preference, with it also renamed to Position 😉

joperezr · 2019-04-18T16:31:31Z

src/System.Text.Json/src/System.Text.Json.csproj

    <!-- BUILDING_INBOX_LIBRARY is only false when building for netstandard to validate that the sources are netstandard compatible. -->
    <!-- This is meant to help with producing a source package and not to ship a netstandard compatible binary. -->
    <DefineConstants Condition="'$(TargetsNETStandard)' != 'true'">$(DefineConstants);BUILDING_INBOX_LIBRARY</DefineConstants>
+    <DefineConstants Condition="'$(TargetsNETStandard)' == 'true'">$(DefineConstants);USE_ABW_INTERNALLY</DefineConstants>


If this is required for NetStandard, keep in mind that the source package won't have this constant defined, so consumers of the source package will get the netstandard sources, but won't have this constant defined.

Good point. It doesn't have any concerns here since all we are doing is going from public to internal (which the source package does anyway). But, I will invert the condition. @stephentoub provided the same feedback here: #36961 (comment)

…dotnet/corefx#36961) * Add an in-box array-backed IBufferWriter<T> * Update Utf8JsonWriter ref and main writer file. * Fix JsonWriter WriteValue APIs. * Use Environment.NewLine static and invert some stream conditions. * Update JsonWriter properties and fix serializer build breaks. * Update JsonWriter unit tests. * Add xml comments, clean up, and improve test coverage. * Update JsonDocument and JsonSerializer to react to JsonWriter changes. * Normalize the reference assembly. * Do not escape/validate comments and update issue number. * Do not escape comments and validate they don't contain embedded delimiter * Remove dead code and update issue number in comments. * Throw InvalidOEx instead of ArgEx if IBW doesn't give requested memory. * Fix test build breaks for netfx. * Remove dead code and fix source packaging. * Address PR feedback. * Disable writing floats test on windows * 8 digit floats don't work well on older TFMs. Reduce to 7. Commit migrated from dotnet/corefx@1f9b84a

ahsonkhan added 13 commits April 14, 2019 23:26

Add an in-box array-backed IBufferWriter<T>

a19d268

Update Utf8JsonWriter ref and main writer file.

b522e0c

Fix JsonWriter WriteValue APIs.

341d937

Use Environment.NewLine static and invert some stream conditions.

9caf27c

Update JsonWriter properties and fix serializer build breaks.

38a3834

Update JsonWriter unit tests.

61f51c4

Add xml comments, clean up, and improve test coverage.

1406858

Update JsonDocument and JsonSerializer to react to JsonWriter changes.

449d935

Normalize the reference assembly.

ec4d03f

Do not escape/validate comments and update issue number.

5a060d1

Do not escape comments and validate they don't contain embedded

fc5c82a

delimiter

Merge branch 'master' of https://github.com/dotnet/corefx into Redesi…

b44b6b8

…gnUtf8JsonWriter

Remove dead code and update issue number in comments.

d6213e3

ahsonkhan changed the title ~~Redesign utf8 json writer~~ Re-design Utf8JsonWriter as a regular class rather than a ref struct. Apr 17, 2019

ahsonkhan added the area-System.Text.Json label Apr 17, 2019

ahsonkhan added this to the 3.0 milestone Apr 17, 2019

Throw InvalidOEx instead of ArgEx if IBW doesn't give requested memory.

50e832c

pranavkm reviewed Apr 17, 2019

View reviewed changes

src/Common/src/System/Buffers/ArrayBufferWriter.cs Outdated Show resolved Hide resolved

Fix test build breaks for netfx.

f94ca69

steveharter reviewed Apr 17, 2019

View reviewed changes

bartonjs reviewed Apr 17, 2019

View reviewed changes

src/Common/src/System/Buffers/ArrayBufferWriter.cs Show resolved Hide resolved

src/Common/src/System/Buffers/ArrayBufferWriter.cs Outdated Show resolved Hide resolved

ahsonkhan commented Apr 17, 2019

View reviewed changes

steveharter reviewed Apr 17, 2019

View reviewed changes

ahsonkhan added 2 commits April 17, 2019 17:07

Remove dead code and fix source packaging.

6aa5efc

Merge branch 'master' of https://github.com/dotnet/corefx into Redesi…

5af4802

…gnUtf8JsonWriter

ahsonkhan commented Apr 18, 2019

View reviewed changes

ahsonkhan added 2 commits April 17, 2019 20:54

Address PR feedback.

695c9df

Disable writing floats test on windows

4cecd6a

8 digit floats don't work well on older TFMs. Reduce to 7.

4d7a90e

ahsonkhan merged commit 1f9b84a into dotnet:master Apr 18, 2019

ahsonkhan deleted the RedesignUtf8JsonWriter branch April 18, 2019 06:58

stephentoub reviewed Apr 18, 2019

View reviewed changes

joperezr reviewed Apr 18, 2019

View reviewed changes

This was referenced Apr 18, 2019

Convert ComHost tasks to use System.Text.Json instead of Newtonsoft.Json dotnet/sdk#3087

Closed

React to Utf8JsonWriter API changes and use built-in ABW dotnet/performance#437

Merged

adamsitnik mentioned this pull request Apr 19, 2019

Remove perf.groovy dotnet/performance#435

Merged

This was referenced Apr 19, 2019

Address and fix ArrayBufferWriter and Utf8JsonWriter feedback and nits #37017

Merged

Improve Utf8JsonWriter code coverage #36811

Merged

ahsonkhan mentioned this pull request Apr 26, 2019

Port System.Text.Json source comments to Docs dotnet/dotnet-api-docs#2360

Closed

qbit86 mentioned this pull request Jul 5, 2019

Utf8JsonWriter: stale documentation dotnet/dotnet-api-docs#2707

Closed

This was referenced Feb 1, 2020

Investigate writing large string values in 4 KB chunks within Utf8JsonWriter instead all at once dotnet/runtime#29293

Closed

Refactor Utf8JsonWriter code to remove code duplication dotnet/runtime#29305

Closed

Re-design Utf8JsonWriter as a regular class rather than a ref struct. #36961

Re-design Utf8JsonWriter as a regular class rather than a ref struct. #36961

Uh oh!

Conversation

ahsonkhan commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveharter Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveharter commented Apr 17, 2019

Uh oh!

ahsonkhan commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

ahsonkhan commented Apr 17, 2019 •

edited

Loading

steveharter Apr 18, 2019 •

edited

Loading

stephentoub Apr 18, 2019 •

edited

Loading

ahsonkhan commented Apr 17, 2019 •

edited

Loading

ahsonkhan commented Apr 17, 2019 •

edited

Loading

ahsonkhan Apr 17, 2019 •

edited

Loading

jkotas Apr 18, 2019 •

edited

Loading

ahsonkhan commented Apr 18, 2019 •

edited

Loading