You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by JoshRosen <gi...@git.apache.org> on 2016/03/22 03:28:42 UTC
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
GitHub user JoshRosen opened a pull request:
https://github.com/apache/spark/pull/11874
[SPARK-3000] Allow MemoryStore blocks to be dropped to disk in parallel
This patch refactors the BlockManager and MemoryManager locking/synchronization logic in order to allow MemoryStore blocks to be dropped to the disk in parallel, significantly reducing `MemoryManager` lock contention when acquiring storage memory.
## Key change
Prior to this patch, `MemoryStore.evictBlocksToFreeSpace()` synchronized on `memoryManager` for the entire block-dropping process. This patch reduces the scope of the synchronization to only cover the phase in which we are selecting the set of blocks to be evicted. Once a task has committed to dropping a set of blocks, it now releases the `memoryManager` lock and performs the block eviction without holding any locks. This allows multiple tasks to drop blocks to disk in parallel and avoids blocking non-spilling tasks via contention on the `memoryManager` lock.
One tricky concern is preventing tasks from stealing the freed storage memory which results from eviction. To address this, this patch modifies the eviction path so that evicted blocks' memory is not immediately freed with the memory manager. Once all of a tasks's block evictions have completed, tasks now compare the total amount of memory freed from eviction (which could exceed their request size) to the amount of memory needed and either acquire more memory or free excess storage memory.
Another concern is making sure that a single block isn't double-evicted by multiple tasks. Double-eviction is prevented by the fact that tasks obtain exclusive write locks on all of the blocks that they will evict and the fact that the process of selecting which blocks to evict is still synchronized on the `memoryManager`.
Note that a single task will still spill blocks sequentially, so parallelism only results from multiple tasks triggering separate block evictions. If we want to enable a single task's evictions to proceed in parallel then we can do that in a separate followup patch (it should be a fairly straightforward change).
## Motivating performance results
This patch is motivated by a simple `spark-shell` benchmark that I wrote to stress the BlockManager's block eviction path:
```scala
import org.apache.spark.storage.StorageLevel
val startTime = System.currentTimeMillis
(1 to 100).map { _ =>
sc.parallelize(1 to 1000, 100)
.map(x => new Array[Byte](1024 * 1024 / 10))
.persist(StorageLevel.MEMORY_AND_DISK)
}.map { x => x.count(); x }
val endTime = System.currentTimeMillis
println((endTime - startTime) / 1000.0)
```
----
**Prior to this patch**, I observed extreme contention for the `UnifiedMemoryManager`'s monitor:
----
**After this patch**, the contention on the `UnifiedMemoryManager` lock is significantly reduced and the lock contention bottlenecks shifted to other sources:
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/JoshRosen/spark SPARK-3000
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/11874.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 #11874
----
commit e4aa6abe86ae6fb3e20208863a6807468605b48d
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-21T19:45:18Z
Remove unreachable code in BlockManager.dropFromMemory
This method is only ever called on blocks which are present in the
MemoryStore, so we don’t need a bunch of the old error-handling
code to cope with this being called on non-existent blocks.
commit c564a40589ed98b148f41ca64f3aaac1fdcec06e
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-21T21:28:08Z
Minor re-arrangement of locking code to reduce contention.
commit 847fdae7a6fbda13ac5c2537671b2c27363c8175
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-21T23:32:54Z
Narrow scope of synchronization in acquireStorageMemory.
commit 5854662717810d27e386fe51cb4d11bf0377f20c
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-21T23:37:21Z
Improve locking in code unroll memory transfer code.
commit b03ff592feb090e2e6f1801088baa1cbcc8eb0f1
Author: Josh Rosen <jo...@databricks.com>
Date: 2016-03-22T00:42:53Z
Merge remote-tracking branch 'origin/master' into SPARK-3000
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200098370
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200146560
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57253712
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -56,12 +58,30 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
/**
* Acquire N bytes of memory to cache the given block, evicting existing ones if necessary.
- *
+ *
* @return whether all N bytes were successfully granted.
*/
- def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = lock.synchronized {
- val numBytesToFree = math.max(0, numBytes - memoryFree)
- acquireMemory(blockId, numBytes, numBytesToFree)
+ def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = {
+ // First, attempt to acquire as much memory as we can without performing any eviction
--- End diff --
I would expand on this: "Otherwise, another thread may jump in and use up some free memory as soon as we release the lock, in which case we may evict too few blocks to store this block later."
Right now it's not clear why we need to do this in 2 steps.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200465359
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53954/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199942718
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200252459
**[Test build #53908 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53908/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200150767
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57258931
--- Diff: core/src/main/scala/org/apache/spark/memory/UnifiedMemoryManager.scala ---
@@ -131,26 +131,28 @@ private[spark] class UnifiedMemoryManager private[memory] (
}
}
- override def acquireStorageMemory(blockId: BlockId, numBytes: Long): Boolean = synchronized {
- assert(onHeapExecutionMemoryPool.poolSize + storageMemoryPool.poolSize == maxMemory)
- assert(numBytes >= 0)
- if (numBytes > maxStorageMemory) {
- // Fail fast if the block simply won't fit
- logInfo(s"Will not store $blockId as the required space ($numBytes bytes) exceeds our " +
- s"memory limit ($maxStorageMemory bytes)")
- return false
- }
- if (numBytes > storageMemoryPool.memoryFree) {
- // There is not enough free memory in the storage pool, so try to borrow free memory from
- // the execution pool.
- val memoryBorrowedFromExecution = Math.min(onHeapExecutionMemoryPool.memoryFree, numBytes)
- onHeapExecutionMemoryPool.decrementPoolSize(memoryBorrowedFromExecution)
- storageMemoryPool.incrementPoolSize(memoryBorrowedFromExecution)
+ override def acquireStorageMemory(blockId: BlockId, numBytes: Long): Boolean = {
+ synchronized {
+ assert(onHeapExecutionMemoryPool.poolSize + storageMemoryPool.poolSize == maxMemory)
+ assert(numBytes >= 0)
+ if (numBytes > maxStorageMemory) {
+ // Fail fast if the block simply won't fit
+ logInfo(s"Will not store $blockId as the required space ($numBytes bytes) exceeds our " +
+ s"memory limit ($maxStorageMemory bytes)")
+ return false
+ }
+ if (numBytes > storageMemoryPool.memoryFree) {
+ // There is not enough free memory in the storage pool, so try to borrow free memory from
+ // the execution pool.
+ val memoryBorrowedFromExecution = Math.min(onHeapExecutionMemoryPool.memoryFree, numBytes)
+ onHeapExecutionMemoryPool.decrementPoolSize(memoryBorrowedFromExecution)
+ storageMemoryPool.incrementPoolSize(memoryBorrowedFromExecution)
--- End diff --
everything in this block is cheap anyway. The only expensive thing we want to keep out of the synchronized block here is evicting blocks from memory store, which is what `storageMemoryPool.acquireMemory` does.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200213519
**[Test build #53908 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53908/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57094823
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -56,11 +56,13 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
/**
* Acquire N bytes of memory to cache the given block, evicting existing ones if necessary.
- *
+ *
* @return whether all N bytes were successfully granted.
*/
- def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = lock.synchronized {
- val numBytesToFree = math.max(0, numBytes - memoryFree)
+ def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = {
+ val numBytesToFree = lock.synchronized {
+ math.max(0, numBytes - memoryFree)
+ }
acquireMemory(blockId, numBytes, numBytesToFree)
--- End diff --
I just pushed a patch which aims to address (1). I don't think that (2) is a concern because we're only missing out on an opportunity which did not exist prior to this patch's changes. In the old code, other tasks' calls to free storage memory would be blocked due to contention on the MemoryManager lock, so this transaction wouldn't observe the effect of free()s which took place after it started.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200099702
Jenkins, retest this please.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200180743
**[Test build #53895 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53895/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200148776
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53858/
Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57253450
--- Diff: core/src/main/scala/org/apache/spark/memory/StaticMemoryManager.scala ---
@@ -62,17 +64,34 @@ private[spark] class StaticMemoryManager(
}
}
- override def acquireUnrollMemory(blockId: BlockId, numBytes: Long): Boolean = synchronized {
- val currentUnrollMemory = storageMemoryPool.memoryStore.currentUnrollMemory
- val freeMemory = storageMemoryPool.memoryFree
+ override def acquireUnrollMemory(blockId: BlockId, numBytes: Long): Boolean = {
// When unrolling, we will use all of the existing free memory, and, if necessary,
// some extra space freed from evicting cached blocks. We must place a cap on the
// amount of memory to be evicted by unrolling, however, otherwise unrolling one
// big block can blow away the entire cache.
- val maxNumBytesToFree = math.max(0, maxUnrollMemory - currentUnrollMemory - freeMemory)
- // Keep it within the range 0 <= X <= maxNumBytesToFree
- val numBytesToFree = math.max(0, math.min(maxNumBytesToFree, numBytes - freeMemory))
- storageMemoryPool.acquireMemory(blockId, numBytes, numBytesToFree)
+ val (freeMemoryAcquired, numBytesToFree) = synchronized {
+ val currentUnrollMemory = storageMemoryPool.memoryStore.currentUnrollMemory
+ val freeMemory = storageMemoryPool.memoryFree
+ val freeMemoryAcquired = math.min(numBytes, freeMemory)
+ val res = storageMemoryPool.acquireMemory(blockId, freeMemoryAcquired, numBytesToFree = 0)
+ assert(res, "failed to acquire free storage memory")
+ val maxNumBytesToFree = math.max(0, maxUnrollMemory - currentUnrollMemory - freeMemory)
+ // Keep it within the range 0 <= X <= maxNumBytesToFree
+ val numBytesToFree = math.min(numBytes - freeMemoryAcquired, maxNumBytesToFree)
+ (freeMemoryAcquired, numBytesToFree)
+ }
+ try {
+ if (storageMemoryPool.acquireMemory(blockId, numBytes - freeMemoryAcquired, numBytesToFree)) {
--- End diff --
actually, don't you want to check whether `numBytes - freeMemoryAcquired` is > 0? If it's 0 then we should probably just return true here without calling anything.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200148653
**[Test build #53858 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53858/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200594484
I left a few suggestions on how to rewrite things in a way that's more readable. I'm not sure if we should change `StaticMemoryManager` since new users will probably not use that, and old users already suffer from the lock contention problem. Making changes there might introduce regressions and increase the surface area of things we need to maintain in Spark 2.0.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57259282
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -338,24 +345,51 @@ private[spark] class MemoryStore(
* RDDs that don't fit into memory that we want to avoid).
*
* @param blockId the ID of the block we are freeing space for, if any
- * @param space the size of this block
- * @return the amount of memory (in bytes) freed by eviction
+ * @param requestedSpace the amount of memory which should be freed via block eviction
+ * @return the total size (in bytes) of the blocks evicted from the MemoryStore. This memory is
+ * not released with the memory manager, so it's the caller's responsibility to perform
+ * the appropriate bookkeeping calls.
--- End diff --
Is it because we want to transfer memory atomically to accommodate the new storage memory requests? Then we should say so in the comment.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200214387
**[Test build #2666 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2666/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199593999
**[Test build #53738 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53738/consoleFull)** for PR 11874 at commit [`b03ff59`](https://github.com/apache/spark/commit/b03ff592feb090e2e6f1801088baa1cbcc8eb0f1).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57254134
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -75,21 +95,31 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
def acquireMemory(
blockId: BlockId,
numBytesToAcquire: Long,
- numBytesToFree: Long): Boolean = lock.synchronized {
+ numBytesToFree: Long): Boolean = {
assert(numBytesToAcquire >= 0)
assert(numBytesToFree >= 0)
- assert(memoryUsed <= poolSize)
- if (numBytesToFree > 0) {
+ assert(lock.synchronized(memoryUsed <= poolSize))
--- End diff --
nit can you do
```
lock.synchronized { assert(memoryUsed <= poolSize) }
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200211313
**[Test build #53895 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53895/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199603760
cc @sameeragarwal too
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200098372
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53855/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57252644
--- Diff: core/src/main/scala/org/apache/spark/memory/StaticMemoryManager.scala ---
@@ -62,17 +64,34 @@ private[spark] class StaticMemoryManager(
}
}
- override def acquireUnrollMemory(blockId: BlockId, numBytes: Long): Boolean = synchronized {
- val currentUnrollMemory = storageMemoryPool.memoryStore.currentUnrollMemory
- val freeMemory = storageMemoryPool.memoryFree
+ override def acquireUnrollMemory(blockId: BlockId, numBytes: Long): Boolean = {
// When unrolling, we will use all of the existing free memory, and, if necessary,
// some extra space freed from evicting cached blocks. We must place a cap on the
// amount of memory to be evicted by unrolling, however, otherwise unrolling one
// big block can blow away the entire cache.
- val maxNumBytesToFree = math.max(0, maxUnrollMemory - currentUnrollMemory - freeMemory)
- // Keep it within the range 0 <= X <= maxNumBytesToFree
- val numBytesToFree = math.max(0, math.min(maxNumBytesToFree, numBytes - freeMemory))
- storageMemoryPool.acquireMemory(blockId, numBytes, numBytesToFree)
+ val (freeMemoryAcquired, numBytesToFree) = synchronized {
+ val currentUnrollMemory = storageMemoryPool.memoryStore.currentUnrollMemory
+ val freeMemory = storageMemoryPool.memoryFree
+ val freeMemoryAcquired = math.min(numBytes, freeMemory)
+ val res = storageMemoryPool.acquireMemory(blockId, freeMemoryAcquired, numBytesToFree = 0)
+ assert(res, "failed to acquire free storage memory")
+ val maxNumBytesToFree = math.max(0, maxUnrollMemory - currentUnrollMemory - freeMemory)
+ // Keep it within the range 0 <= X <= maxNumBytesToFree
+ val numBytesToFree = math.min(numBytes - freeMemoryAcquired, maxNumBytesToFree)
--- End diff --
can we be sure that `numBytesToFree` won't be negative? Did you remove the `math.max(0, ...)` on purpose?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by sameeragarwal <gi...@git.apache.org>.
Github user sameeragarwal commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57224685
--- Diff: core/src/main/scala/org/apache/spark/memory/StaticMemoryManager.scala ---
@@ -62,17 +64,34 @@ private[spark] class StaticMemoryManager(
}
}
- override def acquireUnrollMemory(blockId: BlockId, numBytes: Long): Boolean = synchronized {
- val currentUnrollMemory = storageMemoryPool.memoryStore.currentUnrollMemory
- val freeMemory = storageMemoryPool.memoryFree
+ override def acquireUnrollMemory(blockId: BlockId, numBytes: Long): Boolean = {
// When unrolling, we will use all of the existing free memory, and, if necessary,
// some extra space freed from evicting cached blocks. We must place a cap on the
// amount of memory to be evicted by unrolling, however, otherwise unrolling one
// big block can blow away the entire cache.
- val maxNumBytesToFree = math.max(0, maxUnrollMemory - currentUnrollMemory - freeMemory)
- // Keep it within the range 0 <= X <= maxNumBytesToFree
- val numBytesToFree = math.max(0, math.min(maxNumBytesToFree, numBytes - freeMemory))
- storageMemoryPool.acquireMemory(blockId, numBytes, numBytesToFree)
+ val (freeMemoryAcquired, numBytesToFree) = synchronized {
+ val currentUnrollMemory = storageMemoryPool.memoryStore.currentUnrollMemory
+ val freeMemory = storageMemoryPool.memoryFree
+ val freeMemoryAcquired = math.min(numBytes, freeMemory)
+ val res = storageMemoryPool.acquireMemory(blockId, freeMemoryAcquired, numBytesToFree = 0)
+ assert(res, "failed to acquire free storage memory")
+ val maxNumBytesToFree = math.max(0, maxUnrollMemory - currentUnrollMemory - freeMemory)
+ // Keep it within the range 0 <= X <= maxNumBytesToFree
+ val numBytesToFree = math.min(numBytes - freeMemoryAcquired, maxNumBytesToFree)
--- End diff --
Out of curiosity, what happens if `numBytesToFree < numBytes - freeMemoryAcquired`? Do we then use the disk to unroll the block?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57257678
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -110,17 +140,21 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
* Try to shrink the size of this storage memory pool by `spaceToFree` bytes. Return the number
* of bytes removed from the pool's capacity.
*/
- def shrinkPoolToFreeSpace(spaceToFree: Long): Long = lock.synchronized {
- // First, shrink the pool by reclaiming free memory:
- val spaceFreedByReleasingUnusedMemory = math.min(spaceToFree, memoryFree)
- decrementPoolSize(spaceFreedByReleasingUnusedMemory)
- val remainingSpaceToFree = spaceToFree - spaceFreedByReleasingUnusedMemory
+ def shrinkPoolToFreeSpace(spaceToFree: Long): Long = {
+ var spaceFreedByReleasingUnusedMemory = 0L
--- End diff --
if you do the `val (x, y) = lock.synchronized { ... }` thing here then you don't need a var
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200252708
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200068247
**[Test build #53802 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53802/consoleFull)** for PR 11874 at commit [`fd832f7`](https://github.com/apache/spark/commit/fd832f73bca729c91ca7f1c4a262288a454468da).
* This patch **fails from timeout after a configured wait of \`250m\`**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200068300
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199932901
**[Test build #53790 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53790/consoleFull)** for PR 11874 at commit [`fd832f7`](https://github.com/apache/spark/commit/fd832f73bca729c91ca7f1c4a262288a454468da).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199632390
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53738/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57079697
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -316,17 +324,42 @@ private[spark] class MemoryStore(
*
* @param blockId the ID of the block we are freeing space for, if any
* @param space the size of this block
--- End diff --
This variable name / comment is outdated; `space` is the amount of memory that we'd like to free via eviction, which is not necessarily equal to the whole size of the block.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by sameeragarwal <gi...@git.apache.org>.
Github user sameeragarwal commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57228905
--- Diff: core/src/main/scala/org/apache/spark/memory/UnifiedMemoryManager.scala ---
@@ -131,26 +131,28 @@ private[spark] class UnifiedMemoryManager private[memory] (
}
}
- override def acquireStorageMemory(blockId: BlockId, numBytes: Long): Boolean = synchronized {
- assert(onHeapExecutionMemoryPool.poolSize + storageMemoryPool.poolSize == maxMemory)
- assert(numBytes >= 0)
- if (numBytes > maxStorageMemory) {
- // Fail fast if the block simply won't fit
- logInfo(s"Will not store $blockId as the required space ($numBytes bytes) exceeds our " +
- s"memory limit ($maxStorageMemory bytes)")
- return false
- }
- if (numBytes > storageMemoryPool.memoryFree) {
- // There is not enough free memory in the storage pool, so try to borrow free memory from
- // the execution pool.
- val memoryBorrowedFromExecution = Math.min(onHeapExecutionMemoryPool.memoryFree, numBytes)
- onHeapExecutionMemoryPool.decrementPoolSize(memoryBorrowedFromExecution)
- storageMemoryPool.incrementPoolSize(memoryBorrowedFromExecution)
+ override def acquireStorageMemory(blockId: BlockId, numBytes: Long): Boolean = {
+ synchronized {
+ assert(onHeapExecutionMemoryPool.poolSize + storageMemoryPool.poolSize == maxMemory)
+ assert(numBytes >= 0)
+ if (numBytes > maxStorageMemory) {
+ // Fail fast if the block simply won't fit
+ logInfo(s"Will not store $blockId as the required space ($numBytes bytes) exceeds our " +
+ s"memory limit ($maxStorageMemory bytes)")
+ return false
+ }
+ if (numBytes > storageMemoryPool.memoryFree) {
+ // There is not enough free memory in the storage pool, so try to borrow free memory from
+ // the execution pool.
+ val memoryBorrowedFromExecution = Math.min(onHeapExecutionMemoryPool.memoryFree, numBytes)
+ onHeapExecutionMemoryPool.decrementPoolSize(memoryBorrowedFromExecution)
+ storageMemoryPool.incrementPoolSize(memoryBorrowedFromExecution)
--- End diff --
out of curiosity, why do we synchronize on the whole block instead of just these 2 lines above?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200568212
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200568106
**[Test build #53958 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53958/consoleFull)** for PR 11874 at commit [`aa558a2`](https://github.com/apache/spark/commit/aa558a27bb1533992e0933c1afea932e52598a80).
* This patch **fails from timeout after a configured wait of \`250m\`**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200117567
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53837/
Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57254579
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -56,12 +58,30 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
/**
* Acquire N bytes of memory to cache the given block, evicting existing ones if necessary.
- *
+ *
* @return whether all N bytes were successfully granted.
*/
- def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = lock.synchronized {
- val numBytesToFree = math.max(0, numBytes - memoryFree)
- acquireMemory(blockId, numBytes, numBytesToFree)
+ def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = {
+ // First, attempt to acquire as much memory as we can without performing any eviction
+ val bytesAcquiredWithoutEviction = lock.synchronized {
+ val bytesAcquired = math.min(numBytes, memoryFree)
+ _memoryUsed += bytesAcquired
+ bytesAcquired
+ }
+ // Acquire the remainder of the memory by performing eviction
+ try {
+ val bytesToAcquireViaEviction = numBytes - bytesAcquiredWithoutEviction
+ if (acquireMemory(blockId, bytesToAcquireViaEviction, bytesToAcquireViaEviction)) {
+ true
+ } else {
+ releaseMemory(bytesAcquiredWithoutEviction)
+ false
+ }
+ } catch {
+ case NonFatal(e) =>
+ releaseMemory(bytesAcquiredWithoutEviction)
--- End diff --
same double release comment here
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200534926
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57258072
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -315,6 +315,13 @@ private[spark] class MemoryStore(
}
}
+ private[storage] def removeBlockAndKeepStorageMemory(blockId: BlockId): Long = {
--- End diff --
this is too much... just add a boolean flag to the existing `remove`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200117563
Merged build finished. Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen closed the pull request at:
https://github.com/apache/spark/pull/11874
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200150670
**[Test build #53839 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53839/consoleFull)** for PR 11874 at commit [`63a6e11`](https://github.com/apache/spark/commit/63a6e110f1cb5436b4b865e8c86004013a9a6827).
* This patch **fails from timeout after a configured wait of \`250m\`**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57254527
--- Diff: core/src/main/scala/org/apache/spark/memory/StaticMemoryManager.scala ---
@@ -17,6 +17,8 @@
package org.apache.spark.memory
+import scala.util.control.NonFatal
+
--- End diff --
by the way I'm not sure whether it's worth it to change anything here. This is for legacy mode and will be removed in the future anyway, and for those people who *are* using it they're doing so because the new workload doesn't work for them, so it's best to just keep things the same here as much as possible. I don't feel super strongly about reverting the changes here but I don't think it's totally necessary.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200070365
It took me long time to understand these changes, LGTM. Fine-gain locking is usually hard to prove, it's better to have another people also review this.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57261232
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -75,21 +95,31 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
def acquireMemory(
blockId: BlockId,
numBytesToAcquire: Long,
- numBytesToFree: Long): Boolean = lock.synchronized {
+ numBytesToFree: Long): Boolean = {
assert(numBytesToAcquire >= 0)
assert(numBytesToFree >= 0)
- assert(memoryUsed <= poolSize)
- if (numBytesToFree > 0) {
+ assert(lock.synchronized(memoryUsed <= poolSize))
--- End diff --
This lock acquisition should be cheap, so I think it's fine to have this assert.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200146456
**[Test build #53861 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53861/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200150773
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53839/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57253342
--- Diff: core/src/main/scala/org/apache/spark/memory/StaticMemoryManager.scala ---
@@ -62,17 +64,34 @@ private[spark] class StaticMemoryManager(
}
}
- override def acquireUnrollMemory(blockId: BlockId, numBytes: Long): Boolean = synchronized {
- val currentUnrollMemory = storageMemoryPool.memoryStore.currentUnrollMemory
- val freeMemory = storageMemoryPool.memoryFree
+ override def acquireUnrollMemory(blockId: BlockId, numBytes: Long): Boolean = {
// When unrolling, we will use all of the existing free memory, and, if necessary,
// some extra space freed from evicting cached blocks. We must place a cap on the
// amount of memory to be evicted by unrolling, however, otherwise unrolling one
// big block can blow away the entire cache.
- val maxNumBytesToFree = math.max(0, maxUnrollMemory - currentUnrollMemory - freeMemory)
- // Keep it within the range 0 <= X <= maxNumBytesToFree
- val numBytesToFree = math.max(0, math.min(maxNumBytesToFree, numBytes - freeMemory))
- storageMemoryPool.acquireMemory(blockId, numBytes, numBytesToFree)
+ val (freeMemoryAcquired, numBytesToFree) = synchronized {
+ val currentUnrollMemory = storageMemoryPool.memoryStore.currentUnrollMemory
+ val freeMemory = storageMemoryPool.memoryFree
+ val freeMemoryAcquired = math.min(numBytes, freeMemory)
+ val res = storageMemoryPool.acquireMemory(blockId, freeMemoryAcquired, numBytesToFree = 0)
+ assert(res, "failed to acquire free storage memory")
+ val maxNumBytesToFree = math.max(0, maxUnrollMemory - currentUnrollMemory - freeMemory)
+ // Keep it within the range 0 <= X <= maxNumBytesToFree
+ val numBytesToFree = math.min(numBytes - freeMemoryAcquired, maxNumBytesToFree)
+ (freeMemoryAcquired, numBytesToFree)
+ }
+ try {
+ if (storageMemoryPool.acquireMemory(blockId, numBytes - freeMemoryAcquired, numBytesToFree)) {
+ true
+ } else {
+ storageMemoryPool.releaseMemory(freeMemoryAcquired)
+ false
+ }
+ } catch {
+ case NonFatal(e) =>
+ storageMemoryPool.releaseMemory(freeMemoryAcquired)
--- End diff --
it's kind of weird that it's possible to call `releaseMemory` twice here. If the first one fails at the end (after actually releasing memory) it will come here and possibly release the memory again (and then failing again). A better way to write this would be something like:
```
// Acquire the rest of the memory outside a synchronized block.
// This call may evict existing blocks if necessary.
val acquired =
try {
storageMemoryPool.acquire(...)
} catch {
case NonFatal(e) =>
storageMemoryPool.releaseMemory(...)
throw e
}
if (!acquired) {
storageMemoryPool.releaseMemory(...)
}
acquired
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200116438
**[Test build #53837 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53837/consoleFull)** for PR 11874 at commit [`3c17d43`](https://github.com/apache/spark/commit/3c17d4393b8cf40296243773f1c9cf711cb40c35).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200179763
Jenkins, retest this please.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200489619
**[Test build #53960 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53960/consoleFull)** for PR 11874 at commit [`560e2d2`](https://github.com/apache/spark/commit/560e2d2beb6660c687590ed9e64d46a84374184e).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200908114
I'm going to close this for now. Per some offline discussion, I think there's an easier way to implement all of this by significantly refactoring / simplifying MemoryManager.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199632076
**[Test build #53738 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53738/consoleFull)** for PR 11874 at commit [`b03ff59`](https://github.com/apache/spark/commit/b03ff592feb090e2e6f1801088baa1cbcc8eb0f1).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57253952
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -56,12 +58,30 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
/**
* Acquire N bytes of memory to cache the given block, evicting existing ones if necessary.
- *
+ *
* @return whether all N bytes were successfully granted.
*/
- def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = lock.synchronized {
- val numBytesToFree = math.max(0, numBytes - memoryFree)
- acquireMemory(blockId, numBytes, numBytesToFree)
+ def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = {
+ // First, attempt to acquire as much memory as we can without performing any eviction
+ val bytesAcquiredWithoutEviction = lock.synchronized {
+ val bytesAcquired = math.min(numBytes, memoryFree)
+ _memoryUsed += bytesAcquired
+ bytesAcquired
+ }
+ // Acquire the remainder of the memory by performing eviction
+ try {
+ val bytesToAcquireViaEviction = numBytes - bytesAcquiredWithoutEviction
--- End diff --
can you add some asserts here to make sure this is non-negative? Also I would do all of this only if this is > 0.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199942664
**[Test build #53790 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53790/consoleFull)** for PR 11874 at commit [`fd832f7`](https://github.com/apache/spark/commit/fd832f73bca729c91ca7f1c4a262288a454468da).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by sameeragarwal <gi...@git.apache.org>.
Github user sameeragarwal commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57225044
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -75,21 +95,31 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
def acquireMemory(
blockId: BlockId,
numBytesToAcquire: Long,
- numBytesToFree: Long): Boolean = lock.synchronized {
+ numBytesToFree: Long): Boolean = {
assert(numBytesToAcquire >= 0)
assert(numBytesToFree >= 0)
- assert(memoryUsed <= poolSize)
- if (numBytesToFree > 0) {
+ assert(lock.synchronized(memoryUsed <= poolSize))
+ val memoryFreedByEviction = if (numBytesToFree > 0) {
memoryStore.evictBlocksToFreeSpace(Some(blockId), numBytesToFree)
+ } else {
+ 0
}
- // NOTE: If the memory store evicts blocks, then those evictions will synchronously call
- // back into this StorageMemoryPool in order to free memory. Therefore, these variables
- // should have been updated.
- val enoughMemory = numBytesToAcquire <= memoryFree
- if (enoughMemory) {
- _memoryUsed += numBytesToAcquire
+ if (memoryFreedByEviction > numBytesToAcquire) {
+ releaseMemory(memoryFreedByEviction - numBytesToAcquire)
+ }
--- End diff --
maybe put the remaining code below in an `else` block?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200073627
**[Test build #53837 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53837/consoleFull)** for PR 11874 at commit [`3c17d43`](https://github.com/apache/spark/commit/3c17d4393b8cf40296243773f1c9cf711cb40c35).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200534930
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53960/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200261253
**[Test build #2667 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2667/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200214420
**[Test build #2667 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2667/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200463353
**[Test build #53954 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53954/consoleFull)** for PR 11874 at commit [`3491254`](https://github.com/apache/spark/commit/3491254805c333bad88d1ac1349d605b3daf0d15).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200211636
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53895/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57084302
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -56,11 +56,13 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
/**
* Acquire N bytes of memory to cache the given block, evicting existing ones if necessary.
- *
+ *
* @return whether all N bytes were successfully granted.
*/
- def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = lock.synchronized {
- val numBytesToFree = math.max(0, numBytes - memoryFree)
+ def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = {
+ val numBytesToFree = lock.synchronized {
+ math.max(0, numBytes - memoryFree)
+ }
acquireMemory(blockId, numBytes, numBytesToFree)
--- End diff --
As discussed, the fact that we release the lock here could lead to two race conditions:
(1) If another thread comes in and uses up some free memory before this line, then `numBytesToFree` will be too low, causing the memory acquisition to fail.
(2) If another thread comes in and frees up a lot of memory before this line, then `numBytesToFree` will be too high, causing more blocks to be evicted than necessary.
We can remedy (1) by acquiring as much free space is needed and available before releasing the lock and then try to acquire the rest later. Fixing (2) may be a little more difficult.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57085656
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -206,11 +206,12 @@ private[spark] class MemoryStore(
}
val size = entry.size
def transferUnrollToStorage(amount: Long): Unit = {
--- End diff --
I'm actually going to just revert this change since it's fine to synchronize this transfer since the transfer is incapable of spilling.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200098368
**[Test build #53855 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53855/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
* This patch **fails build dependency tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200148774
Merged build finished. Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57254309
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -56,12 +58,30 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
/**
* Acquire N bytes of memory to cache the given block, evicting existing ones if necessary.
- *
+ *
* @return whether all N bytes were successfully granted.
*/
- def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = lock.synchronized {
- val numBytesToFree = math.max(0, numBytes - memoryFree)
- acquireMemory(blockId, numBytes, numBytesToFree)
+ def acquireMemory(blockId: BlockId, numBytes: Long): Boolean = {
+ // First, attempt to acquire as much memory as we can without performing any eviction
+ val bytesAcquiredWithoutEviction = lock.synchronized {
+ val bytesAcquired = math.min(numBytes, memoryFree)
+ _memoryUsed += bytesAcquired
+ bytesAcquired
+ }
+ // Acquire the remainder of the memory by performing eviction
--- End diff --
can you add something like
```
// Here we want to evict as much space as we are requesting because we have already
// acquired all available free space for this block.
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57257266
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -75,21 +95,31 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
def acquireMemory(
blockId: BlockId,
numBytesToAcquire: Long,
- numBytesToFree: Long): Boolean = lock.synchronized {
+ numBytesToFree: Long): Boolean = {
assert(numBytesToAcquire >= 0)
assert(numBytesToFree >= 0)
- assert(memoryUsed <= poolSize)
- if (numBytesToFree > 0) {
+ assert(lock.synchronized(memoryUsed <= poolSize))
--- End diff --
actually IDK if it's worth asserting this here if we have to acquire the lock for it. I'm OK with just removing it... (or moving it to release or something)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199957121
**[Test build #53802 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53802/consoleFull)** for PR 11874 at commit [`fd832f7`](https://github.com/apache/spark/commit/fd832f73bca729c91ca7f1c4a262288a454468da).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200534389
**[Test build #53960 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53960/consoleFull)** for PR 11874 at commit [`560e2d2`](https://github.com/apache/spark/commit/560e2d2beb6660c687590ed9e64d46a84374184e).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200068307
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53802/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200582486
FYI I'm working on a complete rewrite of this patch from first principles.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200465300
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199942723
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53790/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200252711
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53908/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by sameeragarwal <gi...@git.apache.org>.
Github user sameeragarwal commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200528240
Not really a review as I was just trying to understand the MemoryManager, but overall looks great. Do we have unit tests that try to acquire storage/unroll memory in parallel?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199632385
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200099746
**[Test build #53858 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53858/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200211632
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57259185
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -338,24 +345,51 @@ private[spark] class MemoryStore(
* RDDs that don't fit into memory that we want to avoid).
*
* @param blockId the ID of the block we are freeing space for, if any
- * @param space the size of this block
- * @return the amount of memory (in bytes) freed by eviction
+ * @param requestedSpace the amount of memory which should be freed via block eviction
+ * @return the total size (in bytes) of the blocks evicted from the MemoryStore. This memory is
+ * not released with the memory manager, so it's the caller's responsibility to perform
+ * the appropriate bookkeeping calls.
--- End diff --
I don't understand why this should be the case. I get that we don't want to acquire the lock for each block we drop, but can't we just release all the memory at the end of this method? We still only acquire 1 lock and now the caller doesn't have to worry about forgetting to release memory.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200262485
**[Test build #2666 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2666/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200146562
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53861/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57257098
--- Diff: core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala ---
@@ -75,21 +95,31 @@ private[memory] class StorageMemoryPool(lock: Object) extends MemoryPool(lock) w
def acquireMemory(
blockId: BlockId,
numBytesToAcquire: Long,
- numBytesToFree: Long): Boolean = lock.synchronized {
+ numBytesToFree: Long): Boolean = {
assert(numBytesToAcquire >= 0)
assert(numBytesToFree >= 0)
- assert(memoryUsed <= poolSize)
- if (numBytesToFree > 0) {
+ assert(lock.synchronized(memoryUsed <= poolSize))
+ val memoryFreedByEviction = if (numBytesToFree > 0) {
memoryStore.evictBlocksToFreeSpace(Some(blockId), numBytesToFree)
+ } else {
+ 0
}
- // NOTE: If the memory store evicts blocks, then those evictions will synchronously call
- // back into this StorageMemoryPool in order to free memory. Therefore, these variables
- // should have been updated.
- val enoughMemory = numBytesToAcquire <= memoryFree
- if (enoughMemory) {
- _memoryUsed += numBytesToAcquire
+ if (memoryFreedByEviction > numBytesToAcquire) {
+ releaseMemory(memoryFreedByEviction - numBytesToAcquire)
+ }
--- End diff --
+1, also I'll just wrap both things in synchronized so we don't have to acquire the lock twice. Fewer interleavings to think about.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200475373
**[Test build #53958 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53958/consoleFull)** for PR 11874 at commit [`aa558a2`](https://github.com/apache/spark/commit/aa558a27bb1533992e0933c1afea932e52598a80).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200465254
**[Test build #53954 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53954/consoleFull)** for PR 11874 at commit [`3491254`](https://github.com/apache/spark/commit/3491254805c333bad88d1ac1349d605b3daf0d15).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200076142
**[Test build #53839 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53839/consoleFull)** for PR 11874 at commit [`63a6e11`](https://github.com/apache/spark/commit/63a6e110f1cb5436b4b865e8c86004013a9a6827).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-199955176
Jenkins, retest this please.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57258133
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -410,9 +414,23 @@ private[spark] class MemoryStore(
selectedBlocks.foreach { id =>
blockInfoManager.unlock(id)
}
- 0L
+ selectedBlocks.clear()
+ freedMemory = 0
}
}
+
+ // At this point, `selectedBlocks` is the (possibly-empty) set of blocks that we have chosen
--- End diff --
possibly empty
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200097876
**[Test build #53855 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53855/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200212329
Jenkins, retest this please.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200568216
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53958/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/11874#discussion_r57084313
--- Diff: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala ---
@@ -206,11 +206,12 @@ private[spark] class MemoryStore(
}
val size = entry.size
def transferUnrollToStorage(amount: Long): Unit = {
--- End diff --
Could you change this function name to reflect the change here?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3000] Allow MemoryStore blocks to be dr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11874#issuecomment-200100446
**[Test build #53861 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53861/consoleFull)** for PR 11874 at commit [`ccdd20c`](https://github.com/apache/spark/commit/ccdd20cd7e29a6a8d0d2592494fd0d19292b3702).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org