You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fury.apache.org by "jayhan94 (via GitHub)" <gi...@apache.org> on 2025/02/13 02:55:53 UTC
[PR] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
jayhan94 opened a new pull request, #2057:
URL: https://github.com/apache/fury/pull/2057
<!--
**Thanks for contributing to Fury.**
**If this is your first time opening a PR on fury, you can refer to [CONTRIBUTING.md](https://github.com/apache/fury/blob/main/CONTRIBUTING.md).**
Contribution Checklist
- The **Apache Fury (incubating)** community has restrictions on the naming of pr titles. You can also find instructions in [CONTRIBUTING.md](https://github.com/apache/fury/blob/main/CONTRIBUTING.md).
- Fury has a strong focus on performance. If the PR you submit will have an impact on performance, please benchmark it first and provide the benchmark result here.
-->
## What does this PR do?
Add fast path for the serialization of `Map<String, String>`.
## Related issues
Closes #2026
## Does this PR introduce any user-facing change?
None
## Benchmark
Not yet, just a draft pr now.
--
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] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
Posted by "jayhan94 (via GitHub)" <gi...@apache.org>.
jayhan94 commented on PR #2057:
URL: https://github.com/apache/fury/pull/2057#issuecomment-2658179924
Hmm, I'm having trouble with the benchmark. I'm not sure whether the StringMapSerializationSuite is valid, but the results indicate that this PR introduces a significant performance regression.
```
Before:
Benchmark (mapSize) (stringRatio) Mode Cnt Score Error Units
StringMapSerializationSuite.deserialize 50 0.0 thrpt 5 1199813.656 ± 56659.770 ops/s
StringMapSerializationSuite.deserialize 50 0.2 thrpt 5 1082390.191 ± 22192.017 ops/s
StringMapSerializationSuite.deserialize 50 0.4 thrpt 5 988205.472 ± 10120.216 ops/s
StringMapSerializationSuite.deserialize 50 0.6 thrpt 5 819745.922 ± 14640.016 ops/s
StringMapSerializationSuite.deserialize 50 0.8 thrpt 5 761238.764 ± 41274.025 ops/s
StringMapSerializationSuite.deserialize 50 1.0 thrpt 5 818521.281 ± 42637.267 ops/s
StringMapSerializationSuite.serialize 50 0.0 thrpt 5 1313400.596 ± 41120.713 ops/s
StringMapSerializationSuite.serialize 50 0.2 thrpt 5 1197028.082 ± 38588.963 ops/s
StringMapSerializationSuite.serialize 50 0.4 thrpt 5 1226897.521 ± 28125.905 ops/s
StringMapSerializationSuite.serialize 50 0.6 thrpt 5 1137013.744 ± 54707.471 ops/s
StringMapSerializationSuite.serialize 50 0.8 thrpt 5 1094282.261 ± 12766.788 ops/s
StringMapSerializationSuite.serialize 50 1.0 thrpt 5 1330620.243 ± 37696.001 ops/s
After:
Benchmark (mapSize) (stringRatio) Mode Cnt Score Error Units
StringMapSerializationSuite.deserialize 50 0.0 thrpt 5 1013888.818 ± 24478.168 ops/s
StringMapSerializationSuite.deserialize 50 0.2 thrpt 5 784407.869 ± 40987.059 ops/s
StringMapSerializationSuite.deserialize 50 0.4 thrpt 5 762941.737 ± 36584.379 ops/s
StringMapSerializationSuite.deserialize 50 0.6 thrpt 5 875355.324 ± 7236.977 ops/s
StringMapSerializationSuite.deserialize 50 0.8 thrpt 5 765313.777 ± 16770.509 ops/s
StringMapSerializationSuite.deserialize 50 1.0 thrpt 5 854012.876 ± 8850.744 ops/s
StringMapSerializationSuite.serialize 50 0.0 thrpt 5 1377660.258 ± 13716.225 ops/s
StringMapSerializationSuite.serialize 50 0.2 thrpt 5 1250467.207 ± 6047.695 ops/s
StringMapSerializationSuite.serialize 50 0.4 thrpt 5 1162058.640 ± 5399.060 ops/s
StringMapSerializationSuite.serialize 50 0.6 thrpt 5 1213526.978 ± 4323.156 ops/s
StringMapSerializationSuite.serialize 50 0.8 thrpt 5 1021980.610 ± 28857.004 ops/s
StringMapSerializationSuite.serialize 50 1.0 thrpt 5 1280659.398 ± 2857.126 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-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] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
Posted by "jayhan94 (via GitHub)" <gi...@apache.org>.
jayhan94 commented on code in PR #2057:
URL: https://github.com/apache/fury/pull/2057#discussion_r1954306861
##########
java/fury-core/src/main/java/org/apache/fury/serializer/collection/AbstractMapSerializer.java:
##########
@@ -125,6 +126,32 @@ public void setValueSerializer(Serializer valueSerializer) {
this.valueSerializer = valueSerializer;
}
+ /**
+ * this method try to eliminate the virtual function call and help JIT inline for java String
+ * type.
+ */
+ private void tryFastSerialize(Serializer serializer, MemoryBuffer buffer, Object value) {
Review Comment:
This method is so small that it can be inlined easily.
--
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] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
Posted by "jayhan94 (via GitHub)" <gi...@apache.org>.
jayhan94 commented on PR #2057:
URL: https://github.com/apache/fury/pull/2057#issuecomment-2664577426
> For example, we have three map which have different type for key and value.
I add a benchmark like this, and the result shows this pr has a performance improvement.
```
Before:
Benchmark (mapSize) Mode Cnt Score Error Units
StringMapSerializationSuite.deserialize2 50 thrpt 5 368898.317 ± 21006.990 ops/s
StringMapSerializationSuite.serialize2 50 thrpt 5 590228.934 ± 21081.928 ops/s
After:
Benchmark (mapSize) Mode Cnt Score Error Units
StringMapSerializationSuite.deserialize2 50 thrpt 5 372951.992 ± 5156.001 ops/s
StringMapSerializationSuite.serialize2 50 thrpt 5 748249.524 ± 9676.674 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-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] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
Posted by "jayhan94 (via GitHub)" <gi...@apache.org>.
jayhan94 commented on PR #2057:
URL: https://github.com/apache/fury/pull/2057#issuecomment-2663402477
If the key-value pairs in the map are small (just a few bytes), the serialization overhead is mostly concentrated on the deserialization of some integers. I suspect these numbers represent metadata such as length, headers, etc. Their presence weakens the virtual function call optimization for the string serializer, and instead, the performance decreases due to branch prediction failures and type comparisons.
<img width="1103" alt="image" src="https://github.com/user-attachments/assets/193f4235-0952-4b92-8e43-c6bc38a67790" />
I tried increasing the length of the key-value pairs to 30 and 100 bytes (stringRatio=0.4), and the performance regression improved. However, the expected optimization didn’t occur.
Results of large kv
```
Before:
Benchmark (mapSize) (stringRatio) Mode Cnt Score Error Units
StringMapSerializationSuite.deserialize 50 0.4 thrpt 5 539543.330 ± 7888.817 ops/s
After:
Benchmark (mapSize) (stringRatio) Mode Cnt Score Error Units
StringMapSerializationSuite.deserialize 50 0.4 thrpt 5 513941.928 ± 21391.914 ops/s
```
@chaokunyang @pandalee99 Thanks for your review and patience. If you don't have different opinions, I’ll be closing this PR as it’s a failed change.
--
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] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
Posted by "jayhan94 (via GitHub)" <gi...@apache.org>.
jayhan94 commented on PR #2057:
URL: https://github.com/apache/fury/pull/2057#issuecomment-2655348813
This PR isn't ready yet. I'll be adding a benchmark later. Maintainers, feel free to review it at the meantime, in case I've misunderstood the proposal discussed in #2026.
--
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] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
Posted by "jayhan94 (via GitHub)" <gi...@apache.org>.
jayhan94 closed pull request #2057: perf(java): try to eliminate virtual function call for the kv serialization of Map<String, String>
URL: https://github.com/apache/fury/pull/2057
--
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] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2057:
URL: https://github.com/apache/fury/pull/2057#discussion_r1954301973
##########
java/fury-core/src/main/java/org/apache/fury/serializer/collection/AbstractMapSerializer.java:
##########
@@ -125,6 +126,32 @@ public void setValueSerializer(Serializer valueSerializer) {
this.valueSerializer = valueSerializer;
}
+ /**
+ * this method try to eliminate the virtual function call and help JIT inline for java String
+ * type.
+ */
+ private void tryFastSerialize(Serializer serializer, MemoryBuffer buffer, Object value) {
Review Comment:
Could we avoid create a new method?
--
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] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #2057:
URL: https://github.com/apache/fury/pull/2057#issuecomment-2663495348
Hi @jayhan94 , do you try to mix the different kv type of map in one benchmark? For example, we have three map which have different type for key and value. In such cases, the jvm inline won't work, and the fastpath will kick in.
--
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] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
Posted by "jayhan94 (via GitHub)" <gi...@apache.org>.
jayhan94 commented on PR #2057:
URL: https://github.com/apache/fury/pull/2057#issuecomment-2664461646
> Hi @jayhan94 , do you try to mix the different kv type of map in one benchmark? For example, we have three map which have different type for key and value. In such cases, the jvm inline won't work, and the fastpath will kick in.
I mixed different types of key-value pairs(long,int,string) in single map, and I think the result is similar to having different types of maps, which is why I got the outcome above. The benchmark code is in the `StringMapSerializationSuite`.
--
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] perf(java): try to eliminate virtual function call for the kv serialization of Map [fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #2057:
URL: https://github.com/apache/fury/pull/2057#discussion_r1954302535
##########
java/fury-core/src/main/java/org/apache/fury/serializer/collection/AbstractMapSerializer.java:
##########
@@ -125,6 +126,32 @@ public void setValueSerializer(Serializer valueSerializer) {
this.valueSerializer = valueSerializer;
}
+ /**
+ * this method try to eliminate the virtual function call and help JIT inline for java String
+ * type.
+ */
+ private void tryFastSerialize(Serializer serializer, MemoryBuffer buffer, Object value) {
+ if (serializer instanceof StringSerializer) {
Review Comment:
instanceof is slow in java, could we use serializer.getClass is StringSerializer instead?
--
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