chore: Reserve memory for native shuffle writer per partition #988

viirya · 2024-09-30T20:32:01Z

Which issue does this PR close?

Closes #887.

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Kontinuation · 2024-10-01T06:16:38Z

I've copied the tests on my branch to this PR and the test hangs:

running 6 tests
test execution::datafusion::shuffle_writer::test::test_slot_size ... ok
test execution::datafusion::shuffle_writer::test::test_pmod ... ok
test execution::datafusion::shuffle_writer::test::test_insert_larger_batch ... ok
test execution::datafusion::shuffle_writer::test::test_insert_smaller_batch ... ok
test execution::datafusion::shuffle_writer::test::test_large_number_of_partitions has been running for over 60 seconds
test execution::datafusion::shuffle_writer::test::test_large_number_of_partitions_spilling has been running for over 60 seconds
^C

It is possibly caused by deadlocking on buffered_partitions.lock() when spilling is triggered.

viirya · 2024-10-01T23:14:24Z

Thanks. I knew the cause of the deadlocks. I'm going to revamp some codes.

.github/actions/rust-test/action.yaml

codecov-commenter · 2024-10-09T18:15:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 33.97%. Comparing base (c3023c5) to head (e678cb0).
Report is 25 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #988      +/-   ##
============================================
- Coverage     34.03%   33.97%   -0.07%     
+ Complexity      875      857      -18     
============================================
  Files           112      112              
  Lines         43289    43426     +137     
  Branches       9572     9622      +50     
============================================
+ Hits          14734    14752      +18     
- Misses        25521    25630     +109     
- Partials       3034     3044      +10

Flag	Coverage Δ
	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

viirya · 2024-10-09T21:22:51Z

Hmm, these tests for large partition number shuffle fail on MacOS runners only. And no stack trace...But I cannot reproduce it locally.

viirya · 2024-10-09T21:41:01Z

Okay, it is the error I expected before:

ret: Err(ArrowError(ExternalError(IoError(Custom { kind: Uncategorized, error: PathError { path: "/var/folders/t_/mmhnh941511_hp2lwh383bp00000gn/T/.tmpQv8o2b/.tmpioYozN", err: Os { code: 24, kind: Uncategorized, message: "Too many open files" } } })), None))

But I had increase it by ulimit. It doesn't help.

viirya · 2024-10-09T22:05:10Z

native/core/src/execution/datafusion/shuffle_writer.rs

+
+    #[test]
+    #[cfg_attr(miri, ignore)] // miri can't call foreign function `ZSTD_createCCtx`
+    #[cfg(not(target_os = "macos"))] // Github MacOS runner fails with "Too many open files".


These tests fail on MacOS runners with "Too many open files" error. ulimit cannot help too.

I skip them on MacOS runners. We have ubuntu runners to test them.

The test shuffle_write_test(10000, 10, 200, Some(10 * 1024 * 1024)) spilled 1700 times, it spills too frequently for data of this size. Seems that the excessive spilling problem is inevitable if we reserve full batch capacity for the arrow builder.

This PR seems like an important improvement because it now uses the memory pool features. Perhaps we can follow up with optimizations to reduce spilling. wdyt @Kontinuation?

Sure. Let's merge this.

I'm also considering adding a native sort-based shuffle writer that works better with constraint resources.

We've discussed to support sort-based shuffle in the native shuffle writer, similar to Spark shuffle, in the early development. So I think it is on our roadmap though it is not urgent at that moment.

andygrove · 2024-10-11T18:35:03Z

I'm testing this PR out now, in conjunction with some other PRs because I currently have a reproducible deadlock caused by memory pool issues, as far as I can tell.

native/core/src/execution/datafusion/shuffle_writer.rs

andygrove

Thanks @viirya

viirya · 2024-10-14T15:27:45Z

Thanks @andygrove @Kontinuation

…apache#988)" This reverts commit e146cfa.

