You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fury.apache.org by "theweipeng (via GitHub)" <gi...@apache.org> on 2024/01/12 06:07:13 UTC
[PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
theweipeng opened a new pull request, #1337:
URL: https://github.com/apache/incubator-fury/pull/1337
Now we have introduced a new binary layout for Collection, which is utilized by both Array and Set. The Collection is defined in the [SPEC](https://github.com/apache/incubator-fury/blob/3da32797beb25fd39b06c42f8c8b9e67eec5184d/docs/protocols/java_object_graph_spec.md#unsigned-int). This PR implements the Collection in JavaScript.
1. Array and Set will extend CollectionSerializerGenerator to generate code.
2. The Xlang Collection is not stable yet; it may undergo changes in the future, particularly regarding polymorphism, although the changes are expected to be minimal.
3. We have refined the reference checks, which were previously disorganized.
4. The refactor of collection types has resulted in performance improvements. In our sample benchmark, we observed an increase in performance of nearly 20%.
#1330
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451382881
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,364 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
Review Comment:
This can be cached, the hash look up and method call is costly. We can cache it, and check whether element types are same as previous element to reduce this cose
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "LiangliangSui (via GitHub)" <gi...@apache.org>.
LiangliangSui commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1449897555
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,365 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
+ if (serializer !== null && current !== null && current !== serializer) {
+ isSame = false;
+ }
+ if (current !== null) {
+ serializer = current;
+ }
+ }
+ }
+
+ if (!isSame) {
+ flag |= CollectionFlags.NOT_SAME_TYPE;
+ }
+ if (includeNone) {
+ flag |= CollectionFlags.HAS_NULL;
+ }
+ if (serializer !== null && serializer.meta.needToWriteRef) {
+ flag |= CollectionFlags.TRACKING_REF;
+ }
+ this.fury.binaryWriter.uint8(flag);
+ if (isSame) {
+ if (serializer) {
+ this.fury.binaryWriter.int16(serializer.meta.type);
+ } else {
+ this.fury.binaryWriter.skip(2);
+ }
+ }
+ return {
+ serializer,
+ isSame,
+ flag,
+ includeNone,
+ };
+ }
+
+ write(value: any, size: number) {
+ const { serializer, isSame, includeNone } = this.writeElementsHeader(value);
+ this.fury.binaryWriter.varUInt32(size);
+ if (isSame) {
+ if (!includeNone) {
+ for (const item of value) {
+ serializer!.write(item);
+ }
+ return;
+ } else {
+ for (const item of value) {
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ serializer!.write(item);
+ }
+ }
+ } else {
+ if (includeNone) {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ } else {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ }
+ }
Review Comment:
Can something be streamlined here? A lot of repetition.
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451382936
##########
javascript/test/__snapshots__/codeGen.test.ts.snap:
##########
@@ -8,13 +8,13 @@ return function(fury, external) {
const bw = fury.binaryWriter;
const cr = fury.classResolver;
const rr = fury.referenceResolver;
- const tag_ser_3 = fury.classResolver.getSerializerByTag("example.foo.1");
- const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.2");
- const tag_ser_12 = fury.classResolver.getSerializerByTag("example.bar.1");
- const tag_ser_13 = fury.classResolver.getSerializerByTag("example.bar.2");
- const tagWriter_19 = cr.createTagWriter("tuple-object-wrapper");
+ const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.1");
Review Comment:
Also used for reviewing the generated code on GitHub and tracing the code changes with Git.
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "bytemain (via GitHub)" <gi...@apache.org>.
bytemain commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1449974597
##########
javascript/test/__snapshots__/codeGen.test.ts.snap:
##########
@@ -25,136 +25,178 @@ return function(fury, external) {
tuple2: null,
tuple2_: null
};
- rr.pushReadObject(result_0)
+ if (fromRef) {
+ rr.reference(result_0)
+ } else {
+
+ }
+
+
+ let fromRef_1 = false;
switch (br.int8()) {
+ case 0:
Review Comment:
here we have two `case 0`
##########
javascript/test/__snapshots__/codeGen.test.ts.snap:
##########
@@ -25,136 +25,178 @@ return function(fury, external) {
tuple2: null,
tuple2_: null
};
- rr.pushReadObject(result_0)
+ if (fromRef) {
+ rr.reference(result_0)
+ } else {
+
Review Comment:
shall we remove this empty else condition statement
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451383541
##########
javascript/test/__snapshots__/codeGen.test.ts.snap:
##########
@@ -8,13 +8,13 @@ return function(fury, external) {
const bw = fury.binaryWriter;
const cr = fury.classResolver;
const rr = fury.referenceResolver;
- const tag_ser_3 = fury.classResolver.getSerializerByTag("example.foo.1");
- const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.2");
- const tag_ser_12 = fury.classResolver.getSerializerByTag("example.bar.1");
- const tag_ser_13 = fury.classResolver.getSerializerByTag("example.bar.2");
- const tagWriter_19 = cr.createTagWriter("tuple-object-wrapper");
+ const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.1");
Review Comment:
Can we use unittest to ensure the correctness of generated code. If we have a complicated class, we may generate thounds of lines of code. In fury java, I have see some big objects generated 6000 line of code.
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1449964050
##########
javascript/test/__snapshots__/codeGen.test.ts.snap:
##########
@@ -8,13 +8,13 @@ return function(fury, external) {
const bw = fury.binaryWriter;
const cr = fury.classResolver;
const rr = fury.referenceResolver;
- const tag_ser_3 = fury.classResolver.getSerializerByTag("example.foo.1");
- const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.2");
- const tag_ser_12 = fury.classResolver.getSerializerByTag("example.bar.1");
- const tag_ser_13 = fury.classResolver.getSerializerByTag("example.bar.2");
- const tagWriter_19 = cr.createTagWriter("tuple-object-wrapper");
+ const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.1");
Review Comment:
Could we avoid to commit `ts.snap` to repo?
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451389709
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,364 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
Review Comment:
The `getSerializerByData` function can only check the 8 base types of a JavaScript object because objects in JavaScript usually do not have a type. Therefore, it does not include a hash lookup, just an array index.
Additionally, the`writeElementsHeader` function is used only in compatible mode, where the user declares the type by using `Type.any()` rather than a specific type like `Type.string()`. If a specific type is declared,`readStmtSpecificType` and `writeStmtSpecificType` will generate code that does not require checking if the elements are of the same type.
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "LiangliangSui (via GitHub)" <gi...@apache.org>.
LiangliangSui commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1449917842
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,365 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
+ if (serializer !== null && current !== null && current !== serializer) {
+ isSame = false;
+ }
+ if (current !== null) {
+ serializer = current;
+ }
+ }
+ }
+
+ if (!isSame) {
+ flag |= CollectionFlags.NOT_SAME_TYPE;
+ }
+ if (includeNone) {
+ flag |= CollectionFlags.HAS_NULL;
+ }
+ if (serializer !== null && serializer.meta.needToWriteRef) {
+ flag |= CollectionFlags.TRACKING_REF;
+ }
+ this.fury.binaryWriter.uint8(flag);
+ if (isSame) {
+ if (serializer) {
+ this.fury.binaryWriter.int16(serializer.meta.type);
+ } else {
+ this.fury.binaryWriter.skip(2);
+ }
+ }
+ return {
+ serializer,
+ isSame,
+ flag,
+ includeNone,
+ };
+ }
+
+ write(value: any, size: number) {
+ const { serializer, isSame, includeNone } = this.writeElementsHeader(value);
+ this.fury.binaryWriter.varUInt32(size);
+ if (isSame) {
+ if (!includeNone) {
+ for (const item of value) {
+ serializer!.write(item);
+ }
+ return;
+ } else {
+ for (const item of value) {
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ serializer!.write(item);
+ }
+ }
+ } else {
+ if (includeNone) {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ } else {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ }
+ }
+ }
+
+ read(accessor: (result: any, index: number, v: any) => void, createCollection: (len: number) => any, fromRef: boolean): any {
+ const flags = this.fury.binaryReader.uint8();
+ const isSame = !(flags & CollectionFlags.NOT_SAME_TYPE);
+ const includeNone = flags & CollectionFlags.HAS_NULL;
+ if (isSame) {
+ const serializer = this.fury.classResolver.getSerializerById(this.fury.binaryReader.int16());
+ const len = this.fury.binaryReader.varUInt32();
+ const result = createCollection(len);
+ if (fromRef) {
+ this.fury.referenceResolver.reference(result);
+ }
+ if (!includeNone) {
+ for (let index = 0; index < len; index++) {
+ accessor(result, index, serializer.read());
+ }
+ } else {
+ for (let index = 0; index < len; index++) {
+ const refFlag = this.fury.binaryReader.uint8();
+ if (RefFlags.NullFlag === refFlag) {
+ accessor(result, index, null);
+ } else {
+ accessor(result, index, serializer.read());
+ }
+ }
+ }
+ return result;
+ } else {
+ const len = this.fury.binaryReader.varUInt32();
+ const result = createCollection(len);
+ if (!includeNone) {
+ for (let index = 0; index < len; index++) {
+ const serializer = this.fury.classResolver.getSerializerById(this.fury.binaryReader.int16());
+ accessor(result, index, serializer.read());
+ }
+ } else {
+ for (let index = 0; index < len; index++) {
+ const refFlag = this.fury.binaryReader.uint8();
+ if (RefFlags.NullFlag === refFlag) {
+ accessor(result, index, null);
+ } else {
+ const serializer = this.fury.classResolver.getSerializerById(this.fury.binaryReader.int16());
+ accessor(result, index, serializer.read());
+ }
+ }
+ }
+ return result;
+ }
Review Comment:
ditto
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451382192
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,364 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
+ if (serializer !== null && current !== null && current !== serializer) {
+ isSame = false;
+ }
+ if (current !== null) {
+ serializer = current;
+ }
+ }
+ }
+
+ if (!isSame) {
+ flag |= CollectionFlags.NOT_SAME_TYPE;
+ }
+ if (includeNone) {
+ flag |= CollectionFlags.HAS_NULL;
+ }
+ if (serializer !== null && serializer.meta.needToWriteRef) {
+ flag |= CollectionFlags.TRACKING_REF;
+ }
+ this.fury.binaryWriter.uint8(flag);
+ if (isSame) {
+ if (serializer) {
+ this.fury.binaryWriter.int16(serializer.meta.type);
+ } else {
+ this.fury.binaryWriter.skip(2);
+ }
+ }
+ return {
+ serializer,
+ isSame,
+ flag,
+ includeNone,
+ };
+ }
+
+ write(value: any, size: number) {
+ const { serializer, isSame, includeNone } = this.writeElementsHeader(value);
+ this.fury.binaryWriter.varUInt32(size);
+ if (isSame) {
+ if (!includeNone) {
+ for (const item of value) {
+ serializer!.write(item);
+ }
+ return;
+ } else {
+ for (const item of value) {
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ serializer!.write(item);
+ }
+ }
+ } else {
+ if (includeNone) {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
Review Comment:
this can be moved after `if` branch
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "bytemain (via GitHub)" <gi...@apache.org>.
bytemain commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1449966999
##########
javascript/test/__snapshots__/codeGen.test.ts.snap:
##########
@@ -8,13 +8,13 @@ return function(fury, external) {
const bw = fury.binaryWriter;
const cr = fury.classResolver;
const rr = fury.referenceResolver;
- const tag_ser_3 = fury.classResolver.getSerializerByTag("example.foo.1");
- const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.2");
- const tag_ser_12 = fury.classResolver.getSerializerByTag("example.bar.1");
- const tag_ser_13 = fury.classResolver.getSerializerByTag("example.bar.2");
- const tagWriter_19 = cr.createTagWriter("tuple-object-wrapper");
+ const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.1");
Review Comment:
.snap is used in unittest, it can ensure the generated code to match our expectation
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng commented on PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#issuecomment-1888496215
@bytemain The implementation of the collection mentioned in #1330 is complete.
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng merged PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451381808
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,365 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
+ if (serializer !== null && current !== null && current !== serializer) {
+ isSame = false;
+ }
+ if (current !== null) {
+ serializer = current;
+ }
+ }
+ }
+
+ if (!isSame) {
+ flag |= CollectionFlags.NOT_SAME_TYPE;
+ }
+ if (includeNone) {
+ flag |= CollectionFlags.HAS_NULL;
+ }
+ if (serializer !== null && serializer.meta.needToWriteRef) {
+ flag |= CollectionFlags.TRACKING_REF;
+ }
+ this.fury.binaryWriter.uint8(flag);
+ if (isSame) {
+ if (serializer) {
+ this.fury.binaryWriter.int16(serializer.meta.type);
+ } else {
+ this.fury.binaryWriter.skip(2);
+ }
+ }
+ return {
+ serializer,
+ isSame,
+ flag,
+ includeNone,
+ };
+ }
+
+ write(value: any, size: number) {
+ const { serializer, isSame, includeNone } = this.writeElementsHeader(value);
+ this.fury.binaryWriter.varUInt32(size);
+ if (isSame) {
+ if (!includeNone) {
+ for (const item of value) {
+ serializer!.write(item);
+ }
+ return;
+ } else {
+ for (const item of value) {
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ serializer!.write(item);
+ }
+ }
+ } else {
+ if (includeNone) {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ } else {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ }
+ }
+ }
+
+ read(accessor: (result: any, index: number, v: any) => void, createCollection: (len: number) => any, fromRef: boolean): any {
+ const flags = this.fury.binaryReader.uint8();
+ const isSame = !(flags & CollectionFlags.NOT_SAME_TYPE);
+ const includeNone = flags & CollectionFlags.HAS_NULL;
+ if (isSame) {
+ const serializer = this.fury.classResolver.getSerializerById(this.fury.binaryReader.int16());
+ const len = this.fury.binaryReader.varUInt32();
+ const result = createCollection(len);
+ if (fromRef) {
+ this.fury.referenceResolver.reference(result);
+ }
+ if (!includeNone) {
+ for (let index = 0; index < len; index++) {
+ accessor(result, index, serializer.read());
+ }
+ } else {
+ for (let index = 0; index < len; index++) {
+ const refFlag = this.fury.binaryReader.uint8();
+ if (RefFlags.NullFlag === refFlag) {
+ accessor(result, index, null);
+ } else {
+ accessor(result, index, serializer.read());
+ }
+ }
+ }
+ return result;
+ } else {
+ const len = this.fury.binaryReader.varUInt32();
+ const result = createCollection(len);
+ if (!includeNone) {
+ for (let index = 0; index < len; index++) {
+ const serializer = this.fury.classResolver.getSerializerById(this.fury.binaryReader.int16());
+ accessor(result, index, serializer.read());
+ }
+ } else {
+ for (let index = 0; index < len; index++) {
+ const refFlag = this.fury.binaryReader.uint8();
+ if (RefFlags.NullFlag === refFlag) {
+ accessor(result, index, null);
+ } else {
+ const serializer = this.fury.classResolver.getSerializerById(this.fury.binaryReader.int16());
+ accessor(result, index, serializer.read());
+ }
+ }
+ }
+ return result;
+ }
Review Comment:
Same situation above
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451381679
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,365 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
+ if (serializer !== null && current !== null && current !== serializer) {
+ isSame = false;
+ }
+ if (current !== null) {
+ serializer = current;
+ }
+ }
+ }
+
+ if (!isSame) {
+ flag |= CollectionFlags.NOT_SAME_TYPE;
+ }
+ if (includeNone) {
+ flag |= CollectionFlags.HAS_NULL;
+ }
+ if (serializer !== null && serializer.meta.needToWriteRef) {
+ flag |= CollectionFlags.TRACKING_REF;
+ }
+ this.fury.binaryWriter.uint8(flag);
+ if (isSame) {
+ if (serializer) {
+ this.fury.binaryWriter.int16(serializer.meta.type);
+ } else {
+ this.fury.binaryWriter.skip(2);
+ }
+ }
+ return {
+ serializer,
+ isSame,
+ flag,
+ includeNone,
+ };
+ }
+
+ write(value: any, size: number) {
+ const { serializer, isSame, includeNone } = this.writeElementsHeader(value);
+ this.fury.binaryWriter.varUInt32(size);
+ if (isSame) {
+ if (!includeNone) {
+ for (const item of value) {
+ serializer!.write(item);
+ }
+ return;
+ } else {
+ for (const item of value) {
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ serializer!.write(item);
+ }
+ }
+ } else {
+ if (includeNone) {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ } else {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ }
+ }
Review Comment:
Whether to write the 2-byte type and the 1-byte refflag is determined by whether the element contains null or is entirely of the same type. I can't think of a better way to simplify this without avoiding the determination of 'isSame' and 'includeNone' on each iteration.
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451390127
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,364 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
Review Comment:
I will add a collection snapshot for you to review the generated code.
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451968978
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,365 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
+ if (serializer !== null && current !== null && current !== serializer) {
+ isSame = false;
+ }
+ if (current !== null) {
+ serializer = current;
+ }
+ }
+ }
+
+ if (!isSame) {
+ flag |= CollectionFlags.NOT_SAME_TYPE;
+ }
+ if (includeNone) {
+ flag |= CollectionFlags.HAS_NULL;
+ }
+ if (serializer !== null && serializer.meta.needToWriteRef) {
+ flag |= CollectionFlags.TRACKING_REF;
+ }
+ this.fury.binaryWriter.uint8(flag);
+ if (isSame) {
+ if (serializer) {
+ this.fury.binaryWriter.int16(serializer.meta.type);
+ } else {
+ this.fury.binaryWriter.skip(2);
+ }
+ }
+ return {
+ serializer,
+ isSame,
+ flag,
+ includeNone,
+ };
+ }
+
+ write(value: any, size: number) {
+ const { serializer, isSame, includeNone } = this.writeElementsHeader(value);
+ this.fury.binaryWriter.varUInt32(size);
+ if (isSame) {
+ if (!includeNone) {
+ for (const item of value) {
+ serializer!.write(item);
+ }
+ return;
+ } else {
+ for (const item of value) {
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ serializer!.write(item);
+ }
+ }
+ } else {
+ if (includeNone) {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ } else {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ }
+ }
Review Comment:
LGTM. Although this introduces `isSame` and `includeNone` check overhead for every element, but compared to other memory writing overhead, I think the overhead acceptable
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1449905365
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,365 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
+ if (serializer !== null && current !== null && current !== serializer) {
+ isSame = false;
+ }
+ if (current !== null) {
+ serializer = current;
+ }
+ }
+ }
+
+ if (!isSame) {
+ flag |= CollectionFlags.NOT_SAME_TYPE;
+ }
+ if (includeNone) {
+ flag |= CollectionFlags.HAS_NULL;
+ }
+ if (serializer !== null && serializer.meta.needToWriteRef) {
+ flag |= CollectionFlags.TRACKING_REF;
+ }
+ this.fury.binaryWriter.uint8(flag);
+ if (isSame) {
+ if (serializer) {
+ this.fury.binaryWriter.int16(serializer.meta.type);
+ } else {
+ this.fury.binaryWriter.skip(2);
+ }
+ }
+ return {
+ serializer,
+ isSame,
+ flag,
+ includeNone,
+ };
+ }
+
+ write(value: any, size: number) {
+ const { serializer, isSame, includeNone } = this.writeElementsHeader(value);
+ this.fury.binaryWriter.varUInt32(size);
+ if (isSame) {
+ if (!includeNone) {
+ for (const item of value) {
+ serializer!.write(item);
+ }
+ return;
+ } else {
+ for (const item of value) {
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ serializer!.write(item);
+ }
+ }
+ } else {
+ if (includeNone) {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ } else {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ }
+ }
Review Comment:
Indeed, there is a lot of duplicate code; I'll think about how to simplify it.
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451383062
##########
javascript/test/__snapshots__/codeGen.test.ts.snap:
##########
@@ -8,13 +8,13 @@ return function(fury, external) {
const bw = fury.binaryWriter;
const cr = fury.classResolver;
const rr = fury.referenceResolver;
- const tag_ser_3 = fury.classResolver.getSerializerByTag("example.foo.1");
- const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.2");
- const tag_ser_12 = fury.classResolver.getSerializerByTag("example.bar.1");
- const tag_ser_13 = fury.classResolver.getSerializerByTag("example.bar.2");
- const tagWriter_19 = cr.createTagWriter("tuple-object-wrapper");
+ const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.1");
Review Comment:
Also used for reviewing the generated code on GitHub and tracing the code changes with Git.
##########
javascript/test/__snapshots__/codeGen.test.ts.snap:
##########
@@ -8,13 +8,13 @@ return function(fury, external) {
const bw = fury.binaryWriter;
const cr = fury.classResolver;
const rr = fury.referenceResolver;
- const tag_ser_3 = fury.classResolver.getSerializerByTag("example.foo.1");
- const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.2");
- const tag_ser_12 = fury.classResolver.getSerializerByTag("example.bar.1");
- const tag_ser_13 = fury.classResolver.getSerializerByTag("example.bar.2");
- const tagWriter_19 = cr.createTagWriter("tuple-object-wrapper");
+ const tag_ser_4 = fury.classResolver.getSerializerByTag("example.foo.1");
Review Comment:
Also used for reviewing the generated code on GitHub and tracing the code changes with Git.
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "theweipeng (via GitHub)" <gi...@apache.org>.
theweipeng commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451393314
##########
javascript/test/__snapshots__/collection.test.ts.snap:
##########
@@ -0,0 +1,159 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`collection can generate collection declaration code 1`] = `
+"function anonymous(
+) {
+return function(fury, external) {
+ const br = fury.binaryReader;
+ const bw = fury.binaryWriter;
+ const cr = fury.classResolver;
+ const rr = fury.referenceResolver;
+
+ const readInner = (fromRef) => {
+
+ const flags_2 = br.uint8();
+ br.skip(2);
+ const len_1 = br.varUInt32();
+ const result_0 = new Array(len_1);
+
+ if (fromRef) {
+ rr.reference(result_0)
+ }
+
+ if (flags_2 & 1) {
Review Comment:
@chaokunyang This snapshot is generated by a typed collection declared with `Type.array(Type.string())`, which is equivalent to `Array<String>` in Java.
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(JavaScript): Implement the collection protocol [incubator-fury]
Posted by "LiangliangSui (via GitHub)" <gi...@apache.org>.
LiangliangSui commented on code in PR #1337:
URL: https://github.com/apache/incubator-fury/pull/1337#discussion_r1451453619
##########
javascript/packages/fury/lib/gen/collection.ts:
##########
@@ -0,0 +1,365 @@
+/*
+ * 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 { TypeDescription } from "../description";
+import { CodecBuilder } from "./builder";
+import { BaseSerializerGenerator, RefState } from "./serializer";
+import { CodegenRegistry } from "./router";
+import { InternalSerializerType, RefFlags, Serializer } from "../type";
+import { Scope } from "./scope";
+import Fury from "../fury";
+
+export const CollectionFlags = {
+ /** Whether track elements ref. */
+ TRACKING_REF: 0b1,
+
+ /** Whether collection has null. */
+ HAS_NULL: 0b10,
+
+ /** Whether collection elements type is not declare type. */
+ NOT_DECL_ELEMENT_TYPE: 0b100,
+
+ /** Whether collection elements type different. */
+ NOT_SAME_TYPE: 0b1000,
+};
+
+class CollectionAnySerializer {
+ constructor(private fury: Fury) {
+
+ }
+
+ protected writeElementsHeader(arr: any) {
+ let flag = 0;
+ let isSame = true;
+ let serializer: Serializer | null = null;
+ let includeNone = false;
+
+ for (const item of arr) {
+ if ((item === undefined || item === null) && !includeNone) {
+ includeNone = true;
+ }
+ if (isSame) {
+ const current = this.fury.classResolver.getSerializerByData(item);
+ if (serializer !== null && current !== null && current !== serializer) {
+ isSame = false;
+ }
+ if (current !== null) {
+ serializer = current;
+ }
+ }
+ }
+
+ if (!isSame) {
+ flag |= CollectionFlags.NOT_SAME_TYPE;
+ }
+ if (includeNone) {
+ flag |= CollectionFlags.HAS_NULL;
+ }
+ if (serializer !== null && serializer.meta.needToWriteRef) {
+ flag |= CollectionFlags.TRACKING_REF;
+ }
+ this.fury.binaryWriter.uint8(flag);
+ if (isSame) {
+ if (serializer) {
+ this.fury.binaryWriter.int16(serializer.meta.type);
+ } else {
+ this.fury.binaryWriter.skip(2);
+ }
+ }
+ return {
+ serializer,
+ isSame,
+ flag,
+ includeNone,
+ };
+ }
+
+ write(value: any, size: number) {
+ const { serializer, isSame, includeNone } = this.writeElementsHeader(value);
+ this.fury.binaryWriter.varUInt32(size);
+ if (isSame) {
+ if (!includeNone) {
+ for (const item of value) {
+ serializer!.write(item);
+ }
+ return;
+ } else {
+ for (const item of value) {
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ serializer!.write(item);
+ }
+ }
+ } else {
+ if (includeNone) {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ if (item === null || item === undefined) {
+ this.fury.binaryWriter.uint8(RefFlags.NullFlag);
+ continue;
+ }
+ this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ } else {
+ for (const item of value) {
+ const serializer = this.fury.classResolver.getSerializerByData(item);
+ this.fury.binaryWriter.uint16(serializer!.meta.type);
+ serializer!.write(item);
+ }
+ }
+ }
Review Comment:
```javascript
for (const item of value) {
if (includeNone) {
if (item === null || item === undefined) {
this.fury.binaryWriter.uint8(RefFlags.NullFlag);
continue;
}
this.fury.binaryWriter.uint8(RefFlags.NotNullValueFlag);
}
let finalSerializer = serializer;
if (!isSame) {
finalSerializer = this.fury.classResolver.getSerializerByData(item);
this.fury.binaryWriter.uint16(finalSerializer!.meta.type);
}
finalSerializer!.write(item);
}
```
Can it become like this?
--
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: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org