You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/09/26 08:47:00 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #37999: [SPARK-39146][CORE] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
LuciferYang opened a new pull request, #37999:
URL: https://github.com/apache/spark/pull/37999
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #37999: [SPARK-39146][CORE][SQL] Introduce local singleton for `ObjectMapper` that may be reused
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1263047267
thanks @srowen
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #37999: [SPARK-39146][CORE][SQL][K8S] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1258141808
My concern is that ObjectMapper, while thread-safe, is synchronized in some methods, IIRC. This could introduce contention for locks. Is the perf win really compelling? I wonder if we can reuse ObjectMapper inside classes where it matters for perf and not try to share one instance so widely.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #37999: [SPARK-39146][CORE][SQL][K8S] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1259522776
> I wonder if we can reuse ObjectMapper inside classes where it matters for perf and not try to share one instance so widely.
According to this principle, it is enough to keep the changes in `RebaseDateTime`, `DataSourceV2Utils` and `FileDataSourceV2`, there is no possibility of reuse in other 3 files
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #37999: [SPARK-39146][CORE][SQL][K8S] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1259541627
I'm OK with the change. I guess I'd slightly prefer keeping the change more 'local' by having a singleton in each of the classes that need this. That minimizes the scope. I don't feel strongly about 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen closed pull request #37999: [SPARK-39146][CORE][SQL] Introduce local singleton for `ObjectMapper` that may be reused
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #37999: [SPARK-39146][CORE][SQL] Introduce local singleton for `ObjectMapper` that may be reused
URL: https://github.com/apache/spark/pull/37999
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #37999: [SPARK-39146][CORE][SQL] Introduce local singleton for `ObjectMapper` that may be reused
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1262216787
Merged to master
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #37999: [SPARK-39146][CORE][SQL][K8S] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1258157326
OK, let me add a multi thread comparison to check 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #37999: [SPARK-39146][CORE][SQL][K8S] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1259014121
I write a multi thread test as follows:
https://github.com/LuciferYang/spark/blob/fe26455668a904400e4b81ec696b7cd3abb3c923/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/MJacksonBenchmark.scala
```scala
def testWriteMapToJson(valuesPerIteration: Int, threads: Int): Unit = {
val map = Map("intValue" -> 1,
"longValue" -> 2L,
"doubleValue" -> 3.0D,
"stringValue" -> "4",
"floatValue" -> 5.0F,
"booleanValue" -> true)
val benchmark = new Benchmark(s"Test $threads threads write map to json",
valuesPerIteration, output = output)
val multi = Array.fill(threads)({
val ret = new ObjectMapper()
ret.registerModule(DefaultScalaModule)
ret
})
benchmark.addCase("Test use multi mapper") { _: Int =>
val latch = new CountDownLatch(valuesPerIteration)
val executor = ThreadUtils.newDaemonFixedThreadPool(threads, "multi")
for (i <- 0 until valuesPerIteration) {
executor.submit(new Runnable {
override def run(): Unit = {
val idx = i % threads
multi(idx).writeValueAsString(map)
latch.countDown()
}
})
}
latch.await()
executor.shutdown()
}
val mapper = new ObjectMapper()
mapper.registerModule(DefaultScalaModule)
val singleton = Array.fill(threads)(mapper)
benchmark.addCase("Test use singleton mapper") { _: Int =>
val latch = new CountDownLatch(valuesPerIteration)
val executor = ThreadUtils.newDaemonFixedThreadPool(threads, "singleton")
for (i <- 0 until valuesPerIteration) {
executor.submit(new Runnable {
override def run(): Unit = {
val idx = i % threads
singleton(idx).writeValueAsString(map)
latch.countDown()
}
})
}
latch.await()
executor.shutdown()
}
benchmark.run()
}
def testReadJsonToMap(valuesPerIteration: Int, threads: Int): Unit = {
val input = {
val map = Map("intValue" -> 1,
"longValue" -> 2L,
"doubleValue" -> 3.0D,
"stringValue" -> "4",
"floatValue" -> 5.0F,
"booleanValue" -> true)
val mapper = new ObjectMapper()
mapper.registerModule(DefaultScalaModule)
mapper.writeValueAsString(map)
}
val benchmark = new Benchmark(s"Test $threads threads read json to map",
valuesPerIteration, output = output)
val multi = Array.fill(threads)({
val ret = new ObjectMapper()
ret.registerModule(DefaultScalaModule)
ret
})
benchmark.addCase("Test use multi mapper") { _: Int =>
val latch = new CountDownLatch(valuesPerIteration)
val executor = ThreadUtils.newDaemonFixedThreadPool(threads, "multi")
for (i <- 0 until valuesPerIteration) {
executor.submit(new Runnable {
override def run(): Unit = {
val idx = i % threads
multi(idx).readValue(input, classOf[mutable.HashMap[String, String]])
latch.countDown()
}
})
}
latch.await()
executor.shutdown()
}
val mapper = new ObjectMapper()
mapper.registerModule(DefaultScalaModule)
val singleton = Array.fill(threads)(mapper)
benchmark.addCase("Test use singleton mapper") { _: Int =>
val latch = new CountDownLatch(valuesPerIteration)
val executor = ThreadUtils.newDaemonFixedThreadPool(threads, "singleton")
for (i <- 0 until valuesPerIteration) {
executor.submit(new Runnable {
override def run(): Unit = {
val idx = i % threads
singleton(idx).readValue(input, classOf[mutable.HashMap[String, String]])
latch.countDown()
}
})
}
latch.await()
executor.shutdown()
}
benchmark.run()
}
```
The result from GA as follows:
```
OpenJDK 64-Bit Server VM 1.8.0_345-b01 on Linux 5.15.0-1020-azure
Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz
Test 5 threads read json to map: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 1927 1959 46 0.5 1927.2 1.0X
Test use singleton mapper 1727 1884 222 0.6 1727.0 1.1X
OpenJDK 64-Bit Server VM 1.8.0_345-b01 on Linux 5.15.0-1020-azure
Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz
Test 10 threads read json to map: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 1411 1465 76 0.7 1411.2 1.0X
Test use singleton mapper 1297 1322 36 0.8 1296.7 1.1X
OpenJDK 64-Bit Server VM 1.8.0_345-b01 on Linux 5.15.0-1020-azure
Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz
Test 20 threads read json to map: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 1570 1598 41 0.6 1569.7 1.0X
Test use singleton mapper 1433 1455 31 0.7 1432.7 1.1X
OpenJDK 64-Bit Server VM 1.8.0_345-b01 on Linux 5.15.0-1020-azure
Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz
Test 5 threads write map to json: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 804 948 183 1.2 803.7 1.0X
Test use singleton mapper 778 954 167 1.3 777.7 1.0X
OpenJDK 64-Bit Server VM 1.8.0_345-b01 on Linux 5.15.0-1020-azure
Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz
Test 10 threads write map to json: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 784 865 75 1.3 784.0 1.0X
Test use singleton mapper 777 954 179 1.3 776.6 1.0X
OpenJDK 64-Bit Server VM 1.8.0_345-b01 on Linux 5.15.0-1020-azure
Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz
Test 20 threads write map to json: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 777 841 96 1.3 777.4 1.0X
Test use singleton mapper 782 932 134 1.3 782.1 1.0X
```
and I use a bare metal server to test more threads, the test result as follows:
```
OpenJDK 64-Bit Server VM 1.8.0_312-b07 on Linux 4.14.0_1-0-0-45
Intel(R) Xeon(R) Gold 6271C CPU @ 2.60GHz
Test 5 threads read json to map: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 1197 1338 200 0.8 1197.1 1.0X
Test use singleton mapper 1023 1277 360 1.0 1022.8 1.2X
OpenJDK 64-Bit Server VM 1.8.0_312-b07 on Linux 4.14.0_1-0-0-45
Intel(R) Xeon(R) Gold 6271C CPU @ 2.60GHz
Test 10 threads read json to map: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 1193 2305 1573 0.8 1193.0 1.0X
Test use singleton mapper 1048 1487 621 1.0 1047.6 1.1X
OpenJDK 64-Bit Server VM 1.8.0_312-b07 on Linux 4.14.0_1-0-0-45
Intel(R) Xeon(R) Gold 6271C CPU @ 2.60GHz
Test 20 threads read json to map: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 1113 1491 535 0.9 1113.3 1.0X
Test use singleton mapper 1351 1455 147 0.7 1351.0 0.8X
OpenJDK 64-Bit Server VM 1.8.0_312-b07 on Linux 4.14.0_1-0-0-45
Intel(R) Xeon(R) Gold 6271C CPU @ 2.60GHz
Test 50 threads read json to map: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 1003 1471 661 1.0 1003.3 1.0X
Test use singleton mapper 1000 1330 468 1.0 999.8 1.0X
OpenJDK 64-Bit Server VM 1.8.0_312-b07 on Linux 4.14.0_1-0-0-45
Intel(R) Xeon(R) Gold 6271C CPU @ 2.60GHz
Test 75 threads read json to map: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 1019 1345 461 1.0 1018.7 1.0X
Test use singleton mapper 1024 1241 306 1.0 1024.3 1.0X
OpenJDK 64-Bit Server VM 1.8.0_312-b07 on Linux 4.14.0_1-0-0-45
Intel(R) Xeon(R) Gold 6271C CPU @ 2.60GHz
Test 5 threads write map to json: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 842 1474 894 1.2 842.0 1.0X
Test use singleton mapper 959 1585 884 1.0 959.5 0.9X
OpenJDK 64-Bit Server VM 1.8.0_312-b07 on Linux 4.14.0_1-0-0-45
Intel(R) Xeon(R) Gold 6271C CPU @ 2.60GHz
Test 10 threads write map to json: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 1280 1844 797 0.8 1280.2 1.0X
Test use singleton mapper 889 1634 1054 1.1 888.5 1.4X
OpenJDK 64-Bit Server VM 1.8.0_312-b07 on Linux 4.14.0_1-0-0-45
Intel(R) Xeon(R) Gold 6271C CPU @ 2.60GHz
Test 20 threads write map to json: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 858 957 132 1.2 857.6 1.0X
Test use singleton mapper 896 1625 1031 1.1 895.6 1.0X
OpenJDK 64-Bit Server VM 1.8.0_312-b07 on Linux 4.14.0_1-0-0-45
Intel(R) Xeon(R) Gold 6271C CPU @ 2.60GHz
Test 50 threads write map to json: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 884 960 115 1.1 884.2 1.0X
Test use singleton mapper 843 1227 626 1.2 843.3 1.0X
OpenJDK 64-Bit Server VM 1.8.0_312-b07 on Linux 4.14.0_1-0-0-45
Intel(R) Xeon(R) Gold 6271C CPU @ 2.60GHz
Test 75 threads write map to json: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test use multi mapper 1094 1109 22 0.9 1093.7 1.0X
Test use singleton mapper 872 1257 658 1.1 871.5 1.3X
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #37999: [SPARK-39146][CORE][SQL][K8S] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1259031253
@srowen From the above test results, there is no significant performance difference between using global and local singletons.
From a code perspective, thread safety should not be guaranteed by locks, it seems guaranteed by using a new `JsonParser/JsonGenerator` every serialization and deserialization.
https://github.com/FasterXML/jackson-databind/blob/492334da383b0677c19b08964fddad18230546af/src/main/java/com/fasterxml/jackson/databind/ObjectMapper.java#L3838-L3851
<img width="949" alt="image" src="https://user-images.githubusercontent.com/1475305/192445439-80fc6740-767f-4848-b444-f6c22a40d1cc.png">
https://github.com/FasterXML/jackson-databind/blob/492334da383b0677c19b08964fddad18230546af/src/main/java/com/fasterxml/jackson/databind/ObjectMapper.java#L3647-L3658
<img width="885" alt="image" src="https://user-images.githubusercontent.com/1475305/192445680-6c2065ff-7cf4-4d60-8c01-df219eb629ab.png">
So there seems to be no problem with the global instance. `ObjectMapper` is only used as a configuration template, so I am both ok to use a global singleton or local singleton
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #37999: [SPARK-39146][CORE][SQL][K8S] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1259433386
To be clear I was saying it _is already thread-safe_ but the issue could be lock contention. It may not be an issue in Spark, and/or fixed in Jackson, but I'm looking at posts like https://medium.com/feedzaitech/when-jackson-becomes-a-parallelism-bottleneck-f1440a50b429
I agree it doesn't look like there is evidence of contention here, so probably worth the perf improvement.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #37999: [SPARK-39146][CORE][SQL] Introduce local singleton for `ObjectMapper` that may be reused
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1262125639
GA passed
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #37999: [SPARK-39146][CORE][SQL]Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #37999:
URL: https://github.com/apache/spark/pull/37999#discussion_r981359490
##########
core/src/main/scala/org/apache/spark/ErrorClassesJSONReader.scala:
##########
@@ -39,11 +38,10 @@ import org.apache.spark.annotation.DeveloperApi
class ErrorClassesJsonReader(jsonFileURLs: Seq[URL]) {
assert(jsonFileURLs.nonEmpty)
+ private lazy val mapper = Utils.withScalaModuleMapper
Review Comment:
WOuldn't we want these in a companion object?
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -1966,6 +1969,16 @@ private[spark] object Utils extends Logging {
getHadoopFileSystem(new URI(path), conf)
}
+ /**
+ * Return a new `ObjectMapper` with `ClassTagExtensions`.
+ * The mapper registers `DefaultScalaModule` by default.
+ */
+ def withScalaModuleMapper: ObjectMapper with ClassTagExtensions = {
Review Comment:
OK, don't know if we need to factor out a utility method just for this, I'd be OK inlining this in the ~3 places it is used.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #37999: [SPARK-39146][CORE][SQL] Introduce local singleton for `ObjectMapper` that may be reused
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37999:
URL: https://github.com/apache/spark/pull/37999#discussion_r981438459
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -1966,6 +1969,16 @@ private[spark] object Utils extends Logging {
getHadoopFileSystem(new URI(path), conf)
}
+ /**
+ * Return a new `ObjectMapper` with `ClassTagExtensions`.
+ * The mapper registers `DefaultScalaModule` by default.
+ */
+ def withScalaModuleMapper: ObjectMapper with ClassTagExtensions = {
Review Comment:
Yes, it seems a little redundant and has been removed
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #37999: [SPARK-39146][CORE] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1257719214
Write a mirco-benchmark to test Jackson ObjectWriter read and write:
https://github.com/LuciferYang/spark/blob/objectMapper/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/JacksonBenchmark.scala
```scala
def testReadJsonToMap(valuesPerIteration: Int): Unit = {
val input =
"""
|{"mergeDir":"/a/b/c/mergeDirName","attemptId":"appattempt_1648454518011_994053_000001"}
""".stripMargin
val benchmark = new Benchmark("Test read json to map",
valuesPerIteration, output = output)
benchmark.addCase("Test Multiple") { _: Int =>
for (_ <- 0L until valuesPerIteration) {
val mapper = new ObjectMapper()
mapper.registerModule(DefaultScalaModule)
mapper.readValue(input, classOf[mutable.HashMap[String, String]])
}
}
val mapper = new ObjectMapper()
mapper.registerModule(DefaultScalaModule)
benchmark.addCase("Test Single") { _: Int =>
for (_ <- 0L until valuesPerIteration) {
mapper.readValue(input, classOf[mutable.HashMap[String, String]])
}
}
benchmark.run()
}
def testWriteMapToJson(valuesPerIteration: Int): Unit = {
val map: mutable.HashMap[String, String] = new mutable.HashMap[String, String]()
map.put("mergeDir", "/a/b/c/mergeDirName")
map.put("attemptId", "yarn_appattempt_1648454518011_994053_000001")
val benchmark = new Benchmark("Test write map to json",
valuesPerIteration, output = output)
benchmark.addCase("Test Multiple") { _: Int =>
for (_ <- 0L until valuesPerIteration) {
val mapper = new ObjectMapper()
mapper.registerModule(DefaultScalaModule)
mapper.writeValueAsString(map)
}
}
val mapper = new ObjectMapper()
mapper.registerModule(DefaultScalaModule)
benchmark.addCase("Test Single") { _: Int =>
for (_ <- 0L until valuesPerIteration) {
mapper.writeValueAsString(map)
}
}
benchmark.run()
}
def testCreateObjectMapper(valuesPerIteration: Int): Unit = {
val benchmark = new Benchmark("Test create ObjectMapper",
valuesPerIteration, output = output)
benchmark.addCase("Test create ObjectMapper") { _: Int =>
for (_ <- 0L until valuesPerIteration) {
val mapper = new ObjectMapper()
mapper.registerModule(DefaultScalaModule)
}
}
benchmark.run()
}
override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
val valuesPerIteration = 10000
testCreateObjectMapper(valuesPerIteration = valuesPerIteration)
testWriteMapToJson(valuesPerIteration = valuesPerIteration)
testReadJsonToMap(valuesPerIteration = valuesPerIteration)
}
```
and run this use GA:
```
------------------------------------------------------------------------------------------------------------------------
Test create ObjectMapper 648 652 4 0.0 64819.0 1.0XOpenJDK 64-Bit Server VM 1.8.0_332-b09 on Linux 5.13.0-1022-azure
Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz
Test write map to json: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test Multiple 2116 2127 15 0.0 211556.5 1.0X
Test Single 4 4 0 2.4 416.1 508.4XOpenJDK 64-Bit Server VM 1.8.0_332-b09 on Linux 5.13.0-1022-azure
Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz
Test read json to map: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
Test Multiple 8848 8867 27 0.0 884776.2 1.0X
Test Single
```
From the test results, we should use singleton Jackson ObjectMapper, because it seems expensive to new a ObjectMapper instance.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #37999: [SPARK-39146][CORE][SQL][K8S] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1258914643
In the serial r/w scenario, the benefits are obvious,
- Reading scenario: using singleton is 1800+% faster than creating `ObjectMapper ` every time
- Write scenario: using a single instance is 500+% faster than creating `ObjectMapper ` every 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #37999: [SPARK-39146][CORE][SQL][K8S] Introduce `JacksonUtils` to use singleton Jackson ObjectMapper
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #37999:
URL: https://github.com/apache/spark/pull/37999#issuecomment-1259478461
https://github.com/FasterXML/jackson-core/issues/349#issuecomment-280794659
https://github.com/FasterXML/jackson-core/commit/67add8c6fdbb8f2968b21a7c54b33db151c0a950#diff-190cbec71e87394830d19fa2fea51b3bc324aa5fe694fc036ef85d1ad39d528f
It seems that Jackson 2.8.7 has fixed the problems mentioned in post. If we have doubts about similar issue, I can only make limited changes as you said
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org