You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/06/10 09:03:34 UTC

[GitHub] [spark] WangGuangxin opened a new pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

WangGuangxin opened a new pull request #28780:
URL: https://github.com/apache/spark/pull/28780


   ### What changes were proposed in this pull request?
   It happends when hash aggregate downgrades to sort based aggregate. 
   `UnsafeExternalSorter.createWithExistingInMemorySorter` calls `spill` on an `InMemorySorter` immediately, but the memory pointed by InMemorySorter is acquired by outside `BytesToBytesMap`, instead the `allocatedPages` in `UnsafeExternalSorter`. So the memory spill bytes metric is always 0, but disk bytes spill metric is right. 
   
   Related code is at https://github.com/apache/spark/blob/master/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java#L232.
   
   It can be reproduced by following step.
   ```
   bin/spark-shell --driver-memory 512m --executor-memory 512m --executor-cores 1 --conf "spark.default.parallelism=1"
   scala> sql("select id, count(1) from range(10000000) group by id").write.csv("/tmp/result.json")
   ```
   
   Before this patch, the metric is
   ![image](https://user-images.githubusercontent.com/1312321/84248141-99cd2500-ab3b-11ea-8821-f78cf483557d.png)
   
   After this patch, the metric is
   ![image](https://user-images.githubusercontent.com/1312321/84248156-a18cc980-ab3b-11ea-86d3-fff85d239ed0.png)
   
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   
   ### How was this patch tested?
   Test manually
   


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

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



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

[GitHub] [spark] Ngone51 commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r443336772



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       Is it the same to use `inMemorySorter.getMemoryUsage`?
   
   Also, shall we update `sorter.totalSpillBytes`?




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

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



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

[GitHub] [spark] WangGuangxin commented on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
WangGuangxin commented on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-647103143


   > spill
   
   It's `memoryBytesSpilled` in `TaskMetrics`,  denotes the number of in-memory bytes spilled.
   


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

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



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

[GitHub] [spark] WangGuangxin commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
WangGuangxin commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r443451092



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       At the end of `sorter.spill`,  it increase memory bytes spilled and disk bytes spilled. 
   But the size of memory bytes spilled is not correct because the real data pages are not `UnsafeExternalSorter.allocatedPages`, but `BytesToBytesMap.dataPages`,  as explained here https://github.com/apache/spark/pull/28780#discussion_r443445290, but the disk bytes spilled is correct because it's the real size write to disk.




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

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



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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-641861116


   Can one of the admins verify this patch?


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

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



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

[GitHub] [spark] Ngone51 commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r443343545



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       Or, if we can do `final long spillSize = freeMemory() + inMemorySorter.getMemoryUsage()` in `sorter.spill()`? 




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

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



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

[GitHub] [spark] WangGuangxin commented on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
WangGuangxin commented on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-647103954


   also cc @Ngone51 @JoshRosen since you are some related changes


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

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



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

[GitHub] [spark] cloud-fan commented on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-647945904


   shall we set `sorter.totalSpillBytes`, then we can update the metrics correctly in `sort.spill`.


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

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



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

[GitHub] [spark] Ngone51 commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r443441511



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       You can see it at the end of `sorter.spill()`.




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

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



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

[GitHub] [spark] WangGuangxin commented on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
WangGuangxin commented on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-647103183


   > I'm not familiar with this part. what does "spill (memory)" mean？
   
   It's memoryBytesSpilled in TaskMetrics, denotes the number of in-memory bytes spilled.


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

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



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

[GitHub] [spark] maropu commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r437992135



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       Why didn't you set this value in the caller side (`UnsafeKVExternalSorter`)?




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

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



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

[GitHub] [spark] AmplabJenkins commented on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-641864887


   Can one of the admins verify this patch?


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

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



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

[GitHub] [spark] Ngone51 commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r443930725



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       I see, thanks for your explanation.




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

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



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

[GitHub] [spark] WangGuangxin commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
WangGuangxin commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r443445290



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       > Is it the same to use `inMemorySorter.getMemoryUsage`?
   > 
   > Also, shall we update `sorter.totalSpillBytes`?
   
   1. It's not the same with `inMemorySorter.getMemoryUsage`.
   
   When we insert a record into `UnsafeExternalSorter`, the record itself is copied to sorter's `allocatedPages`, and stores the memory address into `InMemorySorter`. So when we spill it, the memory size is the sum of `inMemorySorter.getMemoryUsage` 
    and `allocatePages`, that's what it does in `UnsafeExternalSorter.spill`.
   
   But when we do `sorter.spill`  in `UnsafeExternalSorter.createWithExistingInMemorySorter`,  the sorter's `allocatedPages` is empty since we didn't copy any records to it. The memory address in `inMemorySorter` points to the memory pages in `BytesToBytesMap`.
   
   The passed parameter denotes the size of memory pages in `BytesToBytesMap`.
   
   2. yes, seems we also need to update `sorter.totalSpillBytes`




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

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



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

[GitHub] [spark] WangGuangxin commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
WangGuangxin commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r438076494



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       I have no strong preference on this.  One benefits is that others who calls `createWithExistingInMemorySorter` will not forget to update memory spilled any more. (though only `UnsafeKVExternalSorter` used this function currently)




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

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



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

[GitHub] [spark] github-actions[bot] commented on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-702469496


   We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
   If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!


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

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



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

[GitHub] [spark] github-actions[bot] closed pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #28780:
URL: https://github.com/apache/spark/pull/28780


   


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

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



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

[GitHub] [spark] Ngone51 commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r443343545



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       Or, if we can do `final long spillSize = freeMemory() + inMemorySorter.getMemoryUsage()` in `sorter.spill()`? 
   
   Seem like the size of inMemorySorter is included in log info by `getMemoryUsage()`, while `spillSize` doesn't.




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

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



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

[GitHub] [spark] WangGuangxin commented on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
WangGuangxin commented on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-647304796


   > > number of in-memory bytes spilled
   > 
   > what's the difference between it and `spill (disk)`? IIUC `spill` means we dump data from memory to disk.
   
   They are trying to describe the same piece of data when dumping from memory from disk. `spill (memory)` is the size these data occupied in memory, while `spill (disk)` means the size these data occupied in disk (after serialization and compression).


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

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



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

[GitHub] [spark] cloud-fan commented on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-647288933


   > number of in-memory bytes spilled
   
   what's the difference between it and `spill (disk)`? IIUC `spill` means we dump data from memory to disk.


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

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



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

[GitHub] [spark] AmplabJenkins commented on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-641861116


   Can one of the admins verify this patch?


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

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



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

[GitHub] [spark] cloud-fan commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r443374779



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       just for reference, where do we update the disk spill metrics?




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

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



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

[GitHub] [spark] WangGuangxin commented on a change in pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
WangGuangxin commented on a change in pull request #28780:
URL: https://github.com/apache/spark/pull/28780#discussion_r440108353



##########
File path: core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -104,11 +104,13 @@ public static UnsafeExternalSorter createWithExistingInMemorySorter(
       int initialSize,
       long pageSizeBytes,
       int numElementsForSpillThreshold,
-      UnsafeInMemorySorter inMemorySorter) throws IOException {
+      UnsafeInMemorySorter inMemorySorter,
+      long existingMemoryConsumption) throws IOException {
     UnsafeExternalSorter sorter = new UnsafeExternalSorter(taskMemoryManager, blockManager,
       serializerManager, taskContext, recordComparatorSupplier, prefixComparator, initialSize,
         pageSizeBytes, numElementsForSpillThreshold, inMemorySorter, false /* ignored */);
     sorter.spill(Long.MAX_VALUE, sorter);
+    taskContext.taskMetrics().incMemoryBytesSpilled(existingMemoryConsumption);

Review comment:
       cc @maropu @cloud-fan 




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

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



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

[GitHub] [spark] cloud-fan edited a comment on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-647945904


   shall we set `sorter.totalSpillBytes`? then we can update the metrics correctly in `sort.spill`.


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

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



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

[GitHub] [spark] cloud-fan commented on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-644697788


   I'm not familiar with this part. what does "spill (memory)" mean？


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

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



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

[GitHub] [spark] WangGuangxin removed a comment on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
WangGuangxin removed a comment on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-647103143


   > spill
   
   It's `memoryBytesSpilled` in `TaskMetrics`,  denotes the number of in-memory bytes spilled.
   


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

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



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

[GitHub] [spark] cloud-fan edited a comment on pull request #28780: [SPARK-31952][SQL]Fix incorrect memory spill metric when doing Aggregate

Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #28780:
URL: https://github.com/apache/spark/pull/28780#issuecomment-647945904


   shall we set `sorter.totalSpillBytes`? then we can update the metrics correctly in `sorter.spill`.


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

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



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