[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...
Github user shivaram commented on a diff in the pull request: https://github.com/apache/spark/pull/15433#discussion_r83043023 --- Diff: core/src/main/scala/org/apache/spark/api/r/RBackendHandler.scala --- @@ -263,18 +264,19 @@ private[r] object JVMObjectTracker { // TODO: This map should be thread-safe if we want to support multiple // connections at the same time - private[this] val objMap = new HashMap[String, Object] + private[this] val objMap: ConcurrentMap[String, Object] = +new MapMaker().weakValues().makeMap[String, Object]() // TODO: We support only one connection now, so an integer is fine. // Investigate using use atomic integer in the future. private[this] var objCounter: Int = 0 def getObject(id: String): Object = { -objMap(id) +objMap.get(id) } def get(id: String): Option[Object] = { -objMap.get(id) +Option(objMap.get(id)) } def put(obj: Object): String = { --- End diff -- There is a another JIRA related to thread safety of this class https://issues.apache.org/jira/browse/SPARK-17823 - It might be good address that in this PR ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/15433#discussion_r82960952 --- Diff: core/src/main/scala/org/apache/spark/api/r/RBackendHandler.scala --- @@ -263,18 +264,19 @@ private[r] object JVMObjectTracker { // TODO: This map should be thread-safe if we want to support multiple // connections at the same time - private[this] val objMap = new HashMap[String, Object] + private[this] val objMap: ConcurrentMap[String, Object] = --- End diff -- Not that it matters a lot, but do you need the reference type? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/15433#discussion_r82961129 --- Diff: core/src/main/scala/org/apache/spark/api/r/RBackendHandler.scala --- @@ -263,18 +264,19 @@ private[r] object JVMObjectTracker { // TODO: This map should be thread-safe if we want to support multiple // connections at the same time - private[this] val objMap = new HashMap[String, Object] + private[this] val objMap: ConcurrentMap[String, Object] = +new MapMaker().weakValues().makeMap[String, Object]() // TODO: We support only one connection now, so an integer is fine. // Investigate using use atomic integer in the future. private[this] var objCounter: Int = 0 def getObject(id: String): Object = { -objMap(id) +objMap.get(id) } def get(id: String): Option[Object] = { -objMap.get(id) +Option(objMap.get(id)) } def put(obj: Object): String = { --- End diff -- It kind of seems like this class is meant to be thread-safe, but this isn't. You could use `AtomicLong` and `getAndIncrement`? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/15433#discussion_r82960799 --- Diff: core/src/main/scala/org/apache/spark/api/r/RBackendHandler.scala --- @@ -284,7 +286,7 @@ private[r] object JVMObjectTracker { objId } - def remove(id: String): Option[Object] = { + def remove(id: String) { --- End diff -- Although it's normal to return the object that was removed from map methods, if this isn't desired, I'd declare this as `: Unit` to be clear --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...
GitHub user techaddict reopened a pull request: https://github.com/apache/spark/pull/15433 [SPARK-17822] Use weak reference in JVMObjectTracker.objMap because it may leak JVM objects ## What changes were proposed in this pull request? Use weak reference in JVMObjectTracker.objMap because it may leak JVM objects ## How was this patch tested? existing tests You can merge this pull request into a Git repository by running: $ git pull https://github.com/techaddict/spark SPARK-17822 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/15433.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #15433 commit 7023c40a99eaa81ee7bcd202a4b74df811d0cfc7 Author: Sandeep Singh Date: 2016-10-10T11:54:34Z [SPARK-17822] Use weak reference in JVMObjectTracker.objMap because it may leak JVM objects commit 69845947df62187eb40f3cc6468b52e38bdab897 Author: Sandeep Singh Date: 2016-10-10T13:23:56Z Merge branch 'master' into SPARK-17822 commit 995611d75351d24907ce2b22e7d33752cc803da3 Author: Sandeep Singh Date: 2016-10-11T13:13:09Z Merge branch 'master' into SPARK-17822 commit 8e763bef78fe147e84e1771f237a75ff42780705 Author: Sandeep Singh Date: 2016-10-12T06:33:26Z fix for failures commit 7d50d84f90fcda9e5dec79c9be834870c83443c4 Author: Sandeep Singh Date: 2016-10-12T06:34:23Z Merge branch 'master' into SPARK-17822 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...
Github user techaddict closed the pull request at: https://github.com/apache/spark/pull/15433 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...
GitHub user techaddict opened a pull request: https://github.com/apache/spark/pull/15433 [SPARK-17822] Use weak reference in JVMObjectTracker.objMap because it may leak JVM objects ## What changes were proposed in this pull request? Use weak reference in JVMObjectTracker.objMap because it may leak JVM objects ## How was this patch tested? existing tests You can merge this pull request into a Git repository by running: $ git pull https://github.com/techaddict/spark SPARK-17822 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/15433.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #15433 commit 7023c40a99eaa81ee7bcd202a4b74df811d0cfc7 Author: Sandeep Singh Date: 2016-10-10T11:54:34Z [SPARK-17822] Use weak reference in JVMObjectTracker.objMap because it may leak JVM objects commit 69845947df62187eb40f3cc6468b52e38bdab897 Author: Sandeep Singh Date: 2016-10-10T13:23:56Z Merge branch 'master' into SPARK-17822 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org