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

STJ and Frozen collections #82642

Open
Tracked by #107787
TonyValenti opened this issue Feb 24, 2023 · 13 comments
Open
Tracked by #107787

STJ and Frozen collections #82642

TonyValenti opened this issue Feb 24, 2023 · 13 comments
Labels
area-System.Text.Json wishlist Issue we would like to prioritize, but we can't commit we will get to it yet
Milestone

Comments

@TonyValenti
Copy link

Will STJ be updated to support them?

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 24, 2023
@ghost
Copy link

ghost commented Feb 24, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Will STJ be updated to support them?

Author: TonyValenti
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

We could, other immutable collections are already supported by STJ. One small concern I have is that whatever baked-in converters we do ship need to make an explicit choice as to whether the optimizeForReading flag is enabled when deserializing; whatever we do end up picking won't be user-configurable. @stephentoub might have opinions about this.

Serialization for frozen collections should already be possible (since they all implement the right interfaces), it's deserialization support that needs to be added. One possible course of action is that we do nothing and simply expose support for parameterized constructors in the contract model: frozen collections are a prime example of where such functionality would be useful.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Feb 27, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Feb 27, 2023
@eiriktsarpalis eiriktsarpalis added the wishlist Issue we would like to prioritize, but we can't commit we will get to it yet label Feb 27, 2023
@stephentoub
Copy link
Member

One small concern I have is that whatever baked-in converters we do ship need to make an explicit choice as to whether the optimizeForReading flag is enabled when deserializing; whatever we do end up picking won't be user-configurable. @stephentoub might have opinions about this.

I'd want to better understand the user scenario for serializing/deserializing these as json. The intent of optimizeForReading:true is for collections expected to be created once and then used repeatedly for a very long time, as creating them is more expensive, with the idea that spending more time at construction is a good tradeoff if there's going to be many, many subsequent reads. If these collections are being routinely serialized back and forth as part of some communication scheme, I'd venture it'd be rare for them to them stick around for a very long time. And if the odd one was, a new frozen collection could be created from it with the appropriate settings. It'd likely be cheaper to just deserialize these as a standard Dictionary<>.

@eiriktsarpalis
Copy link
Member

Agreed. Generally speaking I'd expect collections on deserialized DTOs to be ephemeral in most cases so frozen collections aren't necessarily the ideal representation for doing that. At the same time users expect serialization to "just work" with the type at hand so perhaps having a default that optimizes for deserialization would be good enough for most use cases.

@TonyValenti
Copy link
Author

In my organization, we use records and Immutable structures for all JSON-based communication. We often load JSON into an in-memory ImmutableDictionary cache and then access it from there.

From what I was reading, it seemed like FrozenDictionary would give us better performance which is why I asked.

@eiriktsarpalis
Copy link
Member

Apropos, any new collection type that we support would also need to work in the source generator, currently this means adding new dedicated APIs in the JsonMetadataServices class. This suggests to me that tackling #71944 might let us kill two birds with one stone.

@terrajobst
Copy link
Member

terrajobst commented Aug 20, 2024

In my case I have an immutable data structure that acts like a cache. It's constructed once and sticks around as long as the process exists. However, I want to save it to disk and reload it the next time, which is why it would be convenient if I could use frozen collections as this means I can serialize the cache as-is.

@julealgon
Copy link

julealgon commented Aug 20, 2024

@stephentoub

If these collections are being routinely serialized back and forth as part of some communication scheme, I'd venture it'd be rare for them to them stick around for a very long time.

I do have an upcoming scenario (for a game) where I want to have parts of the game be data-driven and obtained via an API request during initial load. The results of this call will not be updated for the remainder of the session, which could be minutes, hours, or days: basically unbounded.

It's not a very common scenario I admit though.

And if the odd one was, a new frozen collection could be created from it with the appropriate settings. It'd likely be cheaper to just deserialize these as a standard Dictionary<>.

That's probably fair enough though, and likely what I would do in the scenario above if deserialization wasn't supported.

@stephentoub
Copy link
Member

simply expose support for #71944 in the contract model: frozen collections are a prime example of where such functionality would be useful

@eiriktsarpalis, should we support collection expression builders? FrozenSet has one, and FrozenDictionary will once dictionary expressions are a thing.
cc: @CyrusNajmabadi

@TonyValenti
Copy link
Author

I would love it if you did.

@eiriktsarpalis
Copy link
Member

I think we should.

@stephentoub stephentoub modified the milestones: Future, 10.0.0 Aug 31, 2024
@eiriktsarpalis
Copy link
Member

One potential concern is the serializer handling types differently depending on whether the target uses CollectionBuilderAttribute. We should make sure types using the attribute are already supported if they exist in ns2.0

@codymullins
Copy link

With regards to @eiriktsarpalis's comment:

I'd expect collections on deserialized DTOs to be ephemeral in most cases so frozen collections aren't necessarily the ideal representation

I am running into this when serializing state to persist on page reloads or sharing links. I don't really need a separate dto if not for this -- the code is already client side via wasm. So just fyi, DTO serialization may not be the only use case here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json wishlist Issue we would like to prioritize, but we can't commit we will get to it yet
Projects
None yet
Development

No branches or pull requests

6 participants