Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-9411] [SQL] Make Tungsten page sizes configurable #7741

Closed
wants to merge 7 commits into from
Closed

[SPARK-9411] [SQL] Make Tungsten page sizes configurable #7741

wants to merge 7 commits into from

Conversation

JoshRosen
Copy link
Contributor

We need to make page sizes configurable so we can reduce them in unit tests and increase them in real production workloads. These sizes are now controlled by a new configuration, spark.buffer.pageSize. The new default is 64 megabytes.

@JoshRosen JoshRosen mentioned this pull request Jul 29, 2015
Closed
17 tasks
@JoshRosen
Copy link
Contributor Author

Decided to defer renaming of other configurations to a followup patch.

@JoshRosen JoshRosen changed the title [SPARK-9411] [SQL] [WIP] Make Tungsten page sizes configurable [SPARK-9411] [SQL] [WIP] Make Tungsten page sizes configurable; lower default page size to 4MB Jul 29, 2015
@JoshRosen JoshRosen changed the title [SPARK-9411] [SQL] [WIP] Make Tungsten page sizes configurable; lower default page size to 4MB [SPARK-9411] [SQL] [WIP] Make Tungsten page sizes configurable Jul 29, 2015
@JoshRosen JoshRosen changed the title [SPARK-9411] [SQL] [WIP] Make Tungsten page sizes configurable [SPARK-9411] [SQL] Make Tungsten page sizes configurable Jul 29, 2015
@SparkQA
Copy link

SparkQA commented Jul 29, 2015

Test build #38796 has finished for PR 7741 at commit bccfb51.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

@rxin, I tested #7564 rebased on top of the changes here and it looks like these changes are sufficient to get HiveCompatibilitySuite to run without memory limit errors. Therefore, I think that this PR should be ready to merge pending review. I opted not to rename the other configurations yet since that's not on the critical path to getting unsafe enabled for QA.

rxin
rxin reviewed Jul 29, 2015
View reviewed changes
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
@@ -147,9 +145,12 @@ public void spill() throws IOException {
}

private long getMemoryUsage() {
return sorter.getMemoryUsage() + (allocatedPages.size() * (long) PAGE_SIZE);
long totalPageSize = 0;
for (MemoryBlock page : allocatedPages) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this loop necessary? are page sizes different?

If this is called a lot, it might be better to just maintain an incremental view of this, rather than calculating it each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not different yet, but this was just some forward-thinking work when I consider how we'l handle large records.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only called when spilling and the corresponding methods in the other classes are only called in debugging code which isn't called in normal operation. Therefore, I'm not planning to change this in this patch.

@rxin
Copy link
Contributor

rxin commented Jul 29, 2015

lgtm otherwise

@rxin
Copy link
Contributor

rxin commented Jul 29, 2015

LGTM.

@rxin
Copy link
Contributor

rxin commented Jul 29, 2015

cc @zsxwing can you review this? (even after it's merged)

Thanks.

@SparkQA
Copy link

SparkQA commented Jul 29, 2015

Test build #1228 timed out for PR 7741 at commit 2c0eefc after a configured wait of 175m.

@SparkQA
Copy link

SparkQA commented Jul 29, 2015

Test build #38879 has finished for PR 7741 at commit a43c4db.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Jul 29, 2015

Thanks - I've merged this.

@asfgit asfgit closed this in 1b0099f Jul 29, 2015
asfgit pushed a commit that referenced this pull request Jul 30, 2015
@JoshRosen @rxin
[SPARK-8850] [SQL] Enable Unsafe mode by default
520ec0f 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JoshRosen @SparkQA @rxin