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 2015/12/07 05:10:19 UTC

[GitHub] spark pull request: [SPARK-12165] Fix bug in eviction of storage m...

GitHub user JoshRosen opened a pull request:

    https://github.com/apache/spark/pull/10170

    [SPARK-12165] Fix bug in eviction of storage memory by execution

    This patch fixes a bug in the eviction of storage memory by execution.
    
    ## The bug:
    
    In general, execution should be able to storage memory when the total storage memory usage is greater than `maxMemory * spark.memory.storageFraction`. Due to a bug, however, Spark might wind up evicting no storage memory in certain cases where the storage memory usage was between `maxMemory * spark.memory.storageFraction` and `maxMemory`. For example, here is a regression test which illustrates the bug:
    
    ```scala
        val maxMemory = 1000L
        val taskAttemptId = 0L
        val (mm, ms) = makeThings(maxMemory)
        // Since we used the default storage fraction (0.5), we should be able to allocate 500 bytes
        // of storage memory which are immune to eviction by execution memory pressure.
    
        // Acquire enough storage memory to exceed the storage region size
        assert(mm.acquireStorageMemory(dummyBlock, 750L, evictedBlocks))
        assertEvictBlocksToFreeSpaceNotCalled(ms)
        assert(mm.executionMemoryUsed === 0L)
        assert(mm.storageMemoryUsed === 750L)
     
        // At this point, storage is using 250 more bytes of memory than it is guaranteed, so execution
        // should be able to reclaim up to 250 bytes of storage memory.
        // Therefore, execution should now be able to require up to 500 bytes of memory:
        assert(mm.acquireExecutionMemory(500L, taskAttemptId, MemoryMode.ON_HEAP) === 500L) // <--- fails by only returning 250L
        assert(mm.storageMemoryUsed === 500L)
        assert(mm.executionMemoryUsed === 500L)
        assertEvictBlocksToFreeSpaceCalled(ms, 250L)
    ```
    
    The problem relates to the control flow / interaction between `StorageMemoryPool.shrinkPoolToReclaimSpace()` and `MemoryStore.ensureFreeSpace()`. While trying to allocate the 500 bytes of execution memory, the `UnifiedMemoryManager` discovers that it will need to reclaim 250 bytes of memory from storage, so it calls `StorageMemoryPool.shrinkPoolToReclaimSpace(250L)`. This method, in turn, calls `MemoryStore.ensureFreeSpace(250L)`. However, `ensureFreeSpace()` first checks whether the requested space is less than `maxStorageMemory - storageMemoryUsed`, which will be true if there is any free execution memory because it turns out that `MemoryStore.maxStorageMemory = (maxMemory - onHeapExecutionMemoryPool.memoryUsed)` when the `UnifiedMemoryManager` is used.
    
    The control flow here is somewhat confusing (it grew to be messy / confusing over time / as a result of the merging / refactoring of several components). In the pre-Spark 1.6 code, `ensureFreeSpace` was called directly by the `MemoryStore` itself, whereas in 1.6 it's involved in a confusing callback flow where `MemoryStore` calls `MemoryManager.acquireStorageMemory`, which then calls back into `MemoryStore.ensureFreeSpace`, which, in turn, calls `MemoryManager.freeStorageMemory`.
    
    ## The solution:
    
    The solution implemented in this patch is to remove the confusing circular control flow between `MemoryManager` and `MemoryStore`, making the storage memory acquisition process much more linear / straightforward. The key changes:
    
    - Remove a layer of inheritance which made the memory manager code harder to understand (53841174760a24a0df3eb1562af1f33dbe340eb9).
    - Move some bounds checks earlier in the call chain (13ba7ada77f87ef1ec362aec35c89a924e6987cb).
    - Refactor `ensureFreeSpace()` so that the part which evicts blocks can be called independently from the part which checks whether there is enough free space to avoid eviction (7c68ca09cb1b12f157400866983f753ac863380e).
    - Realize that this lets us remove a layer of overloads from `ensureFreeSpace` (eec4f6c87423d5e482b710e098486b3bbc4daf06).
    - Realize that `ensureFreeSpace()` can simply be replaced with an `evictBlocksToFreeSpace()` method which is called [after we've already figured out](https://github.com/apache/spark/blob/2dc842aea82c8895125d46a00aa43dfb0d121de9/core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala#L88) how much memory needs to be reclaimed via eviction; (2dc842aea82c8895125d46a00aa43dfb0d121de9).
    
    Along the way, I fixed some problems with the mocks in `MemoryManagerSuite`: the old mocks would [unconditionally](https://github.com/apache/spark/blob/80a824d36eec9d9a9f092ee1741453851218ec73/core/src/test/scala/org/apache/spark/memory/MemoryManagerSuite.scala#L84) report that a block had been evicted even if there was enough space in the storage pool such that eviction would be avoided.
    
    I also fixed a problem where `StorageMemoryPool._memoryUsed` might become negative due to freed memory being double-counted when excution evicts storage. The problem was that `StorageMemoryPoolshrinkPoolToFreeSpace` would [decrement `_memoryUsed`](https://github.com/apache/spark/commit/7c68ca09cb1b12f157400866983f753ac863380e#diff-935c68a9803be144ed7bafdd2f756a0fL133) even though `StorageMemoryPool.freeMemory` had already decremented it as each evicted block was freed.
    
    ## TODOs
    
    - [ ] Add stronger assertions or a dedicated regression test for the `_memoryUsed < 0` bug, which was uncovered while testing this patch and a related fix for SPARK-12155.
    - [ ] See whether we can now remove the confusing `MemoryStore.maxMemory` method.
    
    /cc @andrewor14 @zsxwing @yhuai @rxin 

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/JoshRosen/spark SPARK-12165

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/10170.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 #10170
    
----
commit 012cb4ba9ba1e32337f2f7bb612abb53fe4070be
Author: Josh Rosen <jo...@databricks.com>
Date:   2015-12-06T20:57:45Z

    Reset ensureFreeSpaceCalled before each test.
    
    This reduces coupling between failed tests.

commit b519fe628a9a2b8238dfedbfd9b74bdd2ddc0de4
Author: Josh Rosen <jo...@databricks.com>
Date:   2015-12-07T00:00:23Z

    Add regression test for storage eviction bug.

commit 7c68ca09cb1b12f157400866983f753ac863380e
Author: Josh Rosen <jo...@databricks.com>
Date:   2015-12-06T22:40:07Z

    Add MemoryStore.freeSpaceForExecution() method, which forces blocks to be dropped.
    
    Previously, ensureFreeSpace() might end up not dropping blocks if the total
    storage memory pool usage was less than the maximum possible storage pool usage.

commit 53841174760a24a0df3eb1562af1f33dbe340eb9
Author: Josh Rosen <jo...@databricks.com>
Date:   2015-12-07T01:47:29Z

    Remove a layer of confusing inheritance.

commit 13ba7ada77f87ef1ec362aec35c89a924e6987cb
Author: Josh Rosen <jo...@databricks.com>
Date:   2015-12-07T01:53:45Z

    Put fail-fast for non-fitting blocks earlier in call chain.

commit eec4f6c87423d5e482b710e098486b3bbc4daf06
Author: Josh Rosen <jo...@databricks.com>
Date:   2015-12-07T01:56:48Z

    Collapse ensureFreeSpace overloads

commit 2dc842aea82c8895125d46a00aa43dfb0d121de9
Author: Josh Rosen <jo...@databricks.com>
Date:   2015-12-07T02:56:11Z

    Replace ensureFreeSpace() with evictBlocksToFreeSpace().

commit 0eac7da041326cfbec2c1db2f279cb655744f90e
Author: Josh Rosen <jo...@databricks.com>
Date:   2015-12-07T03:39:37Z

    Update JIRA in test case.

----


---
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-12165][SPARK-12189] Fix bugs in evictio...

Posted by JoshRosen <gi...@git.apache.org>.

Github user JoshRosen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/10170#discussion_r47035741
  
    --- Diff: core/src/test/scala/org/apache/spark/memory/UnifiedMemoryManagerSuite.scala ---
    @@ -143,7 +148,25 @@ class UnifiedMemoryManagerSuite extends MemoryManagerSuite with PrivateMethodTes
         assert(mm.acquireExecutionMemory(400L, taskAttemptId, MemoryMode.ON_HEAP) === 300L)
         assert(mm.executionMemoryUsed === 600L)
         assert(mm.storageMemoryUsed === 400L)
    -    assertEnsureFreeSpaceNotCalled(ms)
    +    assertEvictBlocksToFreeSpaceNotCalled(ms)
    +    assert(evictedBlocks.isEmpty)
    +  }
    +
    +  test("execution can evict storage blocks when storage memory is below max mem (SPARK-12165)") {
    +    val maxMemory = 1000L
    +    val taskAttemptId = 0L
    +    val (mm, ms) = makeThings(maxMemory)
    +    // Acquire enough storage memory to exceed the storage region size
    +    assert(mm.acquireStorageMemory(dummyBlock, 750L, evictedBlocks))
    +    assertEvictBlocksToFreeSpaceNotCalled(ms)
    +    assert(mm.executionMemoryUsed === 0L)
    +    assert(mm.storageMemoryUsed === 750L)
    +    // Should now be able to require up to 500 bytes of memory
    +    assert(mm.acquireExecutionMemory(500L, taskAttemptId, MemoryMode.ON_HEAP) === 500L)
    --- End diff --
    
    > The problem isn't that "storage memory is below than max memory". Even in the existing test "eviction evicts storage", storage memory (750) is below the max memory (1000).
    
    It's true that this was a little inprecise, but I think that "execution memory requests smaller than free memory should evict storage" is also slightly inprecise because this essentially sounds like a synonym for "execution evicts storage when there is not enough free memory." I guess a slightly more accurate description of SPARK-12165 would be something like "execution evicts storage if free memory is insufficient and free memory > 0", since not _all_ situations where there is insufficient free memory will trigger the first part of SPARK-12165. This precision doesn't really matter, though, so I'm happy to adopt your test name if you find it to be clearer.


---
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-12165][SPARK-12189] Fix bugs in evictio...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/10170#issuecomment-163092608
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47382/
    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-12165][SPARK-12189] Fix bugs in evictio...

Posted by andrewor14 <gi...@git.apache.org>.

Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/10170#discussion_r47016581
  
    --- Diff: core/src/test/scala/org/apache/spark/memory/StaticMemoryManagerSuite.scala ---
    @@ -85,33 +82,33 @@ class StaticMemoryManagerSuite extends MemoryManagerSuite {
         val (mm, ms) = makeThings(Long.MaxValue, maxStorageMem)
         assert(mm.storageMemoryUsed === 0L)
         assert(mm.acquireStorageMemory(dummyBlock, 10L, evictedBlocks))
    -    // `ensureFreeSpace` should be called with the number of bytes requested
    -    assertEnsureFreeSpaceCalled(ms, 10L)
    +    assertEvictBlocksToFreeSpaceNotCalled(ms)
         assert(mm.storageMemoryUsed === 10L)
    +
         assert(mm.acquireStorageMemory(dummyBlock, 100L, evictedBlocks))
    -    assertEnsureFreeSpaceCalled(ms, 100L)
    +    assertEvictBlocksToFreeSpaceNotCalled(ms)
         assert(mm.storageMemoryUsed === 110L)
         // Acquire more than the max, not granted
         assert(!mm.acquireStorageMemory(dummyBlock, maxStorageMem + 1L, evictedBlocks))
    -    assertEnsureFreeSpaceCalled(ms, maxStorageMem + 1L)
    +    assertEvictBlocksToFreeSpaceNotCalled(ms)
         assert(mm.storageMemoryUsed === 110L)
         // Acquire up to the max, requests after this are still granted due to LRU eviction
         assert(mm.acquireStorageMemory(dummyBlock, maxStorageMem, evictedBlocks))
    -    assertEnsureFreeSpaceCalled(ms, 1000L)
    +    assertEvictBlocksToFreeSpaceCalled(ms, 110L)
         assert(mm.storageMemoryUsed === 1000L)
         assert(mm.acquireStorageMemory(dummyBlock, 1L, evictedBlocks))
    -    assertEnsureFreeSpaceCalled(ms, 1L)
    +    assertEvictBlocksToFreeSpaceCalled(ms, 1L)
         assert(mm.storageMemoryUsed === 1000L)
    --- End diff --
    
    It took me a while to understand why this one is still `1000L`. The situation is as follows:
    
    1. Put a 1000 bytes block
    2. Put a 1 byte block, this should have evicted the first block
    3. Storage memory used is still 1000 bytes somehow (I expected 1 byte)
    
    Because of the way the test is set up, however, as of step (2) we actually treat the 1000-byte block inserted in step (1) as 1000 1-byte blocks. When we call `evictBlocksToFreeSpace` here we actually evicted a 1-byte block to put another 1-byte block, so the storage memory usage remains at `1000L`.
    
    Since this deviates from actual block manager behavior I think this line warrants a two-line comment, maybe something like:
    ```
    // Note: We evicted 1 byte to put another 1-byte block in, so the storage memory used remains
    // at 1000 bytes. This is different from real behavior, where the 1-byte block would have evicted
    // the 1000-byte block entirely. This is set up differently so we can write finer-grained tests.
    ```


---
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-12165][SPARK-12189] Fix bugs in evictio...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/10170#issuecomment-163085038
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47378/
    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-12165][SPARK-12189] Fix bugs in evictio...

