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.

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

feat: Improve shuffle metrics (second attempt) #1175

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

andygrove
Copy link
Member

@andygrove andygrove commented Dec 17, 2024

Which issue does this PR close?

N/A

This PR replaces #1173

Rationale for this change

Changes:

  • Fix: Report write time accurately (this is now just the time for writing to disk but it previously included some of the IPC encoding time)
  • New: Number of input batches
  • New: Encoding and compression time
  • New: Total time spent in native shuffle code

Before

2024-12-17_10-34

After

2024-12-17_10-24

What changes are included in this PR?

How are these changes tested?

@andygrove andygrove changed the title [ignore] improve shuffle metrics second attempt feat: Improve shuffle metrics (second attempt) Dec 17, 2024
@andygrove andygrove marked this pull request as ready for review December 17, 2024 17:39
@andygrove
Copy link
Member Author

@mbutrovich @parthchandra fyi

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 34.32%. Comparing base (95727aa) to head (6adb04c).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
...apache/spark/sql/comet/CometCollectLimitExec.scala 50.00% 0 Missing and 1 partial ⚠️
...ark/sql/comet/CometTakeOrderedAndProjectExec.scala 50.00% 0 Missing and 1 partial ⚠️
...t/execution/shuffle/CometShuffleExchangeExec.scala 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1175      +/-   ##
============================================
- Coverage     34.32%   34.32%   -0.01%     
  Complexity      899      899              
============================================
  Files           115      115              
  Lines         43500    43506       +6     
  Branches       9496     9498       +2     
============================================
+ Hits          14931    14932       +1     
- Misses        25659    25661       +2     
- Partials       2910     2913       +3     

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

@parthchandra
Copy link
Contributor

Just for clarification - what's the relation between shuffle write time, encoding and compression total time, and native shuffle total time?
I would think shuffle write time + encoding and compression total time = native shuffle total time, but that does not seem to be the case?

@andygrove
Copy link
Member Author

Just for clarification - what's the relation between shuffle write time, encoding and compression total time, and native shuffle total time? I would think shuffle write time + encoding and compression total time = native shuffle total time, but that does not seem to be the case?

There is also evaluating the partition expressions (typically very fast if they are just column references) and then the time to actually split the batches into partitions.

@parthchandra
Copy link
Contributor

Just for clarification - what's the relation between shuffle write time, encoding and compression total time, and native shuffle total time? I would think shuffle write time + encoding and compression total time = native shuffle total time, but that does not seem to be the case?

There is also evaluating the partition expressions (typically very fast if they are just column references) and then the time to actually split the batches into partitions.

From the above screenshot, shuffle write time + encoding and compression total time = 18.4s and native shuffle total time=28.8s, so there is a difference of 10.4s which is substantial. Wondering if we are missing something.

Nonetheless, the PR certainly improves on the current.

@andygrove
Copy link
Member Author

There is also interaction with the memory pool, which makes JNI calls into synchronized code in the JVM.

I will see if I can make the metrics more complete in this PR.

@andygrove andygrove marked this pull request as draft December 17, 2024 22:43
@andygrove
Copy link
Member Author

@parthchandra The numbers almost add up now.

2024-12-17_16-09

@andygrove andygrove marked this pull request as ready for review December 17, 2024 23:11
@andygrove
Copy link
Member Author

Here is Gluten's equivalent for comparison:

2024-12-17_16-15

@parthchandra
Copy link
Contributor

@parthchandra The numbers almost add up now.

Brilliant!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants