You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "GWphua (via GitHub)" <gi...@apache.org> on 2025/08/29 10:46:12 UTC
[PR] Optimize ObjectMapper reading in ChangeRequestHttpSyncer (druid)
GWphua opened a new pull request, #18457:
URL: https://github.com/apache/druid/pull/18457
### Description
#### Speed up `ChangeRequestHttpSyncer#onSuccess` by caching ObjectMapper's JavaType
- Cache JavaType from provided TypeReference, so ObjectMapper no longer needs to call `constructType` everytime it tries to read from the stream.
- We can even consider making `ObjectMapper#readValue(InputStream, TypeReference<T>)` a forbidden API, given that we can use the more efficient `ObjectMapper#readValue(InputStream, JavaType)`.
- No behavior change; request count and APIs remain the same.
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
Internal deserialization optimization in `ChangeRequestHttpSyncer`
<hr>
##### Key changed/added classes in this PR
* `ChangeRequestHttpSyncer.java`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [x] a release note entry in the PR description.
- [x] been tested in a test Druid cluster.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Optimize ObjectMapper reading in ChangeRequestHttpSyncer (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on PR #18457:
URL: https://github.com/apache/druid/pull/18457#issuecomment-3239143184
Based on the document of Jackson, such change is reasonable but if there's specific data that shows the improvement, it would be better.
And one more thing is, the readValue is widely used in the repo, if we think that the performance matters, maybe we need to forbid current API to use the one that accepts JavaType
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Optimize ObjectMapper reading in ChangeRequestHttpSyncer (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #18457:
URL: https://github.com/apache/druid/pull/18457#issuecomment-3238959269
@GWphua , do you have a benchmark on the perf improvement this change provides?
Wondering if this is something we should do across the board, particularly while reading things from the DB.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Optimize ObjectMapper reading in ChangeRequestHttpSyncer (druid)
Posted by "GWphua (via GitHub)" <gi...@apache.org>.
GWphua commented on PR #18457:
URL: https://github.com/apache/druid/pull/18457#issuecomment-3241092753
@kfaraz @FrankChen021
I did a benchmark for 50,000 accesses of `readValue()`, and the benchmark favours using `JavaType` over `TypeReference`. Furthermore, after referencing [Jackson best practices](https://github.com/FasterXML/jackson-docs/wiki/Presentation:-Jackson-Performance), I have tried to benchmark for `ObjectReader`, and the performance does better. I think we can consider using `ObjectReader` in favour of `ObjectMapper` in the future.
```
# JMH version: 1.21
# VM version: JDK 17.0.12, Java HotSpot(TM) 64-Bit Server VM, 17.0.12+8-LTS-286
# VM invoker: /Library/Java/JavaVirtualMachines/jdk-17.jdk/Contents/Home/bin/java
# VM options: <none>
# Warmup: 2 iterations, 10 s each
# Measurement: 3 iterations, 10 s each
# Timeout: 10 min per iteration
# Threads: 1 thread, will synchronize iterations
# Benchmark mode: Average time, time/op
Benchmark Mode Cnt Score Error Units
ChangeRequestDeserBenchmark.cached_javaType_50k_calls avgt 3 11157.673 ± 566.225 us/op
ChangeRequestDeserBenchmark.cached_objectReader_50k_calls avgt 3 10599.758 ± 338.292 us/op
ChangeRequestDeserBenchmark.typeReference_50k_calls avgt 3 11723.273 ± 1115.112 us/op
```
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Optimize ObjectMapper reading in ChangeRequestHttpSyncer (druid)
Posted by "GWphua (via GitHub)" <gi...@apache.org>.
GWphua merged PR #18457:
URL: https://github.com/apache/druid/pull/18457
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Optimize ObjectMapper reading in ChangeRequestHttpSyncer (druid)
Posted by "GWphua (via GitHub)" <gi...@apache.org>.
GWphua commented on PR #18457:
URL: https://github.com/apache/druid/pull/18457#issuecomment-3252894496
Yea, you are right about the lack of performance improvement @kfaraz
From my flame graph below, I thought the performance to be because of the JsonMapper spending a lot of time to break down the type. Though, i found the bulk of the bottleneck is caused by Interval deserialization, which is addressed in #18477
<img width="1920" height="958" alt="image" src="https://github.com/user-attachments/assets/159ca5d5-6537-44b1-863f-6c680ff38198" />
Still, its nice to see that we can get a performance enhancement via using JavaType / ObjectReader. We can merge this PR, especially since this path is called a lot especially for larger clusters.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Optimize ObjectMapper reading in ChangeRequestHttpSyncer (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #18457:
URL: https://github.com/apache/druid/pull/18457#issuecomment-3251689407
Thanks for sharing the benchmarks, @GWphua !
I am sorry but I fail to see a significant improvement, I must be missing something.
In the second dataset, is the perf improvement when replacing `TypeReference` with `JavaType` from 12 ms to 11 ms per operation?
We can merge off this PR if needed but I don't feel that it is really worth the effort right now to change all call sites that use `TypeReference`.
To really attack the perf bottlenecks (either querying, ingestion or management), we can look at flamegraphs of services and specific threads to see where we are spending most of our time.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Optimize ObjectMapper reading in ChangeRequestHttpSyncer (druid)
Posted by "GWphua (via GitHub)" <gi...@apache.org>.
GWphua commented on PR #18457:
URL: https://github.com/apache/druid/pull/18457#issuecomment-3244538009
A further update on the performances, these are the results from 50,000 accesses of readValue() on a small object (~10 characters of string), using a normal mapper (Results for SmileMapper above). While Jackson JR is the most performant, it might be complex to set up.
We can either look to using `JavaType`, or ObjectReader... They bring some performance boosts for the simplicity of change.
```
Benchmark Mode Cnt Score Error Units
JsonLibrarySerializationBenchmark.jacksonJr_json avgt 30 10779.843 ± 200.994 us/op
JsonLibrarySerializationBenchmark.javaType_json avgt 30 11279.839 ± 368.868 us/op
JsonLibrarySerializationBenchmark.javaType_json_afterburner avgt 30 11080.339 ± 310.208 us/op
JsonLibrarySerializationBenchmark.objectReader_json avgt 30 11519.170 ± 46.703 us/op
JsonLibrarySerializationBenchmark.objectReader_json_afterburner avgt 30 11341.113 ± 183.754 us/op
JsonLibrarySerializationBenchmark.typeReference_json avgt 30 12168.292 ± 35.448 us/op
```
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org