[SPARK-8357] Fix unsafe memory leak on empty inputs in GeneratedAggregate #7560
Conversation
| if (!iter.hasNext) { | ||
| // This is an empty input, so return early so that we do not allocate data structures | ||
| // that won't be cleaned up (see SPARK-8357). | ||
| if (groupingExpressions.isEmpty) { |
There was a problem hiding this comment.
Here, I made a slight simplification compared to @navis's original patch: if groupingExpressions is empty and the input is empty, then always return an empty aggregation buffer. @navis's patch contained an additional branch here which would skip this output if partial = true, but I think that is an unnecessary performance optimization given that the non-generated-Aggregate operator still outputs an empty row even on empty inputs. Removing this branch means fewer cases to have to test.
|
/cc @rxin, I'm planning to merge this pending tests to help unblock other work on enabling Unsafe by default. |
|
Test build #37921 has finished for PR 7560 at commit
|
|
Huh, looks like this failed a HiveCompatibilitySuite test for a query that doesn't even use the GeneratedAggregate operator: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/37921/testReport/org.apache.spark.sql.hive.execution/HiveCompatibilitySuite/partcols1/ |
|
Jenkins, retest this please. |
|
Test build #37933 has finished for PR 7560 at commit
|
|
Jenkins, retest this please. |
|
(Looks like master might be broken?) |
|
Test build #37957 has finished for PR 7560 at commit
|
|
I think that the test failure here is unrelated, so I'm going to give this one final look and then will merge it into master. |
This pull request enables Unsafe mode by default in Spark SQL. In order to do this, we had to fix a number of small issues: **List of fixed blockers**: - [x] Make some default buffer sizes configurable so that HiveCompatibilitySuite can run properly (#7741). - [x] Memory leak on grouped aggregation of empty input (fixed by #7560 to fix this) - [x] Update planner to also check whether codegen is enabled before planning unsafe operators. - [x] Investigate failing HiveThriftBinaryServerSuite test. This turns out to be caused by a ClassCastException that occurs when Exchange tries to apply an interpreted RowOrdering to an UnsafeRow when range partitioning an RDD. This could be fixed by #7408, but a shorter-term fix is to just skip the Unsafe exchange path when RangePartitioner is used. - [x] Memory leak exceptions masking exceptions that actually caused tasks to fail (will be fixed by #7603). - [x] ~~https://issues.apache.org/jira/browse/SPARK-9162, to implement code generation for ScalaUDF. This is necessary for `UDFSuite` to pass. For now, I've just ignored this test in order to try to find other problems while we wait for a fix.~~ This is no longer necessary as of #7682. - [x] Memory leaks from Limit after UnsafeExternalSort cause the memory leak detector to fail tests. This is a huge problem in the HiveCompatibilitySuite (fixed by f4ac642a4e5b2a7931c5e04e086bb10e263b1db6). - [x] Tests in `AggregationQuerySuite` are failing due to NaN-handling issues in UnsafeRow, which were fixed in #7736. - [x] `org.apache.spark.sql.ColumnExpressionSuite.rand` needs to be updated so that the planner check also matches `TungstenProject`. - [x] After having lowered the buffer sizes to 4MB so that most of HiveCompatibilitySuite runs: - [x] Wrong answer in `join_1to1` (fixed by #7680) - [x] Wrong answer in `join_nulls` (fixed by #7680) - [x] Managed memory OOM / leak in `lateral_view` - [x] Seems to hang indefinitely in `partcols1`. This might be a deadlock in script transformation or a bug in error-handling code? The hang was fixed by #7710. - [x] Error while freeing memory in `partcols1`: will be fixed by #7734. - [x] After fixing the `partcols1` hang, it appears that a number of later tests have issues as well. - [x] Fix thread-safety bug in codegen fallback expression evaluation (#7759). Author: Josh Rosen <[email protected]> Closes #7564 from JoshRosen/unsafe-by-default and squashes the following commits: 83c0c56 [Josh Rosen] Merge remote-tracking branch 'origin/master' into unsafe-by-default f4cc859 [Josh Rosen] Merge remote-tracking branch 'origin/master' into unsafe-by-default 963f567 [Josh Rosen] Reduce buffer size for R tests d6986de [Josh Rosen] Lower page size in PySpark tests 013b9da [Josh Rosen] Also match TungstenProject in checkNumProjects 5d0b2d3 [Josh Rosen] Add task completion callback to avoid leak in limit after sort ea250da [Josh Rosen] Disable unsafe Exchange path when RangePartitioning is used 715517b [Josh Rosen] Enable Unsafe by default
This patch fixes a managed memory leak in GeneratedAggregate. The leak occurs when the unsafe aggregation path is used to perform grouped aggregation on an empty input; in this case, GeneratedAggregate allocates an UnsafeFixedWidthAggregationMap that is never cleaned up because
next()is never called on the aggregate result iterator.This patch fixes this by short-circuiting on empty inputs.
This patch is an updated version of #6810.
Closes #6810.