Avoid closures in JsonPropertyInfoOfT/JsonTypeInfoOfT
#117498
Conversation
The lambda capture of the local variable seems avoidable in these places.
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how factory and accessor delegates are set in JsonTypeInfoOfT and JsonPropertyInfoOfT to eliminate lambda closures over local variables and instead reference instance fields directly, reducing allocations.
- In
JsonTypeInfoOfT.SetCreateObject, the untyped and typed object-creation lambdas now call the stored fields rather than capturing the local delegate. - In
JsonPropertyInfoOfT, the getter, setter, and should-serialize delegates are similarly rewritten to reference_typedGet/_untypedGet,_typedSet/_untypedSet, and_shouldSerializeTyped/_shouldSerializefields. - Overall change improves performance by avoiding unnecessary closure allocations.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Text.Json/.../JsonTypeInfoOfT.cs | Updated SetCreateObject to use instance fields in lambdas instead of local captures. |
| src/libraries/System.Text.Json/.../JsonPropertyInfoOfT.cs | Updated SetGetter, SetSetter, and SetShouldSerialize to use fields, avoiding closures. |
| } | ||
| else | ||
| { | ||
| Func<object, object?> untypedGet = (Func<object, object?>)getter; | ||
| _typedGet = (obj => (T)untypedGet(obj)!); | ||
| _typedGet = (obj => (T)_untypedGet!(obj)!); | ||
| _untypedGet = untypedGet; |
There was a problem hiding this comment.
Nit: it doesn't actually matter, because the delegate won't be invoked yet, but could we switch these two lines so that _untypedGet is set before it's referenced? I find it a bit easier to reason about. (You might also be able to remove the !, I'm not sure)
There was a problem hiding this comment.
Hypothetically, if I called the getter to store a copy of the existing delegate, but then set a new delegate via the setter, my copy would start pointing to the new behaviour. I think that makes a subtle difference that could regress code that is heavy on contract customization.
There was a problem hiding this comment.
Hypothetically, if I called the getter to store a copy of the existing delegate, but then set a new delegate via the setter, my copy would start pointing to the new behaviour. I think that makes a subtle difference that could regress code that is heavy on contract customization.
This is in reference to the PR, not to my nit, yes?
There was a problem hiding this comment.
Does this mean I should close this?
I don't mind either way. This is a drop in the bucket. I'm looking at a relatively big app (40 MB) where 15% of the app is System.Text.Json because of how many generics S.T.Json will create for every type used with Json. This looked like something we could do something about, the rest I don't know how to fix.
There was a problem hiding this comment.
Does this mean I should close this?
Yeah I think so. It doesn't change things much either way so I would err towards preserving the current behaviour.
| } | ||
| else | ||
| { | ||
| Action<object, object?> untypedSet = (Action<object, object?>)setter; | ||
| _typedSet = ((obj, value) => untypedSet(obj, value)); | ||
| _typedSet = ((obj, value) => _untypedSet!(obj, value)); | ||
| _untypedSet = untypedSet; |
| } | ||
| else | ||
| { | ||
| Func<object, object?, bool> untypedPredicate = (Func<object, object?, bool>)predicate; | ||
| _shouldSerializeTyped = (obj, value) => untypedPredicate(obj, value); | ||
| _shouldSerializeTyped = (obj, value) => _shouldSerialize!(obj, value); | ||
| _shouldSerialize = untypedPredicate; |
The lambda capture of the local variable seems avoidable in these places.