Copilot commented on code in PR #3394:
URL: https://github.com/apache/fory/pull/3394#discussion_r3055386002
##########
javascript/packages/core/lib/gen/map.ts:
##########
@@ -296,7 +296,10 @@ export class MapSerializerGenerator extends
BaseSerializerGenerator {
}
private isAny() {
- return this.typeInfo.options?.key!.typeId === TypeId.UNKNOWN ||
this.typeInfo.options?.value!.typeId === TypeId.UNKNOWN ||
!this.typeInfo.options?.key?.isMonomorphic() ||
!this.typeInfo.options?.value?.isMonomorphic();
+ const keyTypeId = this.typeInfo.options?.key!.typeId;
+ const valueTypeId = this.typeInfo.options?.value!.typeId;
+ return keyTypeId === TypeId.UNKNOWN || valueTypeId === TypeId.UNKNOWN
+ || !TypeId.isBuiltin(keyTypeId!) || !TypeId.isBuiltin(valueTypeId!);
}
Review Comment:
MapSerializerGenerator.isAny() now treats any non-builtin key/value type as
“any”, forcing the MapAnySerializer path even when the key/value TypeInfo is
monomorphic and declared (e.g., struct/enum). This bypasses the specialized
codegen path in writeSpecificType/readSpecificType and can be a significant
performance regression for common maps of user-defined types. If correctness
doesn’t require the fallback, consider restoring the previous
monomorphism-based check (or otherwise allowing declared monomorphic
user-defined types to use the specialized path).
##########
javascript/packages/core/lib/gen/union.ts:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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.
+ */
+
+import { TypeInfo } from "../typeInfo";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { RefFlags, TypeId } from "../type";
+import { Scope } from "./scope";
+import { AnyHelper } from "./any";
+
+class UnionSerializerGenerator extends BaseSerializerGenerator {
+ typeInfo: TypeInfo;
+ detectedSerializer: string;
+ writerSerializer: string;
+ caseTypesVar: string;
+
+ constructor(typeInfo: TypeInfo, builder: CodecBuilder, scope: Scope) {
+ super(typeInfo, builder, scope);
+ this.typeInfo = typeInfo;
+ this.detectedSerializer = this.scope.declareVar("detectedSerializer",
"null");
+ this.writerSerializer = this.scope.declareVar("writerSerializer", "null");
+
+ // Build case-to-type mapping from typeInfo.options.cases
+ const cases = typeInfo.options?.cases;
+ if (cases) {
+ const caseEntries: string[] = [];
+ for (const [caseIdx, caseTypeInfo] of Object.entries(cases)) {
+ const ti = caseTypeInfo as TypeInfo;
+ const isNamed = TypeId.isNamedType(ti._typeId);
+ const named = isNamed ? `"${ti.named}"` : "null";
+ caseEntries.push(`${caseIdx}: { typeId: ${ti.typeId}, userTypeId:
${ti.userTypeId ?? -1}, named: ${named} }`);
+ }
+ this.caseTypesVar = this.scope.declareVar("caseTypes", `{
${caseEntries.join(", ")} }`);
+ } else {
+ this.caseTypesVar = this.scope.declareVar("caseTypes", "null");
+ }
+ }
+
+ needToWriteRef(): boolean {
+ return false;
+ }
+
+ write(accessor: string): string {
+ const caseIndex = this.scope.uniqueName("caseIndex");
+ const unionValue = this.scope.uniqueName("unionValue");
+ const caseInfo = this.scope.uniqueName("caseInfo");
+ return `
+ const ${caseIndex} = ${accessor} && ${accessor}.case != null ?
${accessor}.case : 0;
+ ${this.builder.writer.writeVarUInt32(caseIndex)}
+ const ${unionValue} = ${accessor} && ${accessor}.value != null ?
${accessor}.value : null;
+ if (${unionValue} === null || ${unionValue} === undefined) {
+ ${this.builder.writer.writeInt8(RefFlags.NullFlag)}
+ } else {
+ const ${caseInfo} = ${this.caseTypesVar} ?
${this.caseTypesVar}[${caseIndex}] : null;
+ if (${caseInfo}) {
+ ${this.writerSerializer} = ${caseInfo}.named ?
fory.typeResolver.getSerializerByName(${caseInfo}.named) :
fory.typeResolver.getSerializerById(${caseInfo}.typeId, ${caseInfo}.userTypeId);
+ } else {
+ ${this.writerSerializer} =
${this.builder.getExternal(AnyHelper.name)}.getSerializer(${this.builder.getForyName()},
${unionValue});
+ }
+ if (${this.writerSerializer}.needToWriteRef()) {
+ const existsId =
${this.builder.referenceResolver.existsWriteObject(unionValue)};
+ if (typeof existsId === "number") {
+ ${this.builder.writer.writeInt8(RefFlags.RefFlag)}
+ ${this.builder.writer.writeVarUInt32("existsId")}
+ } else {
+ ${this.builder.writer.writeInt8(RefFlags.RefValueFlag)}
+ ${this.builder.referenceResolver.writeRef(unionValue)}
+ ${this.writerSerializer}.writeTypeInfo();
+ ${this.writerSerializer}.write(${unionValue});
+ }
+ } else {
+ ${this.builder.writer.writeInt8(RefFlags.NotNullValueFlag)}
+ ${this.writerSerializer}.writeTypeInfo();
+ ${this.writerSerializer}.write(${unionValue});
+ }
+ }
+ `;
+ }
+
+ read(assignStmt: (v: string) => string, refState: string): string {
+ void refState;
+ const caseIndex = this.scope.uniqueName("caseIndex");
+ const refFlag = this.scope.uniqueName("refFlag");
+ const unionValue = this.scope.uniqueName("unionValue");
+ const result = this.scope.uniqueName("result");
+ return `
+ const ${caseIndex} = ${this.builder.reader.readVarUInt32()};
+ const ${refFlag} = ${this.builder.reader.readInt8()};
+ let ${unionValue} = null;
+ if (${refFlag} === ${RefFlags.NullFlag}) {
+ ${unionValue} = null;
+ } else if (${refFlag} === ${RefFlags.RefFlag}) {
+ ${unionValue} =
${this.builder.referenceResolver.getReadObject(this.builder.reader.readVarUInt32())};
+ } else {
+ ${this.detectedSerializer} =
${this.builder.getExternal(AnyHelper.name)}.detectSerializer(${this.builder.getForyName()});
+ fory.incReadDepth();
+ ${unionValue} = ${this.detectedSerializer}.read(${refFlag} ===
${RefFlags.RefValueFlag});
+ fory.decReadDepth();
+ }
+ const ${result} = { case: ${caseIndex}, value: ${unionValue} };
+ ${assignStmt(result)}
+ `;
+ }
+
+ getFixedSize(): number {
+ return 12;
+ }
+
+ getTypeId() {
+ return TypeId.TYPED_UNION;
+ }
+}
+
+CodegenRegistry.register(TypeId.TYPED_UNION, UnionSerializerGenerator);
+CodegenRegistry.register(TypeId.NAMED_UNION, UnionSerializerGenerator);
Review Comment:
UnionSerializerGenerator is registered for both TypeId.TYPED_UNION and
TypeId.NAMED_UNION, but getTypeId() always returns TypeId.TYPED_UNION. This
will cause named-union serializers to emit the wrong typeId in type info (and
potentially try to write/read a userTypeId), breaking
AnyHelper.detectSerializer for TypeId.NAMED_UNION. getTypeId() should reflect
the underlying typeInfo.typeId (and NAMED_UNION likely also needs custom
writeTypeInfo/readTypeInfo to write/skip namespace+typeName or TypeMeta like
other named types).
##########
compiler/fory_compiler/generators/__init__.py:
##########
@@ -33,6 +34,7 @@
"rust": RustGenerator,
"go": GoGenerator,
"csharp": CSharpGenerator,
+ "javascript": JavaScriptGenerator,
"swift": SwiftGenerator,
Review Comment:
PR description references a new
`compiler/fory_compiler/generators/typescript.py` and a `TypeScriptGenerator`,
but the implementation added in this PR is
`compiler/fory_compiler/generators/javascript.py` with `JavaScriptGenerator`
registered under the `javascript` language. Please update the PR description
(or naming) to match the actual generator/module names to avoid confusion for
reviewers and users.
##########
javascript/packages/core/lib/typeInfo.ts:
##########
@@ -683,6 +684,31 @@ export const Type = {
withConstructor,
});
},
+ union(idOrCases?: number | { namespace?: string; typeName?: string } | {
[caseIndex: number]: TypeInfo }, cases?: { [caseIndex: number]: TypeInfo }) {
+ let typeInfo: TypeInfo;
+ if (typeof idOrCases === "number") {
+ typeInfo = new TypeInfo<typeof TypeId.TYPED_UNION>(TypeId.TYPED_UNION);
+ typeInfo.userTypeId = idOrCases;
+ if (cases) {
+ typeInfo.options = { cases };
+ }
+ } else if (idOrCases && ("namespace" in idOrCases || "typeName" in
idOrCases)) {
+ const nameInfo = idOrCases as { namespace?: string; typeName?: string };
+ typeInfo = new TypeInfo<typeof TypeId.NAMED_UNION>(TypeId.NAMED_UNION);
+ typeInfo.namespace = nameInfo.namespace || "";
+ typeInfo.typeName = nameInfo.typeName || "";
+ typeInfo.named = `${typeInfo.namespace}$${typeInfo.typeName}`;
+ if (cases) {
+ typeInfo.options = { cases };
+ }
+ } else {
+ typeInfo = new TypeInfo<typeof TypeId.TYPED_UNION>(TypeId.TYPED_UNION);
+ if (idOrCases) {
+ typeInfo.options = { cases: idOrCases as { [caseIndex: number]:
TypeInfo } };
+ }
+ }
+ return typeInfo;
Review Comment:
Type.union() creates a TypeId.TYPED_UNION TypeInfo even when no numeric
union id is provided (cases-only overload). That leaves userTypeId at -1, but
TYPED_UNION is in TypeId.needsUserTypeId, so any path that writes type info for
this TypeInfo will encode an invalid userTypeId (effectively 0xFFFFFFFF).
Consider using TypeId.UNION for the cases-only overload (and registering a
serializer for TypeId.UNION), or require an explicit numeric id for TYPED_UNION
so userTypeId is always valid.
##########
javascript/packages/core/lib/gen/struct.ts:
##########
@@ -286,35 +328,92 @@ class StructSerializerGenerator extends
BaseSerializerGenerator {
}
readEmbed() {
+ const serializerExpr = this.serializerExpr;
+ const scope = this.scope;
+ const builder = this.builder;
return new Proxy({}, {
get: (target, prop: string) => {
+ if (prop === "readNoRef") {
+ return (accessor: (expr: string) => string, refState: string) => {
+ const result = scope.uniqueName("result");
+ return `
+ ${serializerExpr}.readTypeInfo();
+ fory.incReadDepth();
+ let ${result} = ${serializerExpr}.read(${refState});
+ fory.decReadDepth();
+ ${accessor(result)};
+ `;
+ };
+ }
+ if (prop === "readRef") {
+ return (accessor: (expr: string) => string) => {
+ const refFlag = scope.uniqueName("refFlag");
+ const result = scope.uniqueName("result");
+ return `
+ const ${refFlag} = ${builder.reader.readInt8()};
+ let ${result};
+ if (${refFlag} === ${RefFlags.NullFlag}) {
+ ${result} = null;
+ } else if (${refFlag} === ${RefFlags.RefFlag}) {
+ ${result} =
${builder.referenceResolver.getReadObject(builder.reader.readVarUInt32())};
+ } else {
+ ${serializerExpr}.readTypeInfo();
+ fory.incReadDepth();
+ ${result} = ${serializerExpr}.read(${refFlag} ===
${RefFlags.RefValueFlag});
+ fory.decReadDepth();
+ }
+ ${accessor(result)};
+ `;
+ };
+ }
+ if (prop === "readWithDepth") {
+ return (accessor: (expr: string) => string, refState: string) => {
+ const result = scope.uniqueName("result");
+ return `
+ fory.incReadDepth();
+ let ${result} = ${serializerExpr}.read(${refState});
+ fory.decReadDepth();
+ ${accessor(result)};
+ `;
+ };
+ }
return (accessor: (expr: string) => string, ...args: string[]) => {
- const name = this.scope.declare(
- "tag_ser",
- TypeId.isNamedType(this.typeInfo.typeId)
- ?
this.builder.typeResolver.getSerializerByName(CodecBuilder.replaceBackslashAndQuote(this.typeInfo.named!))
- :
this.builder.typeResolver.getSerializerById(this.typeInfo.typeId,
this.typeInfo.userTypeId)
- );
- return accessor(`${name}.${prop}(${args.join(",")})`);
+ return accessor(`${serializerExpr}.${prop}(${args.join(",")})`);
};
},
});
}
writeEmbed() {
+ const serializerExpr = this.serializerExpr;
+ const scope = this.scope;
return new Proxy({}, {
get: (target, prop: string) => {
+ if (prop === "writeNoRef") {
+ return (accessor: string) => {
+ return `
+ ${serializerExpr}.writeTypeInfo(${accessor});
+ ${serializerExpr}.write(${accessor});
+ `;
+ };
+ }
+ if (prop === "writeRef") {
+ return (accessor: string) => {
+ const noneedWrite = scope.uniqueName("noneedWrite");
+ return `
+ let ${noneedWrite} =
${serializerExpr}.writeRefOrNull(${accessor});
+ if (!${noneedWrite}) {
+ ${serializerExpr}.writeTypeInfo(${accessor});
+ ${serializerExpr}.write(${accessor});
+ }
+ `;
+ };
+ }
return (accessor: string, ...args: any) => {
- const name = this.scope.declare(
- "tag_ser",
- TypeId.isNamedType(this.typeInfo.typeId)
- ?
this.builder.typeResolver.getSerializerByName(CodecBuilder.replaceBackslashAndQuote(this.typeInfo.named!))
- :
this.builder.typeResolver.getSerializerById(this.typeInfo.typeId,
this.typeInfo.userTypeId)
- );
if (prop === "writeRefOrNull") {
- return args[0](`${name}.${prop}(${accessor})`);
+ return args[0](`${serializerExpr}.${prop}(${accessor})`);
}
- return `${name}.${prop}(${accessor})`;
+ return `${serializerExpr}.${prop}(${accessor})`;
};
Review Comment:
readEmbed()/writeEmbed() now inline
`fory.typeResolver.getSerializerById/getSerializerByName` via serializerExpr
each time a nested struct is read/written (including inside loops for
collections). Previously this was cached via scope.declare(...) so the
serializer resolution happened once per generated serializer. Consider caching
serializerExpr into a declared variable to avoid repeated typeResolver lookups
in hot paths while still using the correct (own) TypeInfo/serializer for nested
structs.
--
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]
