Skip to content

Conversation

@LuciferYang
Copy link
Contributor

@LuciferYang LuciferYang commented Jan 4, 2024

What changes were proposed in this pull request?

The following code exists in branch-3.5:

// Here and below, put task metric peaks in a WrappedArray to expose them as a Seq
// without requiring a copy.
val metricPeaks = WrappedArray.make(metricsPoller.getTaskMetricPeaks(taskId))
val reason = TaskKilled(t.reason, accUpdates, accums, metricPeaks.toSeq)
plugins.foreach(_.onTaskFailed(reason))
execBackend.statusUpdate(taskId, TaskState.KILLED, ser.serialize(reason))

From the code comments, when using Scala 2.12, metricsPoller.getTaskMetricPeaks(taskId) is wrapped as WrappedArray to avoid collection copying, and the subsequent metricPeaks.toSeq is a redundant collection conversion for Scala 2.12.

However, for Scala 2.13, if metricsPoller.getTaskMetricPeaks(taskId) is still wrapped as WrappedArray/mutable.ArraySeq, it is impossible to avoid collection conversion, because the subsequent metricPeaks.toSeq will trigger a collection copy when using Scala 2.13.

So this pr changes the process to directly wrap metricPeaks as immutable.ArraySeq to ensure the same effect as when using Scala 2.12, which is also more in line with the original comment description.

Why are the changes needed?

Avoid unnecessary collection copying when using Scala 2.13.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass GitHub Actions

Was this patch authored or co-authored using generative AI tooling?

No

@LuciferYang LuciferYang changed the title [SPARK-46585][CORE] Directly constructed metricPeaks as an immutable.ArraySeq instead of use mutable.ArraySeq.toSeq in Executor [SPARK-46585][CORE] Directly constructed metricPeaks as an immutable.ArraySeq instead of use mutable.ArraySeq.toSeq in Executor Jan 4, 2024
@github-actions github-actions bot added the CORE label Jan 4, 2024
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

@LuciferYang
Copy link
Contributor Author

Merged into master for Spark 4.0. Thanks @dongjoon-hyun ~

@LuciferYang LuciferYang deleted the executor-metricPeaks branch January 4, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants