[GitHub] [spark] vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface.
vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add
monotonic time to Clock interface.
URL: https://github.com/apache/spark/pull/26058#discussion_r333755039
##
File path: core/src/main/scala/org/apache/spark/util/Clock.scala
##
@@ -21,7 +21,14 @@ package org.apache.spark.util
* An interface to represent clocks, so that they can be mocked out in unit
tests.
*/
private[spark] trait Clock {
+ /** @return Current system time, in ms. */
def getTimeMillis(): Long
+ /** @return Current value of monotonic time source, in ns. */
+ def nanoTime(): Long
+ /**
Review comment:
Old code didn't have them, but sure.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface.
vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface. URL: https://github.com/apache/spark/pull/26058#discussion_r332714350 ## File path: core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ## @@ -288,7 +288,7 @@ private[spark] class ExecutorAllocationManager( } // Update executor target number only after initializing flag is unset -updateAndSyncNumExecutorsTarget(clock.getTimeMillis()) +updateAndSyncNumExecutorsTarget(clock.nanoTime()) Review comment: That is interesting, but according to this: http://btorpey.github.io/blog/2014/02/18/clock-sources-in-linux/ `CLOCK_MONOTONIC` uses RDTSC (or RDTSCP when available), which solves that problem. The preferred one (RDTSCP) seems to be available in all modern CPUs. So, `CLOCK_MONOTONIC` actually fulfills the requirements of the `nanoTime()` API (that the source of time is consistent). (The AMD K8 mentioned in your comment, which came out more than 10 years ago, is where RDTSCP was added.) 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface.
vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface. URL: https://github.com/apache/spark/pull/26058#discussion_r332718848 ## File path: core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ## @@ -288,7 +288,7 @@ private[spark] class ExecutorAllocationManager( } // Update executor target number only after initializing flag is unset -updateAndSyncNumExecutorsTarget(clock.getTimeMillis()) +updateAndSyncNumExecutorsTarget(clock.nanoTime()) Review comment: Probably worth to link directly to the following also (which is linked from the article above) https://stackoverflow.com/questions/10921210/cpu-tsc-fetch-operation-especially-in-multicore-multi-processor-environment 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface.
vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface. URL: https://github.com/apache/spark/pull/26058#discussion_r332714350 ## File path: core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ## @@ -288,7 +288,7 @@ private[spark] class ExecutorAllocationManager( } // Update executor target number only after initializing flag is unset -updateAndSyncNumExecutorsTarget(clock.getTimeMillis()) +updateAndSyncNumExecutorsTarget(clock.nanoTime()) Review comment: That is interesting, but according to this: http://btorpey.github.io/blog/2014/02/18/clock-sources-in-linux/ `CLOCK_MONOTONIC` uses RDTSC (or RDTSCP when available), which solves that problem. The preferred one (RDTSCP) seems to be available in all modern CPUs. So, `CLOCK_MONOTONIC` actually fulfills the requirements of the `nanoTime()` API (that the source of time is consistent). (The AMD K8 mentioned in your comment, which came out more than 10 years ago, is where RDTSCP was added.) 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface.
vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface. URL: https://github.com/apache/spark/pull/26058#discussion_r332705981 ## File path: core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ## @@ -288,7 +288,7 @@ private[spark] class ExecutorAllocationManager( } // Update executor target number only after initializing flag is unset -updateAndSyncNumExecutorsTarget(clock.getTimeMillis()) +updateAndSyncNumExecutorsTarget(clock.nanoTime()) Review comment: Also, just wanted to point out that it's ok if `nanoTime` is not completely monotonic, as long as it's better than `getTimeMillis()`. After all, this code was working with a non-monotonic clock before. The only problem is if different threads can have wildly different views of what `nanoTime` returns, so that comparing the values makes no sense. I don't believe that to be the case. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface.
vanzin commented on a change in pull request #26058: [SPARK-10614][core] Add monotonic time to Clock interface. URL: https://github.com/apache/spark/pull/26058#discussion_r332686468 ## File path: core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ## @@ -288,7 +288,7 @@ private[spark] class ExecutorAllocationManager( } // Update executor target number only after initializing flag is unset -updateAndSyncNumExecutorsTarget(clock.getTimeMillis()) +updateAndSyncNumExecutorsTarget(clock.nanoTime()) Review comment: Are you sure about that? This is what the javadocs say: The same origin is used by all invocations of this method in an instance of a Java virtual machine; other virtual machine instances are likely to use a different origin. (Comment about "other virtual machines" aside.) That seems to be corroborated by other sources, too (e.g. https://stackoverflow.com/questions/510462/is-system-nanotime-completely-useless). Seems in a long gone past there might have been issues, but not today. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
