You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jo...@apache.org on 2020/01/27 18:52:16 UTC
[impala] branch master updated: IMPALA-9291: MemTracker's
TryConsume() should behave consistent with Consume()/Release()
This is an automated email from the ASF dual-hosted git repository.
joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
The following commit(s) were added to refs/heads/master by this push:
new 6ac5152 IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
6ac5152 is described below
commit 6ac51522d8505416e8ef7ae2d911741fd1c4a3e5
Author: Zoltan Borok-Nagy <bo...@cloudera.com>
AuthorDate: Wed Jan 15 14:12:26 2020 +0100
IMPALA-9291: MemTracker's TryConsume() should behave consistent with Consume()/Release()
Before this fix MemTracker::Consume() and Release() could take
negative numbers as arguments. In that case Consume(-val) would invoke
Release(val) and vice versa.
On the other hand, TryConsume() was a no-op for negative values and
returned success. This could lead to errors in the following case:
TryConsume(-42); // no-op
...
Release(-42); // -> Consume(42);
And in the end the mem tracker would think there is 42 bytes of
unallocated memory.
This commit changes mem-tracker to forbid negative values.
Another inconsistency was found between Consume()/Release() and
TryConsume(). When Consume()/Release() was invoked with zero as
argument they didn't update 'consumption_' when 'consumption_metric_'
was not null. On the other hand, TryConsume() being invoked with
zero did update 'consumption_' from the metric. I fixed this
inconsistency as well, i.e. now 'consumption_' is not updated
when the argument is zero.
Testing:
* ran exhaustive tests
Change-Id: I22e613c49e4a6bd218e7f7f7c0d6bec95b75cff5
Reviewed-on: http://gerrit.cloudera.org:8080/15050
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/runtime/mem-tracker-test.cc | 17 +++++++++----
be/src/runtime/mem-tracker.h | 49 ++++++++++++++++++++++----------------
2 files changed, 41 insertions(+), 25 deletions(-)
diff --git a/be/src/runtime/mem-tracker-test.cc b/be/src/runtime/mem-tracker-test.cc
index 755e4ce..44bd9f4 100644
--- a/be/src/runtime/mem-tracker-test.cc
+++ b/be/src/runtime/mem-tracker-test.cc
@@ -152,7 +152,7 @@ TEST(MemTestTest, ConsumptionMetric) {
EXPECT_EQ(t.peak_consumption(), 10);
EXPECT_EQ(neg_t.consumption(), -10);
metric.Increment(-5);
- t.Consume(-1);
+ t.Release(1);
neg_t.Consume(1);
EXPECT_EQ(t.consumption(), 5);
EXPECT_EQ(t.peak_consumption(), 10);
@@ -166,7 +166,7 @@ TEST(MemTestTest, ConsumptionMetric) {
EXPECT_TRUE(t.LimitExceeded(MemLimit::HARD));
EXPECT_EQ(neg_t.consumption(), -155);
metric.Increment(-150);
- t.Consume(-1);
+ t.Release(1);
neg_t.Consume(1);
EXPECT_EQ(t.consumption(), 5);
EXPECT_EQ(t.peak_consumption(), 155);
@@ -175,15 +175,22 @@ TEST(MemTestTest, ConsumptionMetric) {
// consumption_ is not updated when Consume()/Release() is called with a zero value
metric.Increment(10);
t.Consume(0);
- neg_t.Consume(0);
+ neg_t.Release(0);
+ EXPECT_EQ(t.consumption(), 5);
+ EXPECT_EQ(t.peak_consumption(), 155);
+ EXPECT_FALSE(t.LimitExceeded(MemLimit::HARD));
+ EXPECT_EQ(neg_t.consumption(), -5);
+ // consumption_ is not updated when TryConsume() is called with a zero value
+ EXPECT_TRUE(t.TryConsume(0));
+ EXPECT_TRUE(neg_t.TryConsume(0));
EXPECT_EQ(t.consumption(), 5);
EXPECT_EQ(t.peak_consumption(), 155);
EXPECT_FALSE(t.LimitExceeded(MemLimit::HARD));
EXPECT_EQ(neg_t.consumption(), -5);
// Clean up.
metric.Increment(-15);
- t.Consume(-1);
- neg_t.Consume(-1);
+ t.Release(1);
+ neg_t.Release(1);
}
TEST(MemTestTest, TrackerHierarchy) {
diff --git a/be/src/runtime/mem-tracker.h b/be/src/runtime/mem-tracker.h
index 52d15cc..a72a7f3 100644
--- a/be/src/runtime/mem-tracker.h
+++ b/be/src/runtime/mem-tracker.h
@@ -128,11 +128,9 @@ class MemTracker {
/// Increases consumption of this tracker and its ancestors by 'bytes'.
void Consume(int64_t bytes) {
+ DCHECK_GE(bytes, 0);
DCHECK(!closed_) << label_;
- if (bytes <= 0) {
- if (bytes < 0) Release(-bytes);
- return;
- }
+ if (UNLIKELY(bytes <= 0)) return; // < 0 needed in RELEASE, hits DCHECK in DEBUG
if (consumption_metric_ != nullptr) {
RefreshConsumptionFromMetric();
@@ -146,25 +144,22 @@ class MemTracker {
}
}
- /// Increases/Decreases the consumption of this tracker and the ancestors up to (but
+ /// Increases the consumption of this tracker and the ancestors up to (but
/// not including) end_tracker. This is useful if we want to move tracking between
/// trackers that share a common (i.e. end_tracker) ancestor. This happens when we want
/// to update tracking on a particular mem tracker but the consumption against
/// the limit recorded in one of its ancestors already happened.
void ConsumeLocal(int64_t bytes, MemTracker* end_tracker) {
- DCHECK(!closed_) << label_;
- DCHECK(consumption_metric_ == nullptr) << "Should not be called on root.";
- for (MemTracker* tracker : all_trackers_) {
- if (tracker == end_tracker) return;
- DCHECK(!tracker->has_limit());
- DCHECK(!tracker->closed_) << tracker->label_;
- tracker->consumption_->Add(bytes);
- }
- DCHECK(false) << "end_tracker is not an ancestor";
+ DCHECK_GE(bytes, 0);
+ if (UNLIKELY(bytes < 0)) return; // needed in RELEASE, hits DCHECK in DEBUG
+ ChangeConsumption(bytes, end_tracker);
}
+ /// Same as above, but it decreases the consumption.
void ReleaseLocal(int64_t bytes, MemTracker* end_tracker) {
- ConsumeLocal(-bytes, end_tracker);
+ DCHECK_GE(bytes, 0);
+ if (UNLIKELY(bytes < 0)) return; // needed in RELEASE, hits DCHECK in DEBUG
+ ChangeConsumption(-bytes, end_tracker);
}
/// Increases consumption of this tracker and its ancestors by 'bytes' only if
@@ -175,9 +170,11 @@ class MemTracker {
/// of success. Returns true if the consumption was successfully updated.
WARN_UNUSED_RESULT
bool TryConsume(int64_t bytes, MemLimit mode = MemLimit::HARD) {
+ DCHECK_GE(bytes, 0);
DCHECK(!closed_) << label_;
+ if (UNLIKELY(bytes == 0)) return true;
+ if (UNLIKELY(bytes < 0)) return false; // needed in RELEASE, hits DCHECK in DEBUG
if (consumption_metric_ != nullptr) RefreshConsumptionFromMetric();
- if (UNLIKELY(bytes <= 0)) return true;
int i;
// Walk the tracker tree top-down.
for (i = all_trackers_.size() - 1; i >= 0; --i) {
@@ -216,11 +213,9 @@ class MemTracker {
/// Decreases consumption of this tracker and its ancestors by 'bytes'.
void Release(int64_t bytes) {
+ DCHECK_GE(bytes, 0);
DCHECK(!closed_) << label_;
- if (bytes <= 0) {
- if (bytes < 0) Consume(-bytes);
- return;
- }
+ if (UNLIKELY(bytes <= 0)) return; // < 0 needed in RELEASE, hits DCHECK in DEBUG
if (consumption_metric_ != nullptr) {
RefreshConsumptionFromMetric();
@@ -408,6 +403,20 @@ class MemTracker {
/// Otherwise return NULL.
MemTracker* GetQueryMemTracker();
+ /// Increases/Decreases the consumption of this tracker and the ancestors up to (but
+ /// not including) end_tracker.
+ void ChangeConsumption(int64_t bytes, MemTracker* end_tracker) {
+ DCHECK(!closed_) << label_;
+ DCHECK(consumption_metric_ == nullptr) << "Should not be called on root.";
+ for (MemTracker* tracker : all_trackers_) {
+ if (tracker == end_tracker) return;
+ DCHECK(!tracker->has_limit());
+ DCHECK(!tracker->closed_) << tracker->label_;
+ tracker->consumption_->Add(bytes);
+ }
+ DCHECK(false) << "end_tracker is not an ancestor";
+ }
+
/// Lock to protect GcMemory(). This prevents many GCs from occurring at once.
boost::mutex gc_lock_;