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

Query/Json: use Utf8JsonReader in materializer #31159

Closed
maumar opened this issue Jun 29, 2023 · 1 comment
Closed

Query/Json: use Utf8JsonReader in materializer #31159

maumar opened this issue Jun 29, 2023 · 1 comment
Labels
closed-no-further-action The issue is closed and no further action is planned.

Comments

@maumar
Copy link
Contributor

maumar commented Jun 29, 2023

part of #30604 (serialization will be done separately)

using utf8 json reader to deserialize json into POCOs, rather than using DOM. Need to modify the shaper to accommodate for the fact that we no longer have random access to the data that we need, but rather data is "streamed" from JSON string. This is somewhat problematic when entity takes arguments in the constructor. Those arguments can be at the end of json string, so we must read all the relevant data, store it in variables, and only then instantiate the object, set all the properties and do the fixup.

Optimized/simplified case will be added in later - when there is no ctor arguments we can just instantiate the entity right away and set the properties as we read them - biggest benefit comes from the fact that we don't need to store navigations (fewer allocations) and fixup can be done in place

maumar added a commit that referenced this issue Jun 29, 2023
Using Utf8JsonReader to read JSON data rather than caching it using DOM. This should reduce allocations significantly. Tricky part is that entity materializers are build in a way that assumes we have random access to all the data we need. This is not the case here.
We read JSON data sequentially and can only do it once, and we don't know the order in which we get the data. This is somewhat problematic in case where entity takes argument in the constructor. Those could be at the very end of the JSON string, so we must read all the data before we can instantiate the object, and populate it's properties and do navigation fixup.
This requires us reading all the JSON data, store them in local variables, and only when we are done reading we instantiate the entity and populate all the properties with data stored in those variables. This adds some allocations (specifically navigations).

We also have to disable de-duplication logic - we can't always safely re-read the JSON string, and definitely can't start reading it from arbitrary position, so now we have to add JSON string for every aggregate projected, even if we already project it's parent.

Also fix to #30993 - Query/Json: data corruption for tracking queries with nested json entities, then updating nested entities outside EF and re-querying

Fix is to recognize and modify shaper in case of tracking query, so that nav expansions are not skipped when parent entity is found in Change Tracker. This is necessary to fix alongside streaming, because now we throw exception from reader (unexpected token) if we don't process the entire stream correctly. Before it would be silently ignored apart from the edge case described in the bug.

Fixes #31159
Fixes #30993
maumar added a commit that referenced this issue Jun 29, 2023
Using Utf8JsonReader to read JSON data rather than caching it using DOM. This should reduce allocations significantly. Tricky part is that entity materializers are build in a way that assumes we have random access to all the data we need. This is not the case here.
We read JSON data sequentially and can only do it once, and we don't know the order in which we get the data. This is somewhat problematic in case where entity takes argument in the constructor. Those could be at the very end of the JSON string, so we must read all the data before we can instantiate the object, and populate it's properties and do navigation fixup.
This requires us reading all the JSON data, store them in local variables, and only when we are done reading we instantiate the entity and populate all the properties with data stored in those variables. This adds some allocations (specifically navigations).

We also have to disable de-duplication logic - we can't always safely re-read the JSON string, and definitely can't start reading it from arbitrary position, so now we have to add JSON string for every aggregate projected, even if we already project it's parent.

Also fix to #30993 - Query/Json: data corruption for tracking queries with nested json entities, then updating nested entities outside EF and re-querying

Fix is to recognize and modify shaper in case of tracking query, so that nav expansions are not skipped when parent entity is found in Change Tracker. This is necessary to fix alongside streaming, because now we throw exception from reader (unexpected token) if we don't process the entire stream correctly. Before it would be silently ignored apart from the edge case described in the bug.

Fixes #31159
Fixes #30993
maumar added a commit that referenced this issue Jun 30, 2023
Using Utf8JsonReader to read JSON data rather than caching it using DOM. This should reduce allocations significantly. Tricky part is that entity materializers are build in a way that assumes we have random access to all the data we need. This is not the case here.
We read JSON data sequentially and can only do it once, and we don't know the order in which we get the data. This is somewhat problematic in case where entity takes argument in the constructor. Those could be at the very end of the JSON string, so we must read all the data before we can instantiate the object, and populate it's properties and do navigation fixup.
This requires us reading all the JSON data, store them in local variables, and only when we are done reading we instantiate the entity and populate all the properties with data stored in those variables. This adds some allocations (specifically navigations).

We also have to disable de-duplication logic - we can't always safely re-read the JSON string, and definitely can't start reading it from arbitrary position, so now we have to add JSON string for every aggregate projected, even if we already project it's parent.

Also fix to #30993 - Query/Json: data corruption for tracking queries with nested json entities, then updating nested entities outside EF and re-querying

Fix is to recognize and modify shaper in case of tracking query, so that nav expansions are not skipped when parent entity is found in Change Tracker. This is necessary to fix alongside streaming, because now we throw exception from reader (unexpected token) if we don't process the entire stream correctly. Before it would be silently ignored apart from the edge case described in the bug.

Fixes #31159
Fixes #30993
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 30, 2023
@maumar maumar added this to the 8.0.0 milestone Jun 30, 2023
@maumar maumar self-assigned this Jun 30, 2023
@maumar
Copy link
Contributor Author

maumar commented Jul 11, 2023

ended up fixing #30604 all at once, closing

@maumar maumar closed this as completed Jul 11, 2023
@maumar maumar added the closed-no-further-action The issue is closed and no further action is planned. label Jul 11, 2023
@maumar maumar removed this from the 8.0.0 milestone Jul 11, 2023
@ajcvickers ajcvickers added this to the 8.0.0 milestone Oct 11, 2023
@ajcvickers ajcvickers removed type-enhancement area-perf closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. area-query area-json labels Oct 11, 2023
@ajcvickers ajcvickers removed this from the 8.0.0 milestone Oct 11, 2023
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned.
Projects
None yet
Development

No branches or pull requests

2 participants