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.

Sign up for GitHub

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

Jump to bottom

Prototype moving JsonData to be backed by JsonDocument #33063

Merged
merged 107 commits into from
Feb 2, 2023
Merged

Prototype moving JsonData to be backed by JsonDocument #33063

merged 107 commits into from
Feb 2, 2023

Conversation

annelo-msft
Copy link
Member

@annelo-msft annelo-msft commented Dec 15, 2022 •
edited
Loading

Summary

In this PR, "JsonData" becomes a collection of types:

  • MutableJsonDocument, a wrapper around JsonDocument that enables mutations to the JSON
  • MutableJsonElement, the parallel of JsonElement
  • DynamicJson, a dynamic layer around MutableJsonDocument and MutableJsonElement
  • DynamicData, an abstract class proving the interface for DynamicJson, and future types like DynamicXml, etc.

Functional features and performance improvements remain to be added, and are tracked in #33856.

For details of the currently implemented API, please see the api changes file.




Prior PR Description

Allocating many Dictionaries to store a JsonData object for each node in the tree is very inefficient.

This prototype explores what it would look like to have each JsonData backed by a JsonElement instead.

Mutations will be tracked in a "Patch List" such as the one described in Mutable "No" Alloc JsonDocument (github.com)

Initial Perf Benchmarks (Prior to changes)

ParseAndReadLargePayloadBenchmark

|          Method |     Mean |    Error |   StdDev |   Median | Ratio | RatioSD |  Gen 0 | Allocated |
|---------------- |---------:|---------:|---------:|---------:|------:|--------:|-------:|----------:|
| ReadJsonElement | 11.05 us | 0.376 us | 1.079 us | 10.73 us |  1.00 |    0.00 | 1.0071 |      4 KB |
|    ReadJsonData | 20.56 us | 0.402 us | 0.494 us | 20.52 us |  1.99 |    0.15 | 4.2114 |     17 KB |


ReadLargePayloadBenchmark

|          Method |      Mean |    Error |   StdDev | Ratio | RatioSD |  Gen 0 | Allocated |
|---------------- |----------:|---------:|---------:|------:|--------:|-------:|----------:|
| ReadJsonElement | 131.72 ns | 2.439 ns | 3.338 ns |  1.00 |    0.00 |      - |         - |
|    ReadJsonData |  85.29 ns | 1.374 ns | 1.285 ns |  0.65 |    0.02 | 0.0114 |      48 B |

@ghost ghost added the Azure.Core label Dec 15, 2022
@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Core.Experimental

@annelo-msft annelo-msft marked this pull request as ready for review February 2, 2023 16:21
@annelo-msft annelo-msft mentioned this pull request Feb 2, 2023
Closed
41 tasks
@annelo-msft
Copy link
Member Author

/azp run net - core - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@annelo-msft annelo-msft changed the title Prototype moving JsonData to be backed by JsonElement Prototype moving JsonData to be backed by JsonDocument Feb 2, 2023
@annelo-msft annelo-msft merged commit e7dea2a into Azure:main Feb 2, 2023
richardcho-msft pushed a commit to richardcho-msft/azure-sdk-for-net that referenced this pull request Feb 6, 2023
@annelo-msft @richardcho-msft
Prototype moving JsonData to be backed by JsonDocument (Azure#33063)
578adcd 
* Add WriteTo(), ==operators, and PR FB

* export API

* Add DocumentSentiment large JSON sample and add parse and read benchmarks

* initial attempt to move to JsonElement

* some fixes

* updates

* nit

* nits

* starting to experiment with changelist approach

* an approach to object assignment

* start working with array elements

* saving changes from before break...

* sm formatting pr fb

* ApiView FB per generality of DynamicData

* nits

* small thoughts

* first steps toward abstracting ChangeTracker

* In flight changes while implementing AddPropertyToObject

* adding tests with simple modifications to structural elements

* Implementing WriteTo()

* Added nested objects

* refactor

* missed changes

* In TDD spirit, add failing WriteTo test

* Test passes

* quick refactor

* update add property test

* Update WriteTo to handle property additions at the root element

* Handle property additions on arbitrary objects

* handle standard property removals

* Support Replace with object

* Support Replace with object

* refactor where we serialize structural changes to centralize

* Implement reference semantics for JsonDataElement

* experiment with checking ancestors for structural changes.

* Update WriteTo to handle structural changes.

* add high water mark logic

* add validation to all reads and add failing test for ignoring pre-structural change.

* Incorporate HWM logic in all change lookups; make PriorChangeToReplacedPropertyIsIgnored

* remove double-check of object and array elements

* some tidy up

* Reimplement GetProperty in terms of TryGetProperty()

* Handle WriteTo for structural changes.

* add some tests of structural changes

* refactor tests

* Bug fix to WriteTo for bools and test refactoring

* bug fix to WriteTo for booleans

* Add support for nulls and fix ToLower() bug

* add perf benchmark prior to working on perf

* missed file

* update Parse() to use Memory<byte>

* move subclasses to separate files for ease of reading

* refactor to add dynamic layer

* Add cast operators to JsonDataElement to pass dynamic GetIntProperty test

* Add support for dynamic nested property access

* enable set via dynamic layer

* Enable setting nested properties via dynamic layer

* nit

* Renames prior to dynamic refactor

* start adding separate dynamic layer

* Move dynamic meta object to dyanmic types.

* API updates

* todos and nits

* rename test to match types

* remove outer DynamicJson class

* add BindConvert to dynamic layer

* Add BindGetIndex to dynamic layer

* save multitarget attempt

* Add target frameworks to Experimental to allow ifdefs by target framework.

* PR feedback; use more efficient Deserialize call when available in ConvertTo()

* refactor per PR fb

* Add BindSetIndex to dynamic layer

* Fix bug in mutable ToString() where changes to descendants weren't accounted for

* Fix WriteTo() bug for string elements and add failing test for handling nulls

* Handle null values

* PR fb

* Add support for floats and longs

* remove HasValue, per pr fb

* export API changes

* Support adding properties on dynamic member assignments

* reshuffle methods around

* some changes before adding support for IEnumerable

* Add ArrayEnumerator to MutableJsonElement

* missed file; dynamic portion not complete

* Bug fix to dynamic ArrayEnumerator; foreach over array now passes

* export API and misc test updates

* nit; cleanup

* Make MutableJsonDocument serializable

* Make DynamicJson serializable; enable reference semantics for added properties

* Make tests pass for net6.0 & net7.0; will address net461 separately

* Fixes for some net461 issues

* Fix Add and Set property for net461

* Work around BindBinaryOperation in net461; move inline TODOs to GH issue

* Export API
@annelo-msft annelo-msft mentioned this pull request Feb 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KrzysztofCwalina KrzysztofCwalina KrzysztofCwalina approved these changes

@jsquire jsquire jsquire left review comments

@ellismg ellismg Awaiting requested review from ellismg

@tg-msft tg-msft Awaiting requested review from tg-msft

@JoshLove-msft JoshLove-msft Awaiting requested review from JoshLove-msft

@christothes christothes Awaiting requested review from christothes

Assignees
No one assigned
Labels
Azure.Core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@annelo-msft @azure-sdk @jsquire @KrzysztofCwalina