chaokunyang commented on code in PR #3861: URL: https://github.com/apache/fory/pull/3861#discussion_r3592420569
########## java/fory-core/src/main/java/org/apache/fory/serializer/ForyExtraFields.java: ########## @@ -0,0 +1,114 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fory.serializer; + +import java.lang.reflect.Field; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import org.apache.fory.collection.ClassValueCache; +import org.apache.fory.meta.TypeDef; +import org.apache.fory.reflect.FieldAccessor; + +/** + * Opt-in sink for compatible-mode extra fields. A class participates by declaring a field of this + * type; the serialization framework detects it, excludes it from the normal field set, and routes Review Comment: Actually exclude the selected sink from normal serialization metadata The contract says the framework excludes this field, but no descriptor, TypeDef, or ObjectSerializer owner filters it. A fresh sink-bearing type therefore exposes this framework field in its local schema, and an initialized sink can be serialized as a normal nested object instead of replaying the remote schema. Please exclude the exact selected instance field in the owning descriptor path and assert that it is absent from the local TypeDef/body. ########## java/fory-core/src/main/java/org/apache/fory/resolver/TypeResolver.java: ########## @@ -1305,9 +1334,17 @@ private TypeInfo getMetaSharedTypeInfo(TypeDef typeDef, Class<?> clz) { if (StaticGeneratedStructSerializer.class.isAssignableFrom(sc)) { typeInfo.setSerializer(this, newStaticGeneratedStructSerializer(sc, cls, typeDef)); } else if (sc == CompatibleSerializer.class) { - typeInfo.setSerializer(this, new CompatibleSerializer(this, cls, typeDef)); + CompatibleSerializer<?> cs = new CompatibleSerializer<>(this, cls, typeDef); Review Comment: Implement capture and replay for the required static and layer serializers Only the runtime `CompatibleSerializer` and generated-compatible branches register replay serializers. `StaticGeneratedStructSerializer` still skips unknown fields, and the CompatibleLayer/ObjectStream path does the same. The issue this PR closes explicitly includes annotation processor, KSP, Scala derive, StaticCompatible, and CompatibleLayer support; deferring them in the guide leaves those supported Java surfaces silently dropping data, including the recommended GraalVM/static-codegen path. Please complete those owner paths or stop closing the full issue and narrow the documented scope. ########## java/fory-core/src/main/java/org/apache/fory/resolver/TypeInfo.java: ########## @@ -170,6 +174,15 @@ public void setSerializer(Serializer<?> serializer) { void setSerializer(TypeResolver resolver, Serializer<?> serializer) { this.serializer = serializer; needToWriteTypeDef = serializer != null && resolver.needToWriteTypeDef(serializer); + this.extraFieldsSinkAccessor = type == null ? null : ForyExtraFields.findSinkAccessor(type); Review Comment: Preserve the sink accessor across TypeInfo copies `extraFieldsSinkAccessor` is initialized only in this setter, while both `copy(...)` methods keep the serializer but drop this state. The supported sequence of materializing/registering a serializer before numeric class registration therefore produces a copied TypeInfo that can capture remote fields but no longer enters replay on write. Treat the accessor as type-owned metadata and initialize or preserve it in every constructor/copy path. ########## java/fory-core/src/main/java/org/apache/fory/resolver/TypeResolver.java: ########## @@ -514,6 +515,30 @@ public boolean isMapDescriptor(Descriptor descriptor) { public abstract TypeInfo getTypeInfo(Class<?> cls, TypeInfoHolder classInfoHolder); + public final TypeInfo getTypeInfoByTypeDefId(long typeDefId) { + return extRegistry.typeInfoByTypeDefId.get(typeDefId); + } + + public final Serializer<?> getExtraFieldsWriteSerializer(Class<?> cls, long typeDefId) { + Map<Long, Serializer<?>> idToSerializer = extRegistry.extraFieldsSerializers.get(cls); + return idToSerializer != null ? idToSerializer.get(typeDefId) : null; + } + + private static boolean hasExtraFieldsSinkField(Class<?> cls) { + return ForyExtraFields.findSinkField(cls) != null; + } + + /** + * Records the reader-class serializer used to replay {@code cls} under the remote {@code + * typeDefId} it was captured from. + */ + private void registerExtraFieldsSerializer(Class<?> cls, long typeDefId, Serializer<?> gen) { + extRegistry + .extraFieldsSerializers + .computeIfAbsent(cls, k -> new ConcurrentHashMap<>()) + .putIfAbsent(typeDefId, gen); Review Comment: Replace the interpreter replay serializer after async compilation The async path normally registers `CompatibleSerializer` first, then the compilation callback calls this method with the generated serializer. `putIfAbsent` leaves the interpreter entry permanently installed (or makes the result timing-dependent if compilation wins the race). The current JIT test checks the normal TypeInfo serializer rather than this replay cache, so it passes without exercising generated replay. Please perform a safe interpreter-to-generated handoff and assert the actual replay owner. ########## java/fory-core/src/main/java/org/apache/fory/serializer/ForyExtraFields.java: ########## @@ -0,0 +1,114 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fory.serializer; + +import java.lang.reflect.Field; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import org.apache.fory.collection.ClassValueCache; +import org.apache.fory.meta.TypeDef; +import org.apache.fory.reflect.FieldAccessor; + +/** + * Opt-in sink for compatible-mode extra fields. A class participates by declaring a field of this + * type; the serialization framework detects it, excludes it from the normal field set, and routes + * unmatched remote fields into it instead of discarding them. + * + * <p>The remote {@link TypeDef} is stored transiently so that on re-serialization the framework can + * emit the remote schema header and replay all fields in their original order, allowing a + * downstream peer with the full schema to recover them. + */ +public final class ForyExtraFields { + + private final Map<String, Object> fields = new HashMap<>(); + + public Object get(String name) { + return fields.get(name); + } + + public Object getOrDefault(String name, Object defaultValue) { + return fields.getOrDefault(name, defaultValue); + } + + public boolean isEmpty() { + return fields.isEmpty(); + } + + public boolean containsKey(String name) { + return fields.containsKey(name); + } + + Object put(String name, Object value) { + return fields.put(name, value); + } + + /** + * GC-transparent class-keyed cache: on JVM this is backed by ClassValue so entries do not prevent + * the associated Class (or its ClassLoader) from being collected. On Android/GraalVM it falls + * back to a ConcurrentHashMap. + */ + private static final ClassValueCache<Optional<FieldAccessor>> SINK_CACHE = Review Comment: Avoid a process-global strong class cache on Android The Android fallback for this static `ClassValueCache` is an unbounded `ConcurrentHashMap<Class<?>, Object>`. Both positive accessors and negative `Optional.empty()` entries strongly retain user classes, so repeatedly creating and discarding DexClassLoaders/Fory runtimes leaks the loaders across runtimes. The accessor already has a natural per-TypeInfo owner; please remove the process-global cache or provide a genuinely weak-key fallback. ########## java/fory-core/src/main/java/org/apache/fory/context/WriteContext.java: ########## @@ -459,6 +513,9 @@ public void writeRef(Object obj) { depth--; return; } + if (typeInfo.hasExtraFieldsSink() && tryWriteExtraFieldsSchema(resolver, typeInfo, obj)) { Review Comment: Keep the opt-in feature out of unrelated object-write hot paths This adds a call and branch to every dynamic object write, including types without a sink and xlang/same-schema configurations. The generated polymorphic path adds the same work, while serializer setup scans every class. Please gate the feature during cold native-compatible setup/codegen so unrelated hot paths remain unchanged, and provide the zero-overhead benchmark or generated-code assertion required for this performance-sensitive path. ########## java/fory-core/src/main/java/org/apache/fory/serializer/ForyExtraFields.java: ########## @@ -0,0 +1,114 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fory.serializer; + +import java.lang.reflect.Field; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import org.apache.fory.collection.ClassValueCache; +import org.apache.fory.meta.TypeDef; +import org.apache.fory.reflect.FieldAccessor; + +/** + * Opt-in sink for compatible-mode extra fields. A class participates by declaring a field of this + * type; the serialization framework detects it, excludes it from the normal field set, and routes + * unmatched remote fields into it instead of discarding them. + * + * <p>The remote {@link TypeDef} is stored transiently so that on re-serialization the framework can + * emit the remote schema header and replay all fields in their original order, allowing a + * downstream peer with the full schema to recover them. + */ +public final class ForyExtraFields { + + private final Map<String, Object> fields = new HashMap<>(); + + public Object get(String name) { + return fields.get(name); + } + + public Object getOrDefault(String name, Object defaultValue) { + return fields.getOrDefault(name, defaultValue); + } + + public boolean isEmpty() { + return fields.isEmpty(); + } + + public boolean containsKey(String name) { + return fields.containsKey(name); + } + + Object put(String name, Object value) { + return fields.put(name, value); + } + + /** + * GC-transparent class-keyed cache: on JVM this is backed by ClassValue so entries do not prevent + * the associated Class (or its ClassLoader) from being collected. On Android/GraalVM it falls + * back to a ConcurrentHashMap. + */ + private static final ClassValueCache<Optional<FieldAccessor>> SINK_CACHE = + ClassValueCache.newClassKeyCache(16); + + public static Field findSinkField(Class<?> cls) { + return SINK_CACHE + .get(cls, () -> scanForExtraField(cls)) + .map(FieldAccessor::getField) + .orElse(null); + } + + /** Returns a {@link FieldAccessor} for the {@link ForyExtraFields} sink field on {@code cls}. */ + public static FieldAccessor findSinkAccessor(Class<?> cls) { + return SINK_CACHE.get(cls, () -> scanForExtraField(cls)).orElse(null); + } + + private static Optional<FieldAccessor> scanForExtraField(Class<?> cls) { + for (Class<?> c = cls; c != null && c != Object.class; c = c.getSuperclass()) { + for (Field f : c.getDeclaredFields()) { + if (ForyExtraFields.class == f.getType()) { + return Optional.of(FieldAccessor.createAccessor(f)); + } + } + } + return Optional.empty(); + } + + // Excluded from Java serialization because TypeDef is runtime metadata used + // only for Fory serialization. If a ForyExtraFields instance is serialized + // with ObjectOutputStream, this field will be null after deserialization, so + // the captured extra fields cannot later be replayed by Fory. + private transient TypeDef typeDef; + + public TypeDef getTypeDef() { + return typeDef; + } + + public static void capture( Review Comment: Keep mutation and reflection helpers out of the public sink API The guide says application code has read-only access, but `capture`, `findSinkField`, and `findSinkAccessor` are public, unmarked implementation APIs. Callers can use them to mutate entries and replace the TypeDef association, breaking replay invariants while also exposing `FieldAccessor` as public surface. Move the generated-code bridge to an `@Internal` support owner and keep `ForyExtraFields` limited to the stable lookup API. -- 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]
