Re: [PR] feat(java): optimize read float/double for jvm jit inline [incubator-fury]
chaokunyang commented on code in PR #1472:
URL: https://github.com/apache/incubator-fury/pull/1472#discussion_r1554641365
##
java/fury-core/src/test/java/org/apache/fury/serializer/PrimitiveSerializersTest.java:
##
@@ -54,4 +59,62 @@ public void testUint16Serializer() {
assertThrows(IllegalArgumentException.class, () ->
serializer.xwrite(buffer, -1));
assertThrows(IllegalArgumentException.class, () ->
serializer.xwrite(buffer, 65536));
}
+
+ @Data
+ @AllArgsConstructor
+ public static class PrimitiveStruct {
Review Comment:
Private will be better. Fury don't support private class codegen previously,
so I used the public class in tests.
--
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-unsubscr...@fury.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@fury.apache.org
For additional commands, e-mail: commits-h...@fury.apache.org
Re: [PR] feat(java): optimize read float/double for jvm jit inline [incubator-fury]
chaokunyang commented on PR #1472: URL: https://github.com/apache/incubator-fury/pull/1472#issuecomment-2041145222 Here is the benchmark result: ``` With This PR: Benchmark (bufferType) (objectType) (references) Mode CntScoreError Units UserTypeDeserializeSuite.fury_deserialize array Struct false thrpt 10 4466936.113 ± 386617.618 ops/s UserTypeDeserializeSuite.fury_deserialize array Struct true thrpt 10 3963160.097 ± 178393.730 ops/s UserTypeDeserializeSuite.fury_deserialize directBuffer Struct false thrpt 10 4879409.124 ± 79998.330 ops/s UserTypeDeserializeSuite.fury_deserialize directBuffer Struct true thrpt 10 4140782.389 ± 95492.701 ops/s Before this PR: Benchmark (bufferType) (objectType) (references) Mode CntScoreError Units UserTypeDeserializeSuite.fury_deserialize array Struct false thrpt 10 4135698.223 ± 344998.039 ops/s UserTypeDeserializeSuite.fury_deserialize array Struct true thrpt 10 4062397.644 ± 94063.642 ops/s UserTypeDeserializeSuite.fury_deserialize directBuffer Struct false thrpt 10 4311519.019 ± 251129.086 ops/s UserTypeDeserializeSuite.fury_deserialize directBuffer Struct true thrpt 10 4056600.395 ± 414134.048 ops/s ``` -- 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-unsubscr...@fury.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@fury.apache.org For additional commands, e-mail: commits-h...@fury.apache.org
Re: [PR] feat(java): optimize read float/double for jvm jit inline [incubator-fury]
chaokunyang commented on PR #1472: URL: https://github.com/apache/incubator-fury/pull/1472#issuecomment-2041140777 > Hi @chaokunyang , I have a suggestion. For this kind of optimization PR, we should force the submitter to provide a benchmark data in the PR, so that we can ensure that the optimization is beneficial to the codebase. > > Then for the PR of the optimization category, we should force the PR title to indicate that it is an optimization PR (standardizing the Fury community). We can add a new category `opt` in [pr-lint.yml](https://github.com/apache/incubator-fury/blob/main/.github/workflows/pr-lint.yml), and I will submit a PR to do this. How abouting add a `perf` category, as described in https://platform.uno/docs/articles/uno-development/git-conventional-commits.html. `opt` can represent many things, like `refactor` -- 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-unsubscr...@fury.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@fury.apache.org For additional commands, e-mail: commits-h...@fury.apache.org
Re: [PR] feat(java): optimize read float/double for jvm jit inline [incubator-fury]
chaokunyang commented on code in PR #1472:
URL: https://github.com/apache/incubator-fury/pull/1472#discussion_r1554641365
##
java/fury-core/src/test/java/org/apache/fury/serializer/PrimitiveSerializersTest.java:
##
@@ -54,4 +59,62 @@ public void testUint16Serializer() {
assertThrows(IllegalArgumentException.class, () ->
serializer.xwrite(buffer, -1));
assertThrows(IllegalArgumentException.class, () ->
serializer.xwrite(buffer, 65536));
}
+
+ @Data
+ @AllArgsConstructor
+ public static class PrimitiveStruct {
Review Comment:
Private will work here too, but the generate code would be different.
Codegen work best for public classes
--
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-unsubscr...@fury.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@fury.apache.org
For additional commands, e-mail: commits-h...@fury.apache.org
Re: [PR] feat(java): optimize read float/double for jvm jit inline [incubator-fury]
chaokunyang commented on code in PR #1472:
URL: https://github.com/apache/incubator-fury/pull/1472#discussion_r1554641104
##
java/fury-core/src/main/java/org/apache/fury/memory/MemoryBuffer.java:
##
@@ -2318,6 +2205,37 @@ public float readFloat() {
}
}
+ // Reduce method body for better inline in the caller.
+ @CodegenInvoke
+ public float readFloatOnLE() {
+int readerIdx = readerIndex;
+// use subtract to avoid overflow
+int remaining = size - readerIdx;
+if (remaining < 4) {
+ throw new IndexOutOfBoundsException(
Review Comment:
This is temprary , it will be replaced by `streamReader.fillBuffer` in
#1451. The exception will be trown in `DoundChecker`
--
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-unsubscr...@fury.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@fury.apache.org
For additional commands, e-mail: commits-h...@fury.apache.org
Re: [PR] feat(java): optimize read float/double for jvm jit inline [incubator-fury]
LiangliangSui commented on PR #1472: URL: https://github.com/apache/incubator-fury/pull/1472#issuecomment-204294 Hi @chaokunyang , I have a suggestion. For this kind of optimization PR, we should force the submitter to provide a benchmark data in the PR, so that we can ensure that the optimization is beneficial to the codebase. Then for the PR of the optimization category, we should force the PR title to indicate that it is an optimization PR (standardizing the Fury community). We can add a new category `opt` in [pr-lint.yml](https://github.com/apache/incubator-fury/blob/main/.github/workflows/pr-lint.yml), and I will submit a PR to do 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-unsubscr...@fury.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@fury.apache.org For additional commands, e-mail: commits-h...@fury.apache.org
Re: [PR] feat(java): optimize read float/double for jvm jit inline [incubator-fury]
LiangliangSui commented on code in PR #1472:
URL: https://github.com/apache/incubator-fury/pull/1472#discussion_r1554623191
##
java/fury-core/src/test/java/org/apache/fury/serializer/PrimitiveSerializersTest.java:
##
@@ -54,4 +59,62 @@ public void testUint16Serializer() {
assertThrows(IllegalArgumentException.class, () ->
serializer.xwrite(buffer, -1));
assertThrows(IllegalArgumentException.class, () ->
serializer.xwrite(buffer, 65536));
}
+
+ @Data
+ @AllArgsConstructor
+ public static class PrimitiveStruct {
+byte byte1;
+byte byte2;
+char char1;
+char char2;
+short short1;
+short short2;
+int int1;
+int int2;
+long long1;
+long long2;
+float float1;
+float float2;
+double double1;
+double double2;
+ }
+
+ @Test(dataProvider = "compressNumberAndCodeGen")
+ public void testPrimitiveStruct(boolean compressNumber, boolean codegen) {
+PrimitiveStruct struct =
+new PrimitiveStruct(
+Byte.MIN_VALUE,
+Byte.MIN_VALUE,
+Character.MIN_VALUE,
+Character.MIN_VALUE,
+Short.MIN_VALUE,
+Short.MIN_VALUE,
+Integer.MIN_VALUE,
+Integer.MIN_VALUE,
+Long.MIN_VALUE,
+Long.MIN_VALUE,
+Float.MIN_VALUE,
+Float.MIN_VALUE,
+Double.MIN_VALUE,
+Double.MIN_VALUE);
+if (compressNumber) {
+ FuryBuilder builder =
Review Comment:
Nit: We can move the common part of FuryBuilder before the if branch, so
that if and else can share this part
##
java/fury-core/src/test/java/org/apache/fury/serializer/PrimitiveSerializersTest.java:
##
@@ -54,4 +59,62 @@ public void testUint16Serializer() {
assertThrows(IllegalArgumentException.class, () ->
serializer.xwrite(buffer, -1));
assertThrows(IllegalArgumentException.class, () ->
serializer.xwrite(buffer, 65536));
}
+
+ @Data
+ @AllArgsConstructor
+ public static class PrimitiveStruct {
Review Comment:
Should be private.
```java
private static class PrimitiveStruct
```
##
java/fury-core/src/main/java/org/apache/fury/memory/MemoryBuffer.java:
##
@@ -2318,6 +2205,37 @@ public float readFloat() {
}
}
+ // Reduce method body for better inline in the caller.
+ @CodegenInvoke
+ public float readFloatOnLE() {
+int readerIdx = readerIndex;
+// use subtract to avoid overflow
+int remaining = size - readerIdx;
+if (remaining < 4) {
+ throw new IndexOutOfBoundsException(
Review Comment:
Nit: we can reuse
[`throwIndexOutOfBoundsException`](https://github.com/apache/incubator-fury/blob/main/java/fury-core/src/main/java/org/apache/fury/memory/MemoryBuffer.java#L1853-L1857).
`throwIndexOutOfBoundsException` is not executed frequently and should have no
impact on inlining.
--
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-unsubscr...@fury.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@fury.apache.org
For additional commands, e-mail: commits-h...@fury.apache.org
Re: [PR] feat(java): optimize read float/double for jvm jit inline [incubator-fury]
chaokunyang merged PR #1472: URL: https://github.com/apache/incubator-fury/pull/1472 -- 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-unsubscr...@fury.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@fury.apache.org For additional commands, e-mail: commits-h...@fury.apache.org
[PR] feat(java): optimize read float/double for jvm jit inline [incubator-fury]
chaokunyang opened a new pull request, #1472: URL: https://github.com/apache/incubator-fury/pull/1472 (no comment) -- 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-unsubscr...@fury.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@fury.apache.org For additional commands, e-mail: commits-h...@fury.apache.org
