chaokunyang commented on code in PR #3254:
URL: https://github.com/apache/fory/pull/3254#discussion_r2890818920
##########
java/fory-core/src/main/java/org/apache/fory/serializer/ArraySerializers.java:
##########
@@ -868,6 +869,146 @@ private void readFloat64BySwapEndian(MemoryBuffer buffer,
double[] values, int n
}
}
+ public static final class Float16ArraySerializer extends
PrimitiveArraySerializer<Float16[]> {
+ private static final int NULLABLE_ENCODING_FLAG = 1;
+
+ public Float16ArraySerializer(Fory fory) {
+ super(fory, Float16[].class);
+ }
+
+ @Override
+ public void write(MemoryBuffer buffer, Float16[] value) {
+ int length = value.length;
+ boolean hasNull = false;
+ for (int i = 0; i < length; i++) {
+ if (value[i] == null) {
+ hasNull = true;
+ break;
+ }
+ }
+ if (hasNull) {
+ writeNullable(buffer, value, length);
+ return;
+ }
+ writeNonNull(buffer, value, length);
+ }
+
+ private void writeNonNull(MemoryBuffer buffer, Float16[] value, int
length) {
+ int size = length * 2;
+ buffer.writeVarUint32Small7(size);
+
+ if (Platform.IS_LITTLE_ENDIAN) {
+ int writerIndex = buffer.writerIndex();
+ buffer.ensure(writerIndex + size);
+ for (int i = 0; i < length; i++) {
+ buffer._unsafePutInt16(writerIndex + i * 2, value[i].toBits());
+ }
+ buffer._unsafeWriterIndex(writerIndex + size);
+ } else {
+ for (int i = 0; i < length; i++) {
+ buffer.writeInt16(value[i].toBits());
+ }
+ }
+ }
+
+ private void writeNullable(MemoryBuffer buffer, Float16[] value, int
length) {
+ int bitmapSize = (length + 7) >>> 3;
+ int payloadSize = Integer.BYTES + bitmapSize + length * 2;
+ int encodedSize = (payloadSize << 1) | NULLABLE_ENCODING_FLAG;
Review Comment:
This changes the FLOAT16_ARRAY wire layout when nulls are present, which
will break cross-language readers that expect the normal length+payload format
for type 53.
In xlang mode, other runtimes decode FLOAT16_ARRAY as plain
[byte_length][2-byte elements]. Here we switch to an alternate encoded-size
scheme for nullable arrays, so non-Java runtimes will misread the payload.
Could we keep FLOAT16_ARRAY encoding stable and handle nullable cases another
way (for example, fallback to LIST encoding when null is present)?
##########
java/fory-core/src/main/java/org/apache/fory/type/DispatchId.java:
##########
@@ -49,19 +49,23 @@ public class DispatchId {
public static final int UINT32 = 14;
public static final int VAR_UINT32 = 15;
public static final int UINT64 = 16;
- public static final int VAR_UINT64 = 17;
- public static final int TAGGED_UINT64 = 18;
- public static final int EXT_UINT8 = 19;
- public static final int EXT_UINT16 = 20;
- public static final int EXT_UINT32 = 21;
- public static final int EXT_VAR_UINT32 = 22;
- public static final int EXT_UINT64 = 23;
- public static final int EXT_VAR_UINT64 = 24;
- public static final int STRING = 25;
+ public static final int FLOAT16 = 17;
Review Comment:
Since FLOAT16 is now a basic dispatch id, FieldSkipper also needs a skip
branch for it.
Right now FieldSkipper has no DispatchId.FLOAT16 case. In compatibility
reads, skipping an unknown Float16 field will throw Unexpected basic dispatchId
instead of advancing by 2 bytes. Please add FLOAT16 to the 2-byte skip branch.
##########
java/fory-core/src/main/java/org/apache/fory/serializer/Serializers.java:
##########
@@ -570,6 +571,7 @@ public static void registerDefaultSerializers(Fory fory) {
resolver.registerInternalSerializer(URI.class, new URISerializer(fory));
resolver.registerInternalSerializer(Pattern.class, new
RegexSerializer(fory));
resolver.registerInternalSerializer(UUID.class, new UUIDSerializer(fory));
+ resolver.registerInternalSerializer(Float16.class, new
Float16Serializer(fory));
Review Comment:
Float16 gets registered twice (primitive serializer path and general
serializers path).
This overrides the earlier registration and makes it harder to reason about
which serializer is intended. It would be cleaner to keep a single registration
point for Float16.
##########
compiler/fory_compiler/generators/java.py:
##########
@@ -1458,7 +1460,11 @@ def generate_equals_method(self, message: Message) ->
List[str]:
)
elif isinstance(field.field_type, PrimitiveType):
kind = field.field_type.kind
- if kind in (PrimitiveKind.FLOAT32,):
+ if kind in (PrimitiveKind.FLOAT16,):
+ comparisons.append(
+ f"({field_name} == null ? that.{field_name} ==
null : (that.{field_name} != null &&
{field_name}.equalsValue(that.{field_name})))"
Review Comment:
Generated equals/hashCode for float16 now use numeric semantics, but
Float16.equals/hashCode are bitwise.
With this change, +0 and -0 compare equal in generated messages, and NaN is
not equal to NaN, while Float16 itself uses raw-bit equality. That mismatch is
surprising in user code. Can we align generated message behavior with
Float16.equals/hashCode (or make the rule explicit and consistent across
runtimes)?
##########
java/fory-core/src/main/java/org/apache/fory/collection/Float16List.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.collection;
+
+import java.util.AbstractList;
+import java.util.Arrays;
+import java.util.Objects;
+import java.util.RandomAccess;
+import org.apache.fory.type.Float16;
+
+public final class Float16List extends AbstractList<Float16> implements
RandomAccess {
+ private static final int DEFAULT_CAPACITY = 10;
+
+ private short[] array;
+ private int size;
+
+ public Float16List() {
+ this(DEFAULT_CAPACITY);
+ }
+
+ public Float16List(int initialCapacity) {
+ if (initialCapacity < 0) {
+ throw new IllegalArgumentException("Illegal capacity: " +
initialCapacity);
+ }
+ this.array = new short[initialCapacity];
+ this.size = 0;
+ }
+
+ public Float16List(short[] array) {
+ this.array = array;
+ this.size = array.length;
+ }
+
+ @Override
+ public Float16 get(int index) {
+ checkIndex(index);
+ return Float16.fromBits(array[index]);
+ }
+
+ @Override
+ public int size() {
+ return size;
+ }
+
+ @Override
+ public Float16 set(int index, Float16 element) {
+ checkIndex(index);
+ Objects.requireNonNull(element, "element");
+ short prev = array[index];
+ array[index] = element.toBits();
+ return Float16.fromBits(prev);
+ }
+
+ public void set(int index, short bits) {
+ checkIndex(index);
+ array[index] = bits;
+ }
+
+ public void set(int index, float value) {
+ checkIndex(index);
+ array[index] = Float16.valueOf(value).toBits();
+ }
+
+ @Override
+ public void add(int index, Float16 element) {
+ checkPositionIndex(index);
+ ensureCapacity(size + 1);
+ System.arraycopy(array, index, array, index + 1, size - index);
+ array[index] = element.toBits();
+ size++;
+ modCount++;
+ }
+
+ @Override
+ public boolean add(Float16 element) {
+ Objects.requireNonNull(element, "element");
+ ensureCapacity(size + 1);
+ array[size++] = element.toBits();
+ modCount++;
+ return true;
+ }
+
+ public boolean add(short bits) {
+ ensureCapacity(size + 1);
+ array[size++] = bits;
+ modCount++;
+ return true;
+ }
+
+ public boolean add(float value) {
Review Comment:
Float16List primitive methods still allocate Float16 objects on the hot path.
add(float), set(float), and getFloat(int) all go through
Float16.valueOf/fromBits, so these paths allocate even though this is a
primitive-style container. A static bits conversion helper would keep these
methods allocation-free.
--
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]
