MemoryTracker new no throw #24483
MemoryTracker new no throw #24483
Conversation
robot-clickhouse
added
the
pr-not-for-changelog
src/Common/MemoryTracker.cpp
Outdated
| description ? " " : "", description ? description : "", | ||
| formatReadableSizeWithBinarySuffix(will_be), | ||
| size, formatReadableSizeWithBinarySuffix(current_hard_limit)); | ||
| } | ||
| } | ||
|
|
||
| updatePeak(will_be); |
There was a problem hiding this comment.
There is logging in updatePeak that should be disabled in "light" version of MemoryTracker.
src/Common/MemoryTracker.cpp
Outdated
| if (unlikely(current_profiler_limit && will_be > current_profiler_limit)) | ||
| { | ||
| BlockerInThread untrack_lock(VariableContext::Global); | ||
| DB::TraceCollector::collect(DB::TraceType::Memory, StackTrace(), size); |
There was a problem hiding this comment.
But profiling should remain enabled in "light" version of MemoryTracker.
There was a problem hiding this comment.
@alexey-milovidov DB::TraceCollector::collect can throw exception.
There was a problem hiding this comment.
Let's make it non throwing.
There was a problem hiding this comment.
Checked DB::TraceCollector::collect does not throw exception, it use special buffer subclass that does not throw.
kitaisreal
force-pushed
the
memory-tracker-new-no-throw
branch
from
479e035
to
60c20ed
Compare
|
Ok. Cannot merge due to Also there is one potential downside - the messages "Current memory usage" that are printed every 1 GiB can be skipped. |
| @@ -173,29 +173,36 @@ void MemoryTracker::alloc(Int64 size) | |||
| } | |||
|
|
|||
| #ifdef MEMORY_TRACKER_DEBUG_CHECKS | |||
| if (unlikely(_memory_tracker_always_throw_logical_error_on_allocation)) | |||
| if (unlikely(_memory_tracker_always_throw_logical_error_on_allocation) && throw_if_memory_exceeded) | |||
There was a problem hiding this comment.
Looks like this should check for throw_if_memory_exceeded? since this is logical error, so an error in the code.
There was a problem hiding this comment.
It is issue, will create pull request.
| description ? " " : "", description ? description : "", | ||
| formatReadableSizeWithBinarySuffix(will_be), | ||
| size, formatReadableSizeWithBinarySuffix(current_hard_limit)); | ||
| throw DB::Exception( |
There was a problem hiding this comment.
But should it prohibit fault injection?
There was a problem hiding this comment.
It was discussed and we decided to prevent fault injection for lite version of MemoryTracker.
| void MemoryTracker::allocNoThrow(Int64 size) | ||
| { | ||
| bool throw_if_memory_exceeded = false; | ||
| allocImpl(size, throw_if_memory_exceeded); |
There was a problem hiding this comment.
BTW why not simply allocImpl(size, /* throw_if_memory_exceeded= */ false);
Looks a little bit excessive.
clang-tidy will detect if the comment will not match with the variable name from the function signature
Or is it the "new coding style"? (in other places I saw this via comment not via separate variable)
There was a problem hiding this comment.
Both variants are ok.
There was a problem hiding this comment.
Both variants are ok.
What we're saying is that introducing a separate non-const variable looks exceedingly verbose (extra line) and confusing (maybe it's used somewhere else later?).
There was a problem hiding this comment.
@akuzm what I am saying that it is just style preference, both variants are equally bad, additional comment of bool property name is unreadable in case you have more than 1 bool. For function with 3 lines current variant is ok.
After ClickHouse#24483 had been merged the only place where the allocation may fail is the insert into PODArray in DB::OwnSplitChannel::log, but this MR ignores the errors in this function, so to check new behaviour separate server is required.
It looks useless nowadays, because operator new cannot throw MEMORY_LIMIT_EXCEEDED today, and so any code that works on Exception is likely safe. Refs: ClickHouse#40249 Refs: ClickHouse#24483 Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
This closes #24110