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

Track memory usage in Non Limited Operators #1569

Closed
Tracked by #587
alamb opened this issue Jan 15, 2022 · 4 comments · Fixed by #1691
Closed
Tracked by #587

Track memory usage in Non Limited Operators #1569

alamb opened this issue Jan 15, 2022 · 4 comments · Fixed by #1691

Comments

@alamb
Copy link
Contributor

alamb commented Jan 15, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
Ensure that physical ExecutionPlan nodes properly report their memory usage so DataFusion's can properly stay under its memory budget

Describe the solution you'd like

  1. Figure out a pattern to make ExecutionPlans be Tracking memory consumers (api in Initial MemoryManager and DiskManager APIs for query execution + External Sort implementation #1526)
  2. Apply that pattern to all operators built into DataFusion

Describe alternatives you've considered
N/A

Context
This is follow on work from the great PR from @yjshen in #1526 and part of the story of limiting memory used by DataFusion #587

@yjshen
Copy link
Member

yjshen commented Jan 24, 2022

I want to propose adding a new kind of Metric: Gauge, which is much like the current Count but provide two extra methods sub and set:

#[derive(Debug, Clone)]
pub struct Gauge {
    /// value of the metric gauge
    value: std::sync::Arc<AtomicUsize>,
}

impl Gauge {
    /// create a new gauge
    pub fn new() -> Self {
        Self {
            value: Arc::new(AtomicUsize::new(0)),
        }
    }

    /// Add `n` to the metric's value
    pub fn add(&self, n: usize) {
        // relaxed ordering for operations on `value` poses no issues
        // we're purely using atomic ops with no associated memory ops
        self.value.fetch_add(n, Ordering::Relaxed);
    }

    /// Substract `n` from the metric's value
    pub fn sub(&self, n: usize) {
    }

    /// set the metric's value to `n`
    pub fn set(&self, n: usize) {
    }

    /// Get the current value
    pub fn value(&self) -> usize {
        self.value.load(Ordering::Relaxed)
    }
}

And by adding CurrentMemoryUsage(Gauge) to BaselineMetrics, we could achieve the goal of memory tracking more easily.

Besides, a MaxMemoryUsage(Gauge) could be used, providing more configuration optimization info to users.

cc @alamb @houqp for more thoughts.

@houqp
Copy link
Member

houqp commented Jan 24, 2022

Yeah makes sense to me, i am actually surprised we don't have gauge type already. Do we really need the add and sub method? I was under the impression that you almost always call set on guage type metrics directly.

@yjshen
Copy link
Member

yjshen commented Jan 24, 2022

add will be helpful for ConsumerType::Requesting since we are growing memory usage each time we acquire. I haven't thought of a use case for sub yet. Maybe leave that until we see the need.

@alamb
Copy link
Contributor Author

alamb commented Jan 24, 2022

I agree getting a Gauge sounds like a good idea. thanks for the writeup @yjshen

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 a pull request may close this issue.

3 participants