-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-19355][SQL] Use map output statistics to improve global limit's parallelism #16677
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
Changes from 1 commit
4e31bb7
b049cc4
45a1fcb
e9679ba
1a56252
df44243
2d37598
b8a2275
867a93d
55ee6b0
8f779ac
f2a7aac
7598337
e53648e
062b8fd
47f6031
a691e88
5594bf9
c9c8be6
ca00701
21b6948
59a3029
4b443cc
1ff1fa5
a737573
b0cca1a
f24171e
2d522b4
9792220
19d7d75
d05c144
69513d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,5 +23,9 @@ package org.apache.spark | |
| * @param shuffleId ID of the shuffle | ||
| * @param bytesByPartitionId approximate number of output bytes for each map output partition | ||
| * (may be inexact due to use of compressed map statuses) | ||
| * @param numberOfOutput number of output for each pre-map output partition | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does pre-map output partition mean?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is different with
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rename it to |
||
| */ | ||
| private[spark] class MapOutputStatistics(val shuffleId: Int, val bytesByPartitionId: Array[Long]) | ||
| private[spark] class MapOutputStatistics( | ||
| val shuffleId: Int, | ||
| val bytesByPartitionId: Array[Long], | ||
| val numberOfOutput: Array[Int]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, maybe Long is better.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Use |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -156,12 +156,14 @@ private[spark] abstract class MapOutputTracker(conf: SparkConf) extends Logging | |
| // Synchronize on the returned array because, on the driver, it gets mutated in place | ||
| statuses.synchronized { | ||
| val totalSizes = new Array[Long](dep.partitioner.numPartitions) | ||
| for (s <- statuses) { | ||
| val numberOfOutput = new Array[Int](statuses.length) | ||
| statuses.zipWithIndex.map { case (s, index) => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. map -> foreach |
||
| for (i <- 0 until totalSizes.length) { | ||
| totalSizes(i) += s.getSizeForBlock(i) | ||
| } | ||
| numberOfOutput(index) = s.numberOfOutput | ||
| } | ||
| new MapOutputStatistics(dep.shuffleId, totalSizes) | ||
| new MapOutputStatistics(dep.shuffleId, totalSizes, numberOfOutput) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,8 @@ import org.apache.spark.util.Utils | |
|
|
||
| /** | ||
| * Result returned by a ShuffleMapTask to a scheduler. Includes the block manager address that the | ||
| * task ran on as well as the sizes of outputs for each reducer, for passing on to the reduce tasks. | ||
| * task ran on, the sizes of outputs for each reducer, and the number of outputs of the map task, | ||
| * for passing on to the reduce tasks. | ||
| */ | ||
| private[spark] sealed trait MapStatus { | ||
| /** Location where this task was run. */ | ||
|
|
@@ -39,16 +40,18 @@ private[spark] sealed trait MapStatus { | |
| * necessary for correctness, since block fetchers are allowed to skip zero-size blocks. | ||
| */ | ||
| def getSizeForBlock(reduceId: Int): Long | ||
|
|
||
| def numberOfOutput: Int | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The number of output may be greater than 2G?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| } | ||
|
|
||
|
|
||
| private[spark] object MapStatus { | ||
|
|
||
| def apply(loc: BlockManagerId, uncompressedSizes: Array[Long]): MapStatus = { | ||
| def apply(loc: BlockManagerId, uncompressedSizes: Array[Long], numOutput: Int): MapStatus = { | ||
| if (uncompressedSizes.length > 2000) { | ||
| HighlyCompressedMapStatus(loc, uncompressedSizes) | ||
| HighlyCompressedMapStatus(loc, uncompressedSizes, numOutput) | ||
| } else { | ||
| new CompressedMapStatus(loc, uncompressedSizes) | ||
| new CompressedMapStatus(loc, uncompressedSizes, numOutput) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -91,29 +94,34 @@ private[spark] object MapStatus { | |
| */ | ||
| private[spark] class CompressedMapStatus( | ||
| private[this] var loc: BlockManagerId, | ||
| private[this] var compressedSizes: Array[Byte]) | ||
| private[this] var compressedSizes: Array[Byte], | ||
| private[this] var numOutput: Int) | ||
| extends MapStatus with Externalizable { | ||
|
|
||
| protected def this() = this(null, null.asInstanceOf[Array[Byte]]) // For deserialization only | ||
| protected def this() = this(null, null.asInstanceOf[Array[Byte]], -1) // For deserialization only | ||
|
|
||
| def this(loc: BlockManagerId, uncompressedSizes: Array[Long]) { | ||
| this(loc, uncompressedSizes.map(MapStatus.compressSize)) | ||
| def this(loc: BlockManagerId, uncompressedSizes: Array[Long], numOutput: Int) { | ||
| this(loc, uncompressedSizes.map(MapStatus.compressSize), numOutput) | ||
| } | ||
|
|
||
| override def location: BlockManagerId = loc | ||
|
|
||
| override def numberOfOutput: Int = numOutput | ||
|
|
||
| override def getSizeForBlock(reduceId: Int): Long = { | ||
| MapStatus.decompressSize(compressedSizes(reduceId)) | ||
| } | ||
|
|
||
| override def writeExternal(out: ObjectOutput): Unit = Utils.tryOrIOException { | ||
| loc.writeExternal(out) | ||
| out.writeInt(numOutput) | ||
| out.writeInt(compressedSizes.length) | ||
| out.write(compressedSizes) | ||
| } | ||
|
|
||
| override def readExternal(in: ObjectInput): Unit = Utils.tryOrIOException { | ||
| loc = BlockManagerId(in) | ||
| numOutput = in.readInt() | ||
| val len = in.readInt() | ||
| compressedSizes = new Array[Byte](len) | ||
| in.readFully(compressedSizes) | ||
|
|
@@ -133,17 +141,20 @@ private[spark] class HighlyCompressedMapStatus private ( | |
| private[this] var loc: BlockManagerId, | ||
| private[this] var numNonEmptyBlocks: Int, | ||
| private[this] var emptyBlocks: RoaringBitmap, | ||
| private[this] var avgSize: Long) | ||
| private[this] var avgSize: Long, | ||
| private[this] var numOutput: Int) | ||
| extends MapStatus with Externalizable { | ||
|
|
||
| // loc could be null when the default constructor is called during deserialization | ||
| require(loc == null || avgSize > 0 || numNonEmptyBlocks == 0, | ||
| "Average size can only be zero for map stages that produced no output") | ||
|
|
||
| protected def this() = this(null, -1, null, -1) // For deserialization only | ||
| protected def this() = this(null, -1, null, -1, -1) // For deserialization only | ||
|
|
||
| override def location: BlockManagerId = loc | ||
|
|
||
| override def numberOfOutput: Int = numOutput | ||
|
|
||
| override def getSizeForBlock(reduceId: Int): Long = { | ||
| if (emptyBlocks.contains(reduceId)) { | ||
| 0 | ||
|
|
@@ -154,20 +165,25 @@ private[spark] class HighlyCompressedMapStatus private ( | |
|
|
||
| override def writeExternal(out: ObjectOutput): Unit = Utils.tryOrIOException { | ||
| loc.writeExternal(out) | ||
| out.writeInt(numOutput) | ||
| emptyBlocks.writeExternal(out) | ||
| out.writeLong(avgSize) | ||
| } | ||
|
|
||
| override def readExternal(in: ObjectInput): Unit = Utils.tryOrIOException { | ||
| loc = BlockManagerId(in) | ||
| numOutput = in.readInt() | ||
| emptyBlocks = new RoaringBitmap() | ||
| emptyBlocks.readExternal(in) | ||
| avgSize = in.readLong() | ||
| } | ||
| } | ||
|
|
||
| private[spark] object HighlyCompressedMapStatus { | ||
| def apply(loc: BlockManagerId, uncompressedSizes: Array[Long]): HighlyCompressedMapStatus = { | ||
| def apply( | ||
| loc: BlockManagerId, | ||
| uncompressedSizes: Array[Long], | ||
| numOutput: Int): HighlyCompressedMapStatus = { | ||
| // We must keep track of which blocks are empty so that we don't report a zero-sized | ||
| // block as being non-empty (or vice-versa) when using the average block size. | ||
| var i = 0 | ||
|
|
@@ -195,6 +211,6 @@ private[spark] object HighlyCompressedMapStatus { | |
| } | ||
| emptyBlocks.trim() | ||
| emptyBlocks.runOptimize() | ||
| new HighlyCompressedMapStatus(loc, numNonEmptyBlocks, emptyBlocks, avgSize) | ||
| new HighlyCompressedMapStatus(loc, numNonEmptyBlocks, emptyBlocks, avgSize, numOutput) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and elsewhere, simply use
writeMetrics._recordsWritteninstead of addingnumOfRecords?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think it is fine. However, maybe I miss it, but I can't find
SortShuffleWriterhas updatedwriteMetrics_recordsWritten?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are introducing numOfRecords because (as you say) some code paths are not updating the metric.
Instead of working around the bug and cluttering code, it is better to fix it cleanly (unless there is some design issue or more complicated issue).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To verify it, I ran a test locally. Looks like
writeMetrics_recordsWrittenis well updated inSortShuffleWriterpath too. So I will replacenumOfRecordswithwriteMetrics_recordsWritten.