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 2021/01/13 02:26:37 UTC
[GitHub] [spark] WamBamBoozle opened a new pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
WamBamBoozle opened a new pull request #31162:
URL: https://github.com/apache/spark/pull/31162
### What changes were proposed in this pull request?
A way of letting sparkR users preload libraries and other initialization
in the daemon before it starts forking. A full description can be found
in docs/sparkr.md.
### Why are the changes needed?
Performance
### Does this PR introduce _any_ user-facing change?
Yes. It introduces two new sparkR session parameters,
spark.r.daemonInit and spark.r.daemonTimeout, as described in
docs/configuration.md.
### How was this patch tested?
added pkg/tests/fulltests/test_daemon_initialization.R
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r558449658
##########
File path: R/pkg/inst/worker/daemon.R
##########
@@ -32,6 +32,9 @@ inputCon <- socketConnection(
SparkR:::doServerAuth(inputCon, Sys.getenv("SPARKR_WORKER_SECRET"))
+# Application-specific daemon initialization. Typical use is loading libraries.
+eval(parse(text = Sys.getenv("SPARKR_DAEMON_INIT")))
+
Review comment:
To give you a sense of the cost of this, consider
```
> microbenchmark(NULL, times = 999999)
Unit: nanoseconds
expr min lq mean median uq max neval
NULL 2 4 4.607765 4 5 11552 999999
```
so on my 2017 MacBook pro, R evaluates NULL in 4 nanoseconds.
```
> Sys.setenv(x="NULL")
> microbenchmark(eval(parse(text = Sys.getenv("x"))), times=99999)
Unit: microseconds
expr min lq mean median uq max neval
eval(parse(text = Sys.getenv("x"))) 33.854 35.82 40.15034 37.479 39.4475 7219.072 99999
```
It takes 40 microseconds to unpack the environment variable and evaluate it.
For comparison, consider
- the 6 milliseconds we use loading worker.R at every fork when we could load it once and then just invoke it as a function thus saving 6 milliseconds off of every fork. It is an obvious optimization that just doesn't seem worth it, and
- the time per fork applications save by moving their initialization here. For example, our application takes 5 seconds to load its libraries for every fork
----------------------------------------------------------------
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.
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] WamBamBoozle edited a comment on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle edited a comment on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-772732386
@srowen, you write
> I dont' know much about R - why does this help improve performance?
It is like moving the invariant expression out of a loop. It is the difference between loading supporting libraries once vs loading them millions of times.
It is described in detail in [docs/sparkr.md#daemon-initialization](https://github.com/WamBamBoozle/spark/blob/daemon_init/docs/sparkr.md#daemon-initialization)
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r558449658
##########
File path: R/pkg/inst/worker/daemon.R
##########
@@ -32,6 +32,9 @@ inputCon <- socketConnection(
SparkR:::doServerAuth(inputCon, Sys.getenv("SPARKR_WORKER_SECRET"))
+# Application-specific daemon initialization. Typical use is loading libraries.
+eval(parse(text = Sys.getenv("SPARKR_DAEMON_INIT")))
+
Review comment:
To give you a sense of the cost of this, consider
```
> microbenchmark(NULL, times = 999999)
Unit: nanoseconds
expr min lq mean median uq max neval
NULL 2 4 4.607765 4 5 11552 999999
```
so on my 2017 MacBook pro, R evaluates NULL in 4 nanoseconds.
```
> Sys.setenv(x="NULL")
> microbenchmark(eval(parse(text = Sys.getenv("x"))), times=99999)
Unit: microseconds
expr min lq mean median uq max neval
eval(parse(text = Sys.getenv("x"))) 33.854 35.82 40.15034 37.479 39.4475 7219.072 99999
```
It takes 40 microseconds to unpack the environment variable and evaluate it.
For comparison, consider
- the 6 milliseconds we use loading worker.R at every fork when we could load it once and then just invoke it as a function thus saving 6 milliseconds off of every fork. It is an obvious optimization that just doesn't seem worth it, and
- the time applications save by moving their initialization here. For example, our application takes 5 seconds to load its libraries, and we've tens of thousands of partitions.
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r558449658
##########
File path: R/pkg/inst/worker/daemon.R
##########
@@ -32,6 +32,9 @@ inputCon <- socketConnection(
SparkR:::doServerAuth(inputCon, Sys.getenv("SPARKR_WORKER_SECRET"))
+# Application-specific daemon initialization. Typical use is loading libraries.
+eval(parse(text = Sys.getenv("SPARKR_DAEMON_INIT")))
+
Review comment:
To give you a sense of the cost of this, consider
```
> microbenchmark(NULL, times = 999999)
Unit: nanoseconds
expr min lq mean median uq max neval
NULL 2 4 4.607765 4 5 11552 999999
```
so on my 2018 MacBook Pro 2.2 GHz 6-Core Intel Core i7, R evaluates NULL in 4 nanoseconds.
```
> Sys.setenv(x="NULL")
> microbenchmark(eval(parse(text = Sys.getenv("x"))), times=99999)
Unit: microseconds
expr min lq mean median uq max neval
eval(parse(text = Sys.getenv("x"))) 33.854 35.82 40.15034 37.479 39.4475 7219.072 99999
```
It takes 40 microseconds to unpack the environment variable and evaluate it.
For comparison, consider
- the 6 milliseconds we use loading worker.R at every fork (we could load it once and then invoke it as a function thus saving 6 milliseconds off of every fork).
- the time applications save by moving their initialization here. For example, our application takes 5 seconds to load its libraries, and we've 40 thousand partitions. If n = the number of executors, then for every call to gapply() the wall time saved is (5 seconds) * (40,000 partitions) / n = 56 hours / n.
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r560424099
##########
File path: R/run-tests.sh
##########
@@ -30,11 +30,22 @@ if [[ $(echo $SPARK_AVRO_JAR_PATH | wc -l) -eq 1 ]]; then
fi
if [ -z "$SPARK_JARS" ]; then
- SPARK_TESTING=1 NOT_CRAN=true $FWDIR/../bin/spark-submit --driver-java-options "-Dlog4j.configuration=file:$FWDIR/log4j.properties" --conf spark.hadoop.fs.defaultFS="file:///" --conf spark.driver.extraJavaOptions="-Dio.netty.tryReflectionSetAccessible=true" --conf spark.executor.extraJavaOptions="-Dio.netty.tryReflectionSetAccessible=true" $FWDIR/pkg/tests/run-all.R 2>&1 | tee -a $LOGFILE
+ JARS=""
else
- SPARK_TESTING=1 NOT_CRAN=true $FWDIR/../bin/spark-submit --jars $SPARK_JARS --driver-java-options "-Dlog4j.configuration=file:$FWDIR/log4j.properties" --conf spark.hadoop.fs.defaultFS="file:///" --conf spark.driver.extraJavaOptions="-Dio.netty.tryReflectionSetAccessible=true" --conf spark.executor.extraJavaOptions="-Dio.netty.tryReflectionSetAccessible=true" $FWDIR/pkg/tests/run-all.R 2>&1 | tee -a $LOGFILE
+ JARS="--jars $SPARK_JARS"
fi
+NOT_CRAN=true \
+SPARK_TESTING=1 \
+$FWDIR/../bin/spark-submit \
+--conf spark.driver.extraJavaOptions="-Dio.netty.tryReflectionSetAccessible=true" \
+--conf spark.executor.extraJavaOptions="-Dio.netty.tryReflectionSetAccessible=true" \
+--conf spark.hadoop.fs.defaultFS="file:///" \
+--conf spark.r.daemonInit='testInit <- "wow"' \
+--driver-java-options "-Dlog4j.configuration=file:$FWDIR/log4j.properties" \
+$JARS \
+$FWDIR/pkg/tests/run-all.R 2>&1 | tee -a $LOGFILE
Review comment:
The only thing new here is "spark.r.daemonInit" -- the rest is just a refactoring of a duplication 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.
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 #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-772761516
I see. This sounds a bit like proposals for executor-wide initialization, though this is a bit more acute as a new R process spawns for every task. I wonder if there are side effects, like, one-time init from one job being picked up in later ones and that causing surprise. I haven't thought that through though.
----------------------------------------------------------------
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.
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] github-actions[bot] closed pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #31162:
URL: https://github.com/apache/spark/pull/31162
--
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.
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] WamBamBoozle edited a comment on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle edited a comment on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-759780566
Greetings @HyukjinKwon
Thanks so much for your review of #31072! I had to close that PR as I forgot rebasing git causes bad things to happen in github.
Regarding parameterizing the path to the core components of sparkR, namely daemon.R and worker.R. I think this is an excellent solution if your objective is to implement coverage analysis. I just want my users to routinely move their initialization into the daemon.
I'm looking at [python/test_coverage/coverage_daemon.py](https://github.com/apache/spark/pull/20204/files#diff-a19676926d6ad7cb2306c47aedc8c9dfd5bc4413ce1aaae8fb3b04c7c5836110)
I agree that "spark.python.daemon.module" is more general and more powerful than what I am proposing. This PR is for fixing one thing, whereas what you did was a very general solution to a different problem. I don't want to warn my users that this is for advanced users only. The concept is simple: put your initialization code here.
I propose we provide
- spark.r.daemon.finalize
- spark.r.daemon.initialize
- spark.r.worker.finalize
- spark.r.worker.initialize
where the value of each of those are strings of R code
that way we've achieved the same ends you have without forcing the user to understand the minutia of how and what to load. If you agree, I will change this PR to be the implementation of spark.r.daemon.initialize (instead of what it is now, spark.r.daemonInit). I would then open another ticket to implement the other flags.
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r558449658
##########
File path: R/pkg/inst/worker/daemon.R
##########
@@ -32,6 +32,9 @@ inputCon <- socketConnection(
SparkR:::doServerAuth(inputCon, Sys.getenv("SPARKR_WORKER_SECRET"))
+# Application-specific daemon initialization. Typical use is loading libraries.
+eval(parse(text = Sys.getenv("SPARKR_DAEMON_INIT")))
+
Review comment:
To give you a sense of the cost of this, consider
```
> microbenchmark(NULL, times = 999999)
Unit: nanoseconds
expr min lq mean median uq max neval
NULL 2 4 4.607765 4 5 11552 999999
```
so on my 2017 MacBook pro, R evaluates NULL in 4 nanoseconds.
```
> Sys.setenv(x="NULL")
> microbenchmark(eval(parse(text = Sys.getenv("x"))), times=99999)
Unit: microseconds
expr min lq mean median uq max neval
eval(parse(text = Sys.getenv("x"))) 33.854 35.82 40.15034 37.479 39.4475 7219.072 99999
```
It takes 40 microseconds to unpack the environment variable and evaluate it.
For comparison, consider
- the 6 milliseconds we use loading worker.R at every fork (we could load it once and then invoke it as a function thus saving 6 milliseconds off of every fork).
- the time applications save by moving their initialization here. For example, our application takes 5 seconds to load its libraries, and we've tens of thousands of partitions.
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r558449658
##########
File path: R/pkg/inst/worker/daemon.R
##########
@@ -32,6 +32,9 @@ inputCon <- socketConnection(
SparkR:::doServerAuth(inputCon, Sys.getenv("SPARKR_WORKER_SECRET"))
+# Application-specific daemon initialization. Typical use is loading libraries.
+eval(parse(text = Sys.getenv("SPARKR_DAEMON_INIT")))
+
Review comment:
To give you a sense of the cost of this, consider
```
> microbenchmark(NULL, times = 999999)
Unit: nanoseconds
expr min lq mean median uq max neval
NULL 2 4 4.328575 4 4 13885 999999
> microbenchmark(eval(parse(text = "NULL")), times=9999)
Unit: microseconds
expr min lq mean median uq max neval
eval(parse(text = "NULL")) 31.489 34.0545 40.1457 35.938 38.496 9370.646 9999
>
```
So, on my 2017 MacBook pro, it takes 34 microseconds.
For comparison, consider the 6 milliseconds we use up loading worker.R at every fork.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-759163709
Can one of the admins verify this patch?
----------------------------------------------------------------
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.
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] WamBamBoozle edited a comment on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle edited a comment on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-772732386
@srowen, you write
> I dont' know much about R - why does this help improve performance?
It is like moving the invariant expression out of the loop. It is the difference between loading supporting libraries once vs loading them millions of times.
It is described in detail in [docs/sparkr.md#daemon-initialization](https://github.com/WamBamBoozle/spark/blob/daemon_init/docs/sparkr.md#daemon-initialization)
----------------------------------------------------------------
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.
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] github-actions[bot] commented on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-855314777
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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.
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] HyukjinKwon commented on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-759845628
I still think it's better to have one solution to cover many cases. Having `spark.r.daemonInit` will fix one arguably corner case. You can wait for other committers to comment if you'd like to stick with the current approach.
----------------------------------------------------------------
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.
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] WamBamBoozle commented on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-760328927
I think the solution you suggest would intimidate my users so yes, I would love a second opinion.
----------------------------------------------------------------
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.
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 #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-772563234
I dont' know much about R - why does this help improve performance?
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r558449658
##########
File path: R/pkg/inst/worker/daemon.R
##########
@@ -32,6 +32,9 @@ inputCon <- socketConnection(
SparkR:::doServerAuth(inputCon, Sys.getenv("SPARKR_WORKER_SECRET"))
+# Application-specific daemon initialization. Typical use is loading libraries.
+eval(parse(text = Sys.getenv("SPARKR_DAEMON_INIT")))
+
Review comment:
To give you a sense of the cost of this, consider
```
> microbenchmark(NULL, times = 999999)
Unit: nanoseconds
expr min lq mean median uq max neval
NULL 2 4 4.607765 4 5 11552 999999
```
so on my 2018 MacBook Pro 2.2 GHz 6-Core Intel Core i7, R evaluates NULL in 4 nanoseconds.
```
> Sys.setenv(x="NULL")
> microbenchmark(eval(parse(text = Sys.getenv("x"))), times=99999)
Unit: microseconds
expr min lq mean median uq max neval
eval(parse(text = Sys.getenv("x"))) 33.854 35.82 40.15034 37.479 39.4475 7219.072 99999
```
It takes 40 microseconds to unpack the environment variable and evaluate it.
For comparison, consider
- the 6 milliseconds we use loading worker.R at every fork (we could load it once and then invoke it as a function thus saving 6 milliseconds off of every fork).
- the time applications save by moving their initialization here. For example, our application takes 5 seconds to load its libraries, and we've tens of thousands of partitions.
----------------------------------------------------------------
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.
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] WamBamBoozle commented on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-772732386
> I dont' know much about R - why does this help improve performance?
It is like moving the invariant expression out of the loop. It is the difference between loading supporting libraries once vs loading them millions of times.
It is described in detail in [docs/sparkr.md](https://github.com/WamBamBoozle/spark/blob/daemon_init/docs/sparkr.md#daemon-initialization)
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r558449658
##########
File path: R/pkg/inst/worker/daemon.R
##########
@@ -32,6 +32,9 @@ inputCon <- socketConnection(
SparkR:::doServerAuth(inputCon, Sys.getenv("SPARKR_WORKER_SECRET"))
+# Application-specific daemon initialization. Typical use is loading libraries.
+eval(parse(text = Sys.getenv("SPARKR_DAEMON_INIT")))
+
Review comment:
To give you a sense of the cost of this, consider
```
> microbenchmark(NULL, times = 999999)
Unit: nanoseconds
expr min lq mean median uq max neval
NULL 2 4 4.607765 4 5 11552 999999
```
so on my 2018 MacBook Pro 2.2 GHz 6-Core Intel Core i7, R evaluates NULL in 4 nanoseconds.
```
> Sys.setenv(x="NULL")
> microbenchmark(eval(parse(text = Sys.getenv("x"))), times=99999)
Unit: microseconds
expr min lq mean median uq max neval
eval(parse(text = Sys.getenv("x"))) 33.854 35.82 40.15034 37.479 39.4475 7219.072 99999
```
It takes 40 microseconds to unpack the environment variable and evaluate it.
For comparison, consider
- the 6 milliseconds we use loading worker.R at every fork (we could load it once and then invoke it as a function thus saving 6 milliseconds off of every fork).
- the time applications save by moving their initialization here. For example, our application takes 5 seconds to load its libraries
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r558449658
##########
File path: R/pkg/inst/worker/daemon.R
##########
@@ -32,6 +32,9 @@ inputCon <- socketConnection(
SparkR:::doServerAuth(inputCon, Sys.getenv("SPARKR_WORKER_SECRET"))
+# Application-specific daemon initialization. Typical use is loading libraries.
+eval(parse(text = Sys.getenv("SPARKR_DAEMON_INIT")))
+
Review comment:
To give you a sense of the cost of this, consider
```
> Sys.setenv(x="NULL")
> microbenchmark(eval(parse(text = Sys.getenv("x"))), times=99999)
Unit: microseconds
expr min lq mean median uq max neval
eval(parse(text = Sys.getenv("x"))) 33.854 35.82 40.15034 37.479 39.4475 7219.072 99999
```
So, on my 2017 MacBook pro, it takes 40 microseconds.
For comparison, consider the 6 milliseconds we use loading worker.R at every fork.
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r558449658
##########
File path: R/pkg/inst/worker/daemon.R
##########
@@ -32,6 +32,9 @@ inputCon <- socketConnection(
SparkR:::doServerAuth(inputCon, Sys.getenv("SPARKR_WORKER_SECRET"))
+# Application-specific daemon initialization. Typical use is loading libraries.
+eval(parse(text = Sys.getenv("SPARKR_DAEMON_INIT")))
+
Review comment:
To give you a sense of the cost of this, consider
```
> microbenchmark(NULL, times = 999999)
Unit: nanoseconds
expr min lq mean median uq max neval
NULL 2 4 4.607765 4 5 11552 999999
```
so on my 2018 MacBook Pro 2.2 GHz 6-Core Intel Core i7, R evaluates NULL in 4 nanoseconds.
```
> Sys.setenv(x="NULL")
> microbenchmark(eval(parse(text = Sys.getenv("x"))), times=99999)
Unit: microseconds
expr min lq mean median uq max neval
eval(parse(text = Sys.getenv("x"))) 33.854 35.82 40.15034 37.479 39.4475 7219.072 99999
```
It takes 40 microseconds to unpack the environment variable and evaluate it.
For comparison, consider
- the 6 milliseconds we use loading worker.R at every fork (we could load it once and then invoke it as a function thus saving 6 milliseconds off of every fork).
- the time applications save by moving their initialization here. For example, our application takes 5 seconds to load its libraries, and we've 40 thousand partitions. 50 s * 40,000 = 56 hours cpu-time saved for every call to gapply
----------------------------------------------------------------
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.
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] WamBamBoozle commented on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-759780566
Greetings @HyukjinKwon
Thanks so much for your review of #31072! I had to close that PR as I forgot rebasing git causes bad things to happen in github.
Regarding parameterizing the path to the core components of sparkR, namely daemon.R and worker.R. I think this is an excellent solution if your objective is to implement coverage analysis. I just want my users to routinely move their initialization into the daemon.
I'm looking at python/test_coverage/coverage_daemon.py
I agree that spark.python.daemon.module is more general and more powerful than what I am proposing. This PR is for fixing one thing, whereas what you did was a very general solution to a different problem. I don't want to warn my users that this is for advanced users only. The concept is simple: put your initialization code here.
I propose we provide
spark.r.daemon.finalize
spark.r.daemon.initialize
spark.r.worker.finalize
spark.r.worker.initialize
where the value of each of those are strings of R code
that way we've achieved the same ends you have without forcing the user to understand the minutia of how and what to load. If you agree, I will change this PR to be the implementation of spark.r.daemon.initialize (instead of what it is now, spark.r.daemonInit). I would then open another ticket to implement the other flags.
----------------------------------------------------------------
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.
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] WamBamBoozle commented on a change in pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle commented on a change in pull request #31162:
URL: https://github.com/apache/spark/pull/31162#discussion_r556688283
##########
File path: docs/configuration.md
##########
@@ -2824,6 +2824,28 @@ External users can query the static sql config values via `SparkSession.conf` or
</td>
<td>1.5.3</td>
</tr>
+<tr>
+ <td><code>spark.r.daemonInit</code></td>
+ <td>NULL</td>
+ <td>
+ R code you would like executed <b>before</b> the daemon starts
+ processing groups. See
+ <a href="sparkr.md#daemon-initialization">Daemon Initialization</a>.
+ Increase spark.r.daemonTimeout if your initialization takes longer
+ than the default.
+ </td>
+ <td>3.2.0</td>
+</tr>
+<tr>
+ <td><code>spark.r.daemonTimeout</code></td>
+ <td>10000</td>
+ <td>
+ What is the upper limit in milliseconds that we allow for daemon
+ initialization? In other words, how long should the spark executor
+ wait until it decides that it can't launch the SparkR daemon?
+ </td>
+ <td>3.2.0</td>
Review comment:
Is this the correct release to put here?
----------------------------------------------------------------
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.
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] WamBamBoozle edited a comment on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle edited a comment on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-772732386
@srowen, you write
> I dont' know much about R - why does this help improve performance?
It is like moving the invariant expression out of the loop. It is the difference between loading supporting libraries once vs loading them millions of times.
It is described in detail in [docs/sparkr.md#daemon-initialization](https://github.com/WamBamBoozle/spark/blob/daemon_init/docs/sparkr.md#daemon-initialization)
----------------------------------------------------------------
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.
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] WamBamBoozle edited a comment on pull request #31162: [SPARK-34033][R] SparkR Daemon Initialization
Posted by GitBox <gi...@apache.org>.
WamBamBoozle edited a comment on pull request #31162:
URL: https://github.com/apache/spark/pull/31162#issuecomment-772732386
@srowen, you write
> I dont' know much about R - why does this help improve performance?
It is like moving the invariant expression out of the loop. It is the difference between loading supporting libraries once vs loading them millions of times.
It is described in detail in [docs/sparkr.md](https://github.com/WamBamBoozle/spark/blob/daemon_init/docs/sparkr.md#daemon-initialization)
----------------------------------------------------------------
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.
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