Skip to content

Remove unneeded virtual calls & branching when accessing Get & Set delegates #7

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.

Sign up for GitHub

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

Merged
krwq merged 1 commit into from
Jun 10, 2022
Merged

Remove unneeded virtual calls & branching when accessing Get & Set delegates #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 12 additions & 17 deletions ...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,29 +53,24 @@ public JsonConverter? CustomConverter
/// </summary>
public Func<object, object?>? Get
{
get => UntypedGetValue;
set
{
CheckMutable();
UntypedGetValue = value;
}
get => _untypedGet;
set => SetGetter(value);
}

/// <summary>
/// Setter delegate. Property cannot be deserialized without it.
/// </summary>
public Action<object, object?>? Set
{
get => UntypedSetValue;
set
{
CheckMutable();
UntypedSetValue = value;
}
get => _untypedSet;
set => SetSetter(value);
}

private protected abstract Func<object, object?>? UntypedGetValue { get; set; }
private protected abstract Action<object, object?>? UntypedSetValue { get; set; }
private protected Func<object, object?>? _untypedGet;
private protected Action<object, object?>? _untypedSet;

private protected abstract void SetGetter(Delegate? getter);
private protected abstract void SetSetter(Delegate? setter);

/// <summary>
/// Decides if property with given declaring object and property value should be serialized.
Expand Down Expand Up @@ -136,7 +131,7 @@ internal static JsonPropertyInfo CreateIgnoredPropertyPlaceholder(
/// </summary>
public Type PropertyType { get; private protected set; } = null!;

private void CheckMutable()
private protected void CheckMutable()
{
if (_isConfigured)
{
Expand Down Expand Up @@ -443,8 +438,8 @@ internal string GetDebugInfo(int indent = 0)
}
#endif

internal bool HasGetter { get; set; }
internal bool HasSetter { get; set; }
internal bool HasGetter => _untypedGet is not null;
Copy link
Owner

@krwq krwq Jun 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be true also when property is for type info (TypeInfo.PropertyInfoForTypeInfo). Are tests passing like this?

Copy link
Collaborator Author

@eiriktsarpalis eiriktsarpalis Jun 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, all green. Why does it need to say HasGetter? Also, is PropertyInfoForTypeInfo still needed? (I figure it was necessary to access the converter, but we currently surface it in JsonTypeInfo itself)

Copy link
Owner

@krwq krwq Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess me must have simplified something which makes it not needed anymore. I initially implemented HasGetter similar way several weeks back and was getting some failures because of that

Copy link
Owner

@krwq krwq Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PropertyInfoForTypeInfo is a bit of a beast to get rid of, we will need quite a bit of refactoring

internal bool HasSetter => _untypedSet is not null;

internal abstract void Initialize(
Type parentClassType,
Expand Down
105 changes: 59 additions & 46 deletions ...aries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,45 +27,66 @@ internal sealed class JsonPropertyInfo<T> : JsonPropertyInfo
// the property's type, we track that and whether the property type can be null.
private bool _propertyTypeEqualsTypeToConvert;

private Func<object, object?>? _untypedGet;
private Action<object, object?>? _untypedSet;
private Func<object, T>? _typedGet;
private Action<object, T>? _typedSet;

// We do not need to worry about invalidating _untypedGet/_untypedSet
// because these are only set during construction
// If these ever became public we'd need to ensure respective value is set to null here
private Func<object, T>? TypedGetValue { get; set; }
internal new Func<object, T>? Get
{
get => _typedGet;
set => SetGetter(value);
}

private Action<object, T>? TypedSetValue { get; set; }
internal new Action<object, T>? Set
{
get => _typedSet;
set => SetSetter(value);
}

private protected override Func<object, object?>? UntypedGetValue
private protected override void SetGetter(Delegate? getter)
{
get
Debug.Assert(getter is null or Func<object, object?> or Func<object, T>);

CheckMutable();

if (getter is null)
{
// We use local here so that we don't capture 'this'
Func<object, T>? typedGetValue = TypedGetValue;
return _untypedGet ??= typedGetValue == null ? null : (o) => typedGetValue(o);
_typedGet = null;
_untypedGet = null;
}
set
else if (getter is Func<object, T> typedGetter)
{
_typedGet = typedGetter;
_untypedGet = getter is Func<object, object?> untypedGet ? untypedGet : obj => typedGetter(obj);
}
else
{
_untypedGet = value;
TypedGetValue = value == null ? null : (o) => (T)value(o)!;
HasGetter = value != null;
Func<object, object?> untypedGet = (Func<object, object?>)getter;
_typedGet = (obj => (T)untypedGet(obj)!);
_untypedGet = untypedGet;
}
}

private protected override Action<object, object?>? UntypedSetValue
private protected override void SetSetter(Delegate? setter)
{
get
Debug.Assert(setter is null or Action<object, object?> or Action<object, T>);

CheckMutable();

if (setter is null)
{
// We use local here so that we don't capture 'this'
Action<object, T>? typedSetValue = TypedSetValue;
return _untypedSet ??= typedSetValue == null ? null : (o, v) => typedSetValue(o, (T)v!);
_typedSet = null;
_untypedSet = null;
}
set
else if (setter is Action<object, T> typedSetter)
{
_typedSet = typedSetter;
_untypedSet = setter is Action<object, object?> untypedSet ? untypedSet : (obj, value) => typedSetter(obj, (T)value!);
}
else
{
_untypedSet = value;
TypedSetValue = value == null ? null : (o, v) => value(o, v);
HasSetter = value != null;
Action<object, object?> untypedSet = (Action<object, object?>)setter;
_typedSet = ((obj, value) => untypedSet(obj, (T)value!));
_untypedSet = untypedSet;
}
}

Expand Down Expand Up @@ -112,15 +133,13 @@ internal override void Initialize(
MethodInfo? getMethod = propertyInfo.GetMethod;
if (getMethod != null && (getMethod.IsPublic || useNonPublicAccessors))
{
HasGetter = true;
TypedGetValue = options.MemberAccessorStrategy.CreatePropertyGetter<T>(propertyInfo);
Get = options.MemberAccessorStrategy.CreatePropertyGetter<T>(propertyInfo);
}

MethodInfo? setMethod = propertyInfo.SetMethod;
if (setMethod != null && (setMethod.IsPublic || useNonPublicAccessors))
{
HasSetter = true;
TypedSetValue = options.MemberAccessorStrategy.CreatePropertySetter<T>(propertyInfo);
Set = options.MemberAccessorStrategy.CreatePropertySetter<T>(propertyInfo);
}

MemberType = MemberTypes.Property;
Expand All @@ -132,13 +151,11 @@ internal override void Initialize(
{
Debug.Assert(fieldInfo.IsPublic);

HasGetter = true;
TypedGetValue = options.MemberAccessorStrategy.CreateFieldGetter<T>(fieldInfo);
Get = options.MemberAccessorStrategy.CreateFieldGetter<T>(fieldInfo);

if (!fieldInfo.IsInitOnly)
{
HasSetter = true;
TypedSetValue = options.MemberAccessorStrategy.CreateFieldSetter<T>(fieldInfo);
Set = options.MemberAccessorStrategy.CreateFieldSetter<T>(fieldInfo);
}

MemberType = MemberTypes.Field;
Expand All @@ -158,8 +175,6 @@ internal override void Initialize(
else if (!isCustomProperty)
{
IsForTypeInfo = true;
HasGetter = true;
HasSetter = true;
}
}

Expand Down Expand Up @@ -213,10 +228,8 @@ internal void InitializeForSourceGen(JsonSerializerOptions options, JsonProperty
}
else
{
TypedGetValue = propertyInfo.Getter!;
TypedSetValue = propertyInfo.Setter;
HasGetter = TypedGetValue != null;
HasSetter = TypedSetValue != null;
Get = propertyInfo.Getter!;
Set = propertyInfo.Setter;
JsonTypeInfo = propertyTypeInfo;
DeclaringType = declaringType;
IgnoreCondition = propertyInfo.IgnoreCondition;
Expand Down Expand Up @@ -277,12 +290,12 @@ internal override JsonConverter EffectiveConverter
}

Debug.Assert(HasGetter);
return TypedGetValue!(obj);
return Get!(obj);
}

internal override bool GetMemberAndWriteJson(object obj, ref WriteStack state, Utf8JsonWriter writer)
{
T value = TypedGetValue!(obj);
T value = Get!(obj);

if (ShouldSerialize != null)
{
Expand Down Expand Up @@ -384,7 +397,7 @@ value is not null &&
internal override bool GetMemberAndWriteJsonExtensionData(object obj, ref WriteStack state, Utf8JsonWriter writer)
{
bool success;
T value = TypedGetValue!(obj);
T value = Get!(obj);

if (ShouldSerialize != null)
{
Expand Down Expand Up @@ -425,7 +438,7 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref
if (!IgnoreDefaultValuesOnRead)
{
T? value = default;
TypedSetValue!(obj, value!);
Set!(obj, value!);
}

success = true;
Expand All @@ -439,7 +452,7 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref
{
// Optimize for internal converters by avoiding the extra call to TryRead.
T? fastValue = TypedEffectiveConverter.Read(ref reader, PropertyType, Options);
TypedSetValue!(obj, fastValue!);
Set!(obj, fastValue!);
}

success = true;
Expand Down Expand Up @@ -470,7 +483,7 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref
}
}

TypedSetValue!(obj, value!);
Set!(obj, value!);
}
}
}
Expand Down Expand Up @@ -517,7 +530,7 @@ internal override void SetExtensionDictionaryAsObject(object obj, object? extens
{
Debug.Assert(HasSetter);
T typedValue = (T)extensionDict!;
TypedSetValue!(obj, typedValue);
Set!(obj, typedValue);
}
}
}
11 changes: 1 addition & 10 deletions ...libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,17 +44,8 @@ private protected override void SetCreateObject(Delegate? createObject, bool use
}
else if (createObject is Func<T> typedDelegate)
{
if (createObject is Func<object> untypedDelegate)
{
untypedCreateObject = untypedDelegate;
}
else
{
// we will get here if T is value type
untypedCreateObject = () => typedDelegate()!;
}

typedCreateObject = typedDelegate;
untypedCreateObject = createObject is Func<object> untypedDelegate ? untypedDelegate : () => typedDelegate()!;
}
else
{
Expand Down