Cleanup ExternalSorter metrics (#5885) #6364
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tustvold
commented
May 16, 2023
Comment on lines
-54
to
-61
| /// count of spills during the execution of the operator | ||
| spill_count: Count, | ||
|
|
||
| /// total spilled bytes during the execution of the operator | ||
| spilled_bytes: Count, | ||
|
|
||
| /// current memory usage for the operator | ||
| mem_used: Gauge, |
There was a problem hiding this comment.
These were only used by ExternalSorter and so I figure aren't really Baseline
tustvold
force-pushed
the
cleanup-sort-metrics
branch
from
93b24f0 to
09a798a
Compare
tustvold
commented
May 16, 2023
| /// multiple in-mem sort metrics and final merge-sort metrics from `SortPreservingMergeStream`. | ||
| /// Therefore, We need a separation of metrics for which are final metrics (for output_rows accumulation), | ||
| /// and which are intermediate metrics that we only account for elapsed_compute time. | ||
| pub struct CompositeMetricsSet { |
There was a problem hiding this comment.
BaselineMetrics::intermediate replaces the need for this
tustvold
commented
May 16, 2023
|
|
||
| /// Wraps a [`BaselineMetrics`] and records memory usage on a [`MemoryReservation`] | ||
| #[derive(Debug)] | ||
| pub struct MemTrackingMetrics { |
There was a problem hiding this comment.
I think this construction may date from an earlier iteration of the memory tracking, as it stands now it makes little sense.
The memory reported by the mem_used metric will be a somewhat arbitrary value based on the last value at the point the plan finished. Additionally there isn't any way to use the MemoryReservation in a fallible manner. It felt easier to just separate the concerns of reporting plan metrics from tracking runtime memory usage.
tustvold
force-pushed
the
cleanup-sort-metrics
branch
from
09a798a to
eafefe4
Compare
alamb
reviewed
May 16, 2023
| @@ -56,6 +55,27 @@ use tempfile::NamedTempFile; | |||
| use tokio::sync::mpsc::{Receiver, Sender}; | |||
| use tokio::task; | |||
|
|
|||
| struct ExternalSorterMetrics { | |||
There was a problem hiding this comment.
I agree it is much nicer to put sorting metrics on the sorter rather than BaselineMetrics
|
My benchmark run shows no changes in performance, as expected |
richox
pushed a commit
to richox/arrow-datafusion
that referenced
this pull request
May 29, 2023
)" This reverts commit cf81117.
richox
pushed a commit
to richox/arrow-datafusion
that referenced
this pull request
May 29, 2023
)" This reverts commit cf81117.
richox
pushed a commit
to richox/arrow-datafusion
that referenced
this pull request
Jul 5, 2023
)" This reverts commit cf81117.
richox
pushed a commit
to richox/arrow-datafusion
that referenced
this pull request
Jul 5, 2023
)" This reverts commit cf81117.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #5885
Part of #5108
Rationale for this change
In preparation for improving the memory accounting in ExternalSorter / SortPreservingMerge I first wanted to sanitise what already existed. I will call out the various changes in the PR
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
mem_used, spill_count and spilled_bytes are no longer included in BaselineMetrics (as they were only used by ExternalSorter)
CompositeMetricsSet and MemTrackingMetrics have been removed