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

Cosmos: Allow to use a custom JSON serializer #17306

Open
AndriySvyryd opened this issue Aug 20, 2019 · 17 comments
Open

Cosmos: Allow to use a custom JSON serializer #17306

AndriySvyryd opened this issue Aug 20, 2019 · 17 comments
Assignees
Labels
area-cosmos consider-for-current-release punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. type-enhancement
Milestone

Comments

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Aug 20, 2019

Allow value converters to output a raw object (e.g. JToken)

Also provide access to unmapped properties (through a JSON shadow property or dynamic property bag).

Make sure that FromSqlRaw parameters are properly serialized.

Test with STJ source generators

Related to #14570, #30677

@ajcvickers
Copy link
Member

Note from triage: this is currently blocked on the new Cosmos SDK, tracked by #18753.

@bjorn-ali-goransson
Copy link

bjorn-ali-goransson commented Nov 20, 2021

Yes please! This way I will be able to serialize an IList<ISomeCustomInterface> - there and back again ...

@vyarymovych

This comment has been minimized.

@roji

This comment has been minimized.

@vyarymovych

This comment has been minimized.

@roji

This comment has been minimized.

@ajcvickers ajcvickers added punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. and removed propose-punt labels Jul 7, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0, Backlog Jul 7, 2022
@athinton
Copy link

If I understand this correctly, PR #24334 was not merged because a better solution is to upgrade to a newer version of the SDK i.e. #18753. Unfortunately, the comments on this issue indicate that V4 has been abandoned.

Is my understanding correct? What does this mean for the possibility of customising the serialisation options?

@AndriySvyryd
Copy link
Member Author

We will add some way of customizing the serializer, but for now it's still undecided how that will look.

@klemmchr
Copy link

Is there any progress on this? Would love to use System.Text.Json to reduce the memory footprint. Newtonsoft.Json is hitting really hard.

@ajcvickers
Copy link
Member

@klemmchr Make sure to vote (👍) for the issue. The more votes, the higher priority is gets. This issue currently has 16 votes, which means 193 other issues have more votes and are above it in the priority list.

@klemmchr
Copy link

So prioritization is solely based on some upvotes?
You're currently forcing users to use a third party dependency with horrible performance while you're having a Microsoft based serializer in place. It's really not understandable how this situation is accepted.

@ajcvickers
Copy link
Member

So prioritization is solely based on some upvotes?

No, it's one input into the planning process.

@roji
Copy link
Member

roji commented Jan 21, 2024

@klemmchr note that Newtonsoft.Json is a dependency of the Cosmos SDK itself, so not something EF itself controls. I also doubt that the difference between Newtonsoft.Json vs. System.Text.Json would result in "horrible performance" specifically when working against Cosmos - is that statement backed by actual benchmarking?

@klemmchr
Copy link

klemmchr commented Jan 21, 2024

note that Newtonsoft.Json is a dependency of the Cosmos SDK itself, so not something EF itself controls.

The Cosmos SDK does not force me to utilize Newtonsoft.Json. I can plug in any deserializer I want to. Plus, I can retrieve data as a stream directly.

I also doubt that the difference between Newtonsoft.Json vs. System.Text.Json would result in "horrible performance" specifically when working against Cosmos - is that statement backed by actual benchmarking?

There are tons of benchmarks out there. Microsoft is bragging about their performance gain compared to Newtonsoft due to new API that is used, reducing both compute time and memory allocation. The quoted benchmark is five years old, the gap only got bigger since.
I did a memory analysis on one of our applications and did find out that de-/serialization takes up tons of memory using Newtonsoft. That footprint would be way lower when I could use a different serializer.

@roji
Copy link
Member

roji commented Jan 22, 2024

There are tons of benchmarks out there.

My comment above was whether the performance difference here would be actually meaninful when working with Cosmos specifically, given that you're transferring data over the network to a cloud service that's performing I/O to search and execute queries. I'd recommend avoiding assumptions about perf without actually checking.

@klemmchr
Copy link

klemmchr commented Jan 22, 2024

You're confusing latency with resource utilization. I'm explicitly talking about CPU and RAM usage and not about latency issues (that are common for all cloud services). I'm not making assumptions, I'm citing benchmarks done by Microsoft. There are tons of other benchmarks out there too. I don't see where this is going given that you're just ignoring the reason why Newtonsoft is bad as a hardcoded serializer.

I just don't get this argument. You're basically saying that all other performance factors are completely irrelevant once I/O and network is involved. This would mean that we don't need any more performance improvements for ASP.NET or Microsoft.Data.SqlClient. This is just weird to argue about.

@roji
Copy link
Member

roji commented Jan 22, 2024

@klemmchr I did not mention latency vs. resource utilization - the question is whether the resource utilization you're referring to, e.g. the extra allocations, are meaningful to the overall performance specifically when interacting with Cosmos.

In other words, you may very well find out that switching from Newtonsoft.Json to System.Text.Json considerably reduces memory allocations, but does nothing to your general application performance, since that improvement is negligible compared to everything else that has to happen to service a Cosmos request. If that's the case, then spending time on this feature for performance reasons isn't worth it (it may be worth it for other reasons, e.g. functionality differences between the two serializers).

I just don't get this argument. You're basically saying that all other performance factors are completely irrelevant once I/O and network is involved. This would mean that we don't need any more performance improvements for ASP.NET or Microsoft.Data.SqlClient.

I'm not. It's good to be concerned with performance, but it's important to always have a holistic view of performance when optimizing things - especially when considering priority (i.e. where we invest our time). If reducing a few allocations in a path where those are negligible doesn't actually get you anything, then that's not something we should prioritize. Otherwise we can spend a lot of time on meaningless micro-optimizations which technically improve performance, but not in any way that matters to anyone. I hope that's clear.

To summarize, I think this is something we should do in general, but I'd be surprised if it would lead to a meaningful improvement perf-wise when working with Cosmos. I'd be happy to see actual figures to the contrary though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-cosmos consider-for-current-release punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. type-enhancement
Projects
None yet
Development

No branches or pull requests

8 participants