Posted by JoshRosen <gi...@git.apache.org>.

Github user JoshRosen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/10170#discussion_r47038991
  
    --- Diff: core/src/test/scala/org/apache/spark/memory/UnifiedMemoryManagerSuite.scala ---
    @@ -154,32 +177,34 @@ class UnifiedMemoryManagerSuite extends MemoryManagerSuite with PrivateMethodTes
         assert(mm.acquireExecutionMemory(800L, taskAttemptId, MemoryMode.ON_HEAP) === 800L)
         assert(mm.executionMemoryUsed === 800L)
         assert(mm.storageMemoryUsed === 0L)
    -    assertEnsureFreeSpaceNotCalled(ms)
    +    assertEvictBlocksToFreeSpaceNotCalled(ms)
         // Storage should not be able to evict execution
         assert(mm.acquireStorageMemory(dummyBlock, 100L, evictedBlocks))
         assert(mm.executionMemoryUsed === 800L)
         assert(mm.storageMemoryUsed === 100L)
    -    assertEnsureFreeSpaceCalled(ms, 100L)
    +    assertEvictBlocksToFreeSpaceNotCalled(ms)
         assert(!mm.acquireStorageMemory(dummyBlock, 250L, evictedBlocks))
         assert(mm.executionMemoryUsed === 800L)
         assert(mm.storageMemoryUsed === 100L)
    -    assertEnsureFreeSpaceCalled(ms, 250L)
    +    assertEvictBlocksToFreeSpaceCalled(ms, 150L) // try to evict blocks ...
    +    assert(evictedBlocks.isEmpty) // ... but don't evict since evicting will not be sufficient
         mm.releaseExecutionMemory(maxMemory, taskAttemptId, MemoryMode.ON_HEAP)
         mm.releaseStorageMemory(maxMemory)
         // Acquire some execution memory again, but this time keep it within the execution region
         assert(mm.acquireExecutionMemory(200L, taskAttemptId, MemoryMode.ON_HEAP) === 200L)
         assert(mm.executionMemoryUsed === 200L)
         assert(mm.storageMemoryUsed === 0L)
    -    assertEnsureFreeSpaceNotCalled(ms)
    +    assertEvictBlocksToFreeSpaceNotCalled(ms)
         // Storage should still not be able to evict execution
         assert(mm.acquireStorageMemory(dummyBlock, 750L, evictedBlocks))
         assert(mm.executionMemoryUsed === 200L)
         assert(mm.storageMemoryUsed === 750L)
    -    assertEnsureFreeSpaceCalled(ms, 750L)
    +    assertEvictBlocksToFreeSpaceNotCalled(ms) // since there was 800 bytes free
         assert(!mm.acquireStorageMemory(dummyBlock, 850L, evictedBlocks))
         assert(mm.executionMemoryUsed === 200L)
         assert(mm.storageMemoryUsed === 750L)
    -    assertEnsureFreeSpaceCalled(ms, 850L)
    +    assertEvictBlocksToFreeSpaceCalled(ms, 800L) // try to evict blocks...
    --- End diff --
    
    Yep, this was addressed by changing `maxMemory` to `maxStorageMemory` in the check at the start of `acquireStorageMemory`.


