chaokunyang commented on code in PR #3451:
URL: https://github.com/apache/fory/pull/3451#discussion_r2881905176
##########
csharp/src/Fory/TypeResolver.cs:
##########
@@ -33,13 +33,14 @@ public sealed class TypeResolver
typeof(TypeResolver).GetMethod(
nameof(CreateNullableSerializerTypeInfo),
BindingFlags.NonPublic | BindingFlags.Instance)!;
- private static readonly HashSet<TypeId> CompatibleStructAllowedWireTypes =
- [
- TypeId.Struct,
- TypeId.NamedStruct,
- TypeId.CompatibleStruct,
- TypeId.NamedCompatibleStruct,
- ];
+
+ private static class GenericTypeCache<T>
Review Comment:
[P2] Static generic cache pins `TypeResolver` instances
`GenericTypeCache<T>` is static and stores a strong `TypeResolver`
reference. That means the last resolver used for each `T` is rooted for the
lifetime of the AppDomain, which can retain large registration graphs and
serializer caches after runtimes should be collectable. This is a regression
from the previous instance-only lookup path and can create long-lived memory
retention in services that create/destroy runtimes.
##########
csharp/src/Fory/TypeMeta.cs:
##########
@@ -553,6 +569,125 @@ public static TypeMeta Decode(ByteReader reader)
header >> (int)(64 - TypeMetaConstants.TypeMetaNumHashBits));
}
+ /// <summary>
+ /// Assigns local sorted field indexes for a remote compatible type meta.
+ /// The result is written to each remote field's <see
cref="TypeMetaFieldInfo.AssignedFieldId"/>:
+ /// - local sorted field index when a compatible local field is found
+ /// - -1 when no compatible local field is found and the field should be
skipped
+ /// </summary>
+ public static void AssignFieldIds(
+ TypeMeta remoteTypeMeta,
+ IReadOnlyList<TypeMetaFieldInfo> localFieldInfos)
+ {
+ ArgumentNullException.ThrowIfNull(remoteTypeMeta);
+ ArgumentNullException.ThrowIfNull(localFieldInfos);
+
+ Dictionary<string, (int Index, TypeMetaFieldInfo Field)> localByName =
new(localFieldInfos.Count, StringComparer.Ordinal);
+ Dictionary<short, (int Index, TypeMetaFieldInfo Field)> localById =
new(localFieldInfos.Count);
+ for (int i = 0; i < localFieldInfos.Count; i++)
+ {
+ TypeMetaFieldInfo localField = localFieldInfos[i];
+ if (!string.IsNullOrEmpty(localField.FieldName))
+ {
+ localByName.TryAdd(localField.FieldName, (i, localField));
+ }
+
+ if (localField.FieldId.HasValue && localField.FieldId.Value >= 0)
Review Comment:
[P2] Duplicate `FieldAttribute.Id` values are not rejected
Compatible field-id mapping uses `localById.TryAdd(...)`, so duplicate ids
are silently accepted and only the first field is kept in the lookup. With
duplicate ids on a generated model, multiple remote fields can resolve to the
same local field index and produce silent data loss/overwrite during compatible
reads. The generator should emit a diagnostic (or runtime should throw) when
duplicate non-negative field ids exist in a type.
##########
csharp/src/Fory/Context.cs:
##########
@@ -268,6 +383,8 @@ public ReadContext(
public bool CheckStructVersion { get; }
+ public bool ShouldConsumeCompatibleTypeMeta =>
_shouldConsumeCompatibleTypeMeta;
Review Comment:
[P3] Unused compatible-meta expectation state is dead code
`ShouldConsumeCompatibleTypeMeta`, `ReplaceCompatibleTypeMetaExpectation`,
and `RestoreCompatibleTypeMetaExpectation` are introduced but never read by any
serializer/runtime path in this PR. Keeping this state machine unused adds
maintenance overhead and makes compatible-read flow harder to reason about
without delivering behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