…rtition (#988)" (#1020) This reverts commit e146cfa.

…apache#988)" This reverts commit e146cfa.

…artition (apache#988)"" This reverts commit 481127d.

…er per partition (apache#988)""" This reverts commit 9469d16.

…fle writer per partition (apache#988)"""" This reverts commit 6002726.

…r per partition (apache#988)" (apache#1020)" This reverts commit 8d097d5.

* Revert "chore: Revert "chore: Reserve memory for native shuffle writer per partition (#988)" (#1020)" This reverts commit 8d097d5. * fix * fix * fix * fix

…HashJoin (#1007) * experiment * fix and add credit * disable by default and make internal * remove sort * minor optimization * minor optimization * remove unused import * disable feature by default * fix dockerfile * Add section to tuning guide * update benchmarking guide * Revert "chore: Reserve memory for native shuffle writer per partition (#988)" This reverts commit e146cfa. * mark feature as experimental and explain risks * workaround for TPC-DS q14 hanging on a RightSemi join * revert a change * remove debug logging: * format * add link to tuning guide

…#988) * chore: Reserve memory for native shuffle writer per partition * Revise * skip large partition number shuffle on macos runners * For review

## Which issue does this PR close?  Closes #. ## Rationale for this change  ## What changes are included in this PR?  ``` cb3e977 perf: Add experimental feature to replace SortMergeJoin with ShuffledHashJoin (apache#1007) 3df9d5c fix: Make comet-git-info.properties optional (apache#1027) 4033687 chore: Reserve memory for native shuffle writer per partition (apache#1022) bd541d6 (public/main) remove hard-coded version number from Dockerfile (apache#1025) e3ac6cf feat: Implement bloom_filter_agg (apache#987) 8d097d5 (origin/main) chore: Revert "chore: Reserve memory for native shuffle writer per partition (apache#988)" (apache#1020) 591f45a chore: Bump arrow-rs to 53.1.0 and datafusion (apache#1001) e146cfa chore: Reserve memory for native shuffle writer per partition (apache#988) abd9f85 fix: Fallback to Spark if named_struct contains duplicate field names (apache#1016) 22613e9 remove legacy comet-spark-shell (apache#1013) d40c802 clarify that Maven central only has jars for Linux (apache#1009) 837c256 docs: Various documentation improvements (apache#1005) 0667c60 chore: Make parquet reader options Comet options instead of Hadoop options (apache#968) 0028f1e fix: Fallback to Spark if scan has meta columns (apache#997) b131cc3 feat: Support `GetArrayStructFields` expression (apache#993) 3413397 docs: Update tuning guide (apache#995) afd28b9 Quality of life fixes for easier hacking (apache#982) 18150fb chore: Don't transform the HashAggregate to CometHashAggregate if Comet shuffle is disabled (apache#991) a1599e2 chore: Update for 0.3.0 release, prepare for 0.4.0 development (apache#970) ``` ## How are these changes tested?

viirya force-pushed the revise_shuffle_memory branch from 18be135 to d063f15 Compare September 30, 2024 20:38

andygrove mentioned this pull request Oct 8, 2024

feat: Use fair-spill pool when spark.memory.offHeap.enabled=false #1004

Closed

viirya force-pushed the revise_shuffle_memory branch from 20b3711 to d2c6102 Compare October 9, 2024 15:42

chore: Reserve memory for native shuffle writer per partition

4595468

viirya force-pushed the revise_shuffle_memory branch 2 times, most recently from 64c7c0d to d25837a Compare October 9, 2024 15:47

Revise

da8d679

viirya force-pushed the revise_shuffle_memory branch from d25837a to da8d679 Compare October 9, 2024 16:07

viirya commented Oct 9, 2024

View reviewed changes

.github/actions/rust-test/action.yaml Outdated Show resolved Hide resolved

viirya force-pushed the revise_shuffle_memory branch from 718c15c to 8172a7c Compare October 9, 2024 17:12

viirya force-pushed the revise_shuffle_memory branch from 5e50f98 to ebf4663 Compare October 9, 2024 22:02

skip large partition number shuffle on macos runners

e121814

viirya force-pushed the revise_shuffle_memory branch from ebf4663 to e121814 Compare October 9, 2024 22:03

viirya commented Oct 9, 2024

View reviewed changes

viirya requested a review from andygrove October 9, 2024 23:50

andygrove reviewed Oct 11, 2024

View reviewed changes

native/core/src/execution/datafusion/shuffle_writer.rs Outdated Show resolved Hide resolved

For review

6763b1e

viirya force-pushed the revise_shuffle_memory branch from cc9e531 to 6763b1e Compare October 12, 2024 16:50

andygrove approved these changes Oct 14, 2024

View reviewed changes

andygrove merged commit e146cfa into apache:main Oct 14, 2024

viirya deleted the revise_shuffle_memory branch October 14, 2024 15:27

andygrove added a commit to andygrove/datafusion-comet that referenced this pull request Oct 15, 2024

Revert "chore: Reserve memory for native shuffle writer per partition (…

7ce8726

…apache#988)" This reverts commit e146cfa.

viirya added a commit to viirya/arrow-datafusion-comet that referenced this pull request Oct 15, 2024

Revert "chore: Reserve memory for native shuffle writer per partition (…

5a33e2a

…apache#988)" This reverts commit e146cfa.

viirya added a commit that referenced this pull request Oct 15, 2024

chore: Revert "chore: Reserve memory for native shuffle writer per pa…

8d097d5

…rtition (#988)" (#1020) This reverts commit e146cfa.

viirya added a commit to viirya/arrow-datafusion-comet that referenced this pull request Oct 16, 2024

Revert "chore: Reserve memory for native shuffle writer per partition (…

481127d

…apache#988)" This reverts commit e146cfa.

viirya added a commit to viirya/arrow-datafusion-comet that referenced this pull request Oct 16, 2024

Revert "Revert "chore: Reserve memory for native shuffle writer per p…

9469d16

…artition (apache#988)"" This reverts commit 481127d.

viirya added a commit to viirya/arrow-datafusion-comet that referenced this pull request Oct 16, 2024

Revert "Revert "Revert "chore: Reserve memory for native shuffle writ…

6002726

…er per partition (apache#988)""" This reverts commit 9469d16.

viirya added a commit to viirya/arrow-datafusion-comet that referenced this pull request Oct 16, 2024

Revert "Revert "Revert "Revert "chore: Reserve memory for native shuf…

418707c

…fle writer per partition (apache#988)"""" This reverts commit 6002726.

viirya added a commit to viirya/arrow-datafusion-comet that referenced this pull request Oct 16, 2024

Revert "chore: Revert "chore: Reserve memory for native shuffle write…

3ec4d63

…r per partition (apache#988)" (apache#1020)" This reverts commit 8d097d5.

viirya mentioned this pull request Oct 16, 2024

chore: Reserve memory for native shuffle writer per partition #1022

Merged

andygrove mentioned this pull request Oct 30, 2024

SparkOutOfMemoryError happens when running CometColumnarExchange #886

Closed

chore: Reserve memory for native shuffle writer per partition #988

chore: Reserve memory for native shuffle writer per partition #988

Uh oh!

Conversation

viirya commented Sep 30, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

Kontinuation commented Oct 1, 2024

Uh oh!

viirya commented Oct 1, 2024

Uh oh!

Uh oh!

codecov-commenter commented Oct 9, 2024

Codecov Report

Uh oh!

viirya commented Oct 9, 2024

Uh oh!

viirya commented Oct 9, 2024

Uh oh!

viirya Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

Kontinuation Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

andygrove Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

Kontinuation Oct 12, 2024

Choose a reason for hiding this comment

Uh oh!

viirya Oct 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented Oct 11, 2024

Uh oh!

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented Oct 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

viirya Oct 12, 2024 •

edited

Loading