---
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-12165][SPARK-12189] Fix bugs in evictio...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/10170#issuecomment-163367683
  
    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-12165][SPARK-12189] Fix bugs in evictio...

Posted by andrewor14 <gi...@git.apache.org>.

Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/10170#discussion_r46997970
  
    --- Diff: core/src/main/scala/org/apache/spark/memory/UnifiedMemoryManager.scala ---
    @@ -100,7 +100,7 @@ private[spark] class UnifiedMemoryManager private[memory] (
           case MemoryMode.OFF_HEAP =>
             // For now, we only support on-heap caching of data, so we do not need to interact with
             // the storage pool when allocating off-heap memory. This will change in the future, though.
    -        super.acquireExecutionMemory(numBytes, taskAttemptId, memoryMode)
    +        offHeapExecutionMemoryPool.acquireMemory(numBytes, taskAttemptId)
    --- End diff --
    
    +1 to this change. It's much less brittle!


---
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-12165][SPARK-12189] Fix bugs in evictio...

Posted by JoshRosen <gi...@git.apache.org>.

Github user JoshRosen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/10170#discussion_r47037149
  
    --- Diff: core/src/test/scala/org/apache/spark/memory/MemoryManagerSuite.scala ---
    @@ -36,105 +37,98 @@ import org.apache.spark.storage.{BlockId, BlockStatus, MemoryStore, StorageLevel
     /**
      * Helper trait for sharing code among [[MemoryManager]] tests.
      */
    -private[memory] trait MemoryManagerSuite extends SparkFunSuite {
    +private[memory] trait MemoryManagerSuite extends SparkFunSuite with BeforeAndAfterEach {
     
    -  import MemoryManagerSuite.DEFAULT_ENSURE_FREE_SPACE_CALLED
    +  protected val evictedBlocks = new mutable.ArrayBuffer[(BlockId, BlockStatus)]
    +
    +  import MemoryManagerSuite.DEFAULT_EVICT_BLOCKS_TO_FREE_SPACE_CALLED
     
       // Note: Mockito's verify mechanism does not provide a way to reset method call counts
       // without also resetting stubbed methods. Since our test code relies on the latter,
    -  // we need to use our own variable to track invocations of `ensureFreeSpace`.
    +  // we need to use our own variable to track invocations of `evictBlocksToFreeSpace`.
     
       /**
    -   * The amount of free space requested in the last call to [[MemoryStore.ensureFreeSpace]]
    +   * The amount of space requested in the last call to [[MemoryStore.evictBlocksToFreeSpace]].
        *
    -   * This set whenever [[MemoryStore.ensureFreeSpace]] is called, and cleared when the test
    -   * code makes explicit assertions on this variable through [[assertEnsureFreeSpaceCalled]].
    +   * This set whenever [[MemoryStore.evictBlocksToFreeSpace]] is called, and cleared when the test
    +   * code makes explicit assertions on this variable through
    +   * [[assertEvictBlocksToFreeSpaceCalled]].
        */
    -  private val ensureFreeSpaceCalled = new AtomicLong(DEFAULT_ENSURE_FREE_SPACE_CALLED)
    +  private val evictBlocksToFreeSpaceCalled = new AtomicLong(0)
    +
    +  override def beforeEach(): Unit = {
    +    super.beforeEach()
    +    evictedBlocks.clear()
    +    evictBlocksToFreeSpaceCalled.set(DEFAULT_EVICT_BLOCKS_TO_FREE_SPACE_CALLED)
    +  }
     
       /**
    -   * Make a mocked [[MemoryStore]] whose [[MemoryStore.ensureFreeSpace]] method is stubbed.
    +   * Make a mocked [[MemoryStore]] whose [[MemoryStore.evictBlocksToFreeSpace]] method is stubbed.
        *
    -   * This allows our test code to release storage memory when [[MemoryStore.ensureFreeSpace]]
    -   * is called without relying on [[org.apache.spark.storage.BlockManager]] and all of its
    -   * dependencies.
    +   * This allows our test code to release storage memory when these methods are called
    +   * without relying on [[org.apache.spark.storage.BlockManager]] and all of its dependencies.
        */
       protected def makeMemoryStore(mm: MemoryManager): MemoryStore = {
    -    val ms = mock(classOf[MemoryStore])
    -    when(ms.ensureFreeSpace(anyLong(), any())).thenAnswer(ensureFreeSpaceAnswer(mm, 0))
    -    when(ms.ensureFreeSpace(any(), anyLong(), any())).thenAnswer(ensureFreeSpaceAnswer(mm, 1))
    +    val ms = mock(classOf[MemoryStore], RETURNS_SMART_NULLS)
    +    when(ms.evictBlocksToFreeSpace(any(), anyLong(), any()))
    +      .thenAnswer(evictBlocksToFreeSpaceAnswer(mm))
         mm.setMemoryStore(ms)
         ms
       }
     
       /**
    -   * Make an [[Answer]] that stubs [[MemoryStore.ensureFreeSpace]] with the right arguments.
    -   */
    -  private def ensureFreeSpaceAnswer(mm: MemoryManager, numBytesPos: Int): Answer[Boolean] = {
    +    * Simulate the part of [[MemoryStore.evictBlocksToFreeSpace]] that releases storage memory.
    +    *
    +    * This is a significant simplification of the real method, which actually drops existing
    +    * blocks based on the size of each block. Instead, here we simply release as many bytes
    +    * as needed to ensure the requested amount of free space. This allows us to set up the
    +    * test without relying on the [[org.apache.spark.storage.BlockManager]], which brings in
    +    * many other dependencies.
    +    *
    +    * Every call to this method will set a global variable, [[evictBlocksToFreeSpaceCalled]], that
    +    * records the number of bytes this is called with. This variable is expected to be cleared
    +    * by the test code later through [[assertEvictBlocksToFreeSpaceCalled]].
    +    */
    +  private def evictBlocksToFreeSpaceAnswer(mm: MemoryManager): Answer[Boolean] = {
         new Answer[Boolean] {
           override def answer(invocation: InvocationOnMock): Boolean = {
             val args = invocation.getArguments
    -        require(args.size > numBytesPos, s"bad test: expected >$numBytesPos arguments " +
    -          s"in ensureFreeSpace, found ${args.size}")
    -        require(args(numBytesPos).isInstanceOf[Long], s"bad test: expected ensureFreeSpace " +
    -          s"argument at index $numBytesPos to be a Long: ${args.mkString(", ")}")
    -        val numBytes = args(numBytesPos).asInstanceOf[Long]
    -        val success = mockEnsureFreeSpace(mm, numBytes)
    -        if (success) {
    +        val numBytesToFree = args(1).asInstanceOf[Long]
    +        assert(numBytesToFree > 0)
    +        require(evictBlocksToFreeSpaceCalled.get() === DEFAULT_EVICT_BLOCKS_TO_FREE_SPACE_CALLED,
    +          "bad test: evictBlocksToFreeSpace() variable was not reset")
    +        evictBlocksToFreeSpaceCalled.set(numBytesToFree)
    +        if (numBytesToFree <= mm.storageMemoryUsed) {
    +          // We can evict enough blocks to fulfill the request for space
    +          mm.releaseStorageMemory(numBytesToFree)
               args.last.asInstanceOf[mutable.Buffer[(BlockId, BlockStatus)]].append(
    -            (null, BlockStatus(StorageLevel.MEMORY_ONLY, numBytes, 0L, 0L)))
    +            (null, BlockStatus(StorageLevel.MEMORY_ONLY, numBytesToFree, 0L, 0L)))
    +          evictedBlocks.append(
    +            (null, BlockStatus(StorageLevel.MEMORY_ONLY, numBytesToFree, 0L, 0L)))
    --- End diff --
    
    If you just add it to the one in the invocation args, then you won't be able to assert that execution memory requests resulted in the eviction of blocks, since the execution request doesn't pass in the `evictedBlocks` from the suite.
    
    A simple fix is to only add if `!(args.last eq evictedBlocks)`.


---
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-12165][SPARK-12189] Fix bugs in evictio...

Posted by JoshRosen <gi...@git.apache.org>.

Github user JoshRosen commented on the pull request:

    https://github.com/apache/spark/pull/10170#issuecomment-163086853
  
    Note to self RE: unroll fraction (just to be precise):
    
    At a high-level, the StaticMemoryManager a request for unroll memory will be able to obtain more memory if:
    
    - there is free storage memory, or
    - `currentUnrollMemory / maxStorageMemory` < `spark.memory.unrollFraction`.
    
    An unroll request can only evict blocks if  `finalUnrollMemory / maxStorageMemory` < `spark.memory.unrollFraction`, where `finalUnrollMemory` is the amount of unroll memory that would be used after the eviction was performed.


---
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