JsonDocument Working PR #2
Conversation
At this stage ParseBoolean and ParseNestedJson pass. Others might, too.
src/System.Text.Json/src/System/Text/Json/JsonDocument.CustomDb.cs
Outdated
Show resolved
Hide resolved
src/System.Text.Json/src/System/Text/Json/JsonDocument.TryGetProperty.cs
Outdated
Show resolved
Hide resolved
| @@ -13,6 +13,49 @@ public enum JsonCommentHandling : byte | |||
| Disallow = (byte)0, | |||
| Skip = (byte)2, | |||
| } | |||
| public sealed partial class JsonDocument : System.IDisposable | |||
There was a problem hiding this comment.
Is it going to be strange for folks to need to dispose an XxDocument? Is that required for other XxDocument in the framework? Is it required for any other JSON document-like types in other JSON libraries?
There was a problem hiding this comment.
Usually the DOM APIs aren't IDisposable, but they have readers that are. So "no, but nearly". It seems like a natural consequence of the performance goals.
If it's "too strange for the surface area" then the next option that I see is to use generic List; the consequence of not disposing in the current model being that the document "stole" the array from the pool rather than rented it.
src/System.Text.Json/src/System/Text/Json/JsonDocument.TryGetProperty.cs
Show resolved
Hide resolved
|
|
||
| int tokenStart = Unsafe.ByteOffset( | ||
| ref jsonStart, | ||
| ref MemoryMarshal.GetReference(reader.ValueSpan)).ToInt32(); |
There was a problem hiding this comment.
Utf8JsonReader doesn't provide this information? If not, is that a flaw in the Utf8JsonReader API, that JsonDocument needs to resort to such tactics to get data Utf8JsonReader doesn't provide?
There was a problem hiding this comment.
The prototype JsonReader (as implemented in corefxlab) had an internal "token start" field which tracked this. This was removed when we moved to corefx since (along with "inArray") since we didn't have a JsonDocument. We could consider adding it back.
| arrayItemsCount = row.SizeOrLength; | ||
| numberOfRowsForValues += row.NumberOfRows; | ||
| } | ||
| else if (tokenType == JsonTokenType.PropertyName) |
There was a problem hiding this comment.
Are these ordered from most common to least common? Since the further you get here the more checks you need to do as part of parsing, it might be interesting to gather some stats on frequency on common documents and order accordingly. Or maybe it doesn't matter.
| { | ||
| if (_parent == null) | ||
| { | ||
| throw new InvalidOperationException(); |
There was a problem hiding this comment.
Do we gain anything perf-wise if we were to move this out with the ThrowHelper pattern?
Also, I assume error messages will be added later?
There was a problem hiding this comment.
I moved all of these checks "am I default(JsonElement)?" to a helper CheckValidInstance, and all of the disposed checks in JsonDocument to CheckNotDisposed and all of the "am I the right type?" checks in JsonDocument to CheckExpectedType... and the performance is either unchanged or worse. (within overlapping sigmas, but the "after" had a higher mean when the JsonObject baseline got a lower mean).
But I guess those changes are fine, especially if you think we want to be more helpful on "don't call members on the default JsonElement" case by using a non-default string.
Or maybe I don't understand what the actual ThrowHelper pattern is.
There was a problem hiding this comment.
Did you try a throw helper after adding a message string? @benaadams previously saw a benefit (at least in asm) here: dotnet/corefxlab#2366 (comment)
Also, from @AndyAyersMS,
dotnet#33216 (comment)
Methods that conditionally throw exceptions on some paths can still be inlined, as can methods with complex flow where every path ultimately throws.
Simple methods that unconditionally throw won't be inlined w/o AggressiveInlining.
The benefits of the throw helper pattern are that it can save code size if the same exception can be thrown from multiple places within a method or across multiple methods, and that it also may reduce some of the time and space overhead the throwing path may impose on non-throwing paths in a method (say the method prolog).
Using a throw helper for just one site in one method may not provide any benefit -- you'd have to look at the non-exceptional codegen paths with and without a helper to see.
Run the ParseJson tests for all the entrypoints. Some caching, test perf fixes, and some test case reduction took a 23 second run down to 7.
* Json prototype (#1) Basic API for Json writable DOM with scenarios including collection initialization, accessing and modifying Json nodes. * Json prototype - transformation API (#2) * transformation API added * assertions to existing scenarios added * Json prototype (#1) Basic API for Json writable DOM with scenarios including collection initialization, accessing and modifying Json nodes. * Json prototype - transformation API (#2) * transformation API added * assertions to existing scenarios added * JsonNumber implementation and tests (dotnet#3) * IEquatables added * JsonNumber implementation and unit tests added * pragmas deleted * review comments included, more tests added * decimal support added to JsonNumber * decimal support added to JsonArray and JsonObject * all unimplemented classes and methods with accompanying tests removed * First part of documentation added * documentation completed * missing exceptions added * JsonElement changes removed * part of the review comments included * work on review comments * code refactor * more decimal tests added using MemberData * more decimal tests added using MemberData * more test cases added * equals summary adjusted, equals tests added * more Equals tests added, GetHashCode tests added, minor changes * scientifing notation support added, rational numbers tests fixes * rational overflow tests added * ulong maxvalue tests added to rational types * presision problems fixes * exception strings fixed * CI failing fixes (hopefully), review comments included * missing == tests added to achieve 100% branch coverage * review comments included * trailing whitespaces fixes * equals comments added * equals object refactored to call quals json number * merge fix
Work-In-Progress PR for unified working branch commenting