Lazily instantiate JsonSerializerOptions.Converters #77219
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue Detailsnull
|
| { | ||
| int n; | ||
| if ((n = left.Count) != right.Count) | ||
| int leftCount = left is null ? 0 : left.Count; |
There was a problem hiding this comment.
I'm curious - when converters are not null - what's the chance that someone cached converter but didn't cache options? Won't this converters check realistically always be false?
There was a problem hiding this comment.
Possibly we could add equals and hash code to our implementations of converters and make them return trues when types match? That would make it relatively more likely to be true
There was a problem hiding this comment.
Won't this converters check realistically always be false?
It would almost always be false, I wouldn't expect users to cache converters if they're not caching options.
Possibly we could add equals and hash code to our implementations of converters and make them return trues when types match? That would make it relatively more likely to be true
It might also be false because some converters accept parameters. We want to avoid false positives under all circumstances since it would lead to observable bugs -- on the other hand we tolerate false negatives since they can only result in reduced performance.
| /// Once serialization or deserialization occurs, the list cannot be modified. | ||
| /// </remarks> | ||
| public IList<JsonConverter> Converters => _converters; | ||
| public IList<JsonConverter> Converters => _converters ??= new ConverterList(this); |
There was a problem hiding this comment.
This isn't thread-safe, in that two threads concurrently accessing this could end up getting different ConverterList instances. Is that ok?
And, it seems like this could transition from null to non-null even after being marked as isreadonly. Is that ok?
There was a problem hiding this comment.
It's ok for both cases, all logic consuming that field (including equality comparison in the global cache) has been updated so that null instances are equated to empty instances.
There was a problem hiding this comment.
It can, of course, result in issues when multiple threads attempt to configure the same mutable instance, but generally speaking the type is not designed to be thread safe while mutable.
ghost
locked as resolved and limited conversation to collaborators
3 participants
No description provided.