[GitHub] [spark] viirya commented on a change in pull request #34846: [SPARK-37593][CORE] Optimize HeapMemoryAllocator to avoid memory waste when using G1GC

[GitBox <gi...@apache.org>]

viirya commented on a change in pull request #34846:
URL: https://github.com/apache/spark/pull/34846#discussion_r771904457



##########
File path: core/src/main/scala/org/apache/spark/memory/MemoryManager.scala
##########
@@ -242,22 +243,30 @@ private[spark] abstract class MemoryManager(
    * If user didn't explicitly set "spark.buffer.pageSize", we figure out the default value
    * by looking at the number of cores available to the process, and the total amount of memory,
    * and then divide it by a factor of safety.
+   *
+   * SPARK-37593 If we are using G1GC, it's better to take the LONG_ARRAY_OFFSET
+   * into consideration so that the requested memory size is power of 2
+   * and can be divided by G1 heap region size to reduce memory waste within one G1 region.
    */
-  val pageSizeBytes: Long = {
-    val minPageSize = 1L * 1024 * 1024   // 1MB
-    val maxPageSize = 64L * minPageSize  // 64MB
-    val cores = if (numCores > 0) numCores else Runtime.getRuntime.availableProcessors()
-    // Because of rounding to next power of 2, we may have safetyFactor as 8 in worst case
-    val safetyFactor = 16
-    val maxTungstenMemory: Long = tungstenMemoryMode match {
-      case MemoryMode.ON_HEAP => onHeapExecutionMemoryPool.poolSize
-      case MemoryMode.OFF_HEAP => offHeapExecutionMemoryPool.poolSize
-    }
-    val size = ByteArrayMethods.nextPowerOf2(maxTungstenMemory / cores / safetyFactor)
-    val default = math.min(maxPageSize, math.max(minPageSize, size))
-    conf.get(BUFFER_PAGESIZE).getOrElse(default)
+  private lazy val defaultPageSizeBytes = Utils.maybeG1HeapRegionSize match {
+    case Some(heapRegionSize) if tungstenMemoryMode == MemoryMode.ON_HEAP =>
+      heapRegionSize - Platform.LONG_ARRAY_OFFSET
+    case _ =>

Review comment:
       Hmm, so as this is now limited by `G1HeapRegionSize`, so the maximum `pageSizeBytes` will be reduced to `32MB - Platform.LONG_ARRAY_OFFSET` after this change? What does it possibly affect users?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org