Skip to content

Commit

Permalink
fix: recursive calls in the allocation tracker (#3665)
Browse files Browse the repository at this point in the history
Also, remove dependence of absl::TimeZone bloated monstrosity, which was required by
absl::FormatTime api, even though we do not actually format a timezone.

When absl::LocalTimeZone is accessed it allocates hundreds of thousands of bytes
for each shard thread (maybe due to lack thread safety during lazy initialization).

At the end, strftime does a great job without any shenanigans.

Signed-off-by: Roman Gershman <[email protected]>
  • Loading branch information
romange committed Sep 8, 2024
1 parent 67117ff commit 64faf87
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 10 deletions.
13 changes: 9 additions & 4 deletions src/core/allocation_tracker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,12 @@ void AllocationTracker::ProcessNew(void* ptr, size_t size) {
return;
}

thread_local bool inside_process_new = false;
if (inside_process_new) {
if (inside_tracker_) {
return;
}

// Prevent endless recursion, in case logging allocates memory
inside_process_new = true;
inside_tracker_ = true;
double random = absl::Uniform(g_bitgen, 0.0, 1.0);
for (const auto& band : tracking_) {
if (random >= band.sample_odds || size > band.upper_bound || size < band.lower_bound) {
Expand All @@ -72,10 +71,15 @@ void AllocationTracker::ProcessNew(void* ptr, size_t size) {
<< "). Stack: " << util::fb2::GetStacktrace();
break;
}
inside_process_new = false;
inside_tracker_ = false;
}

void AllocationTracker::ProcessDelete(void* ptr) {
if (inside_tracker_) {
return;
}

inside_tracker_ = true;
// we partially handle deletes, specifically when specifying a single range with
// 100% sampling rate.
if (tracking_.size() == 1 && tracking_.front().sample_odds == 1) {
Expand All @@ -84,6 +88,7 @@ void AllocationTracker::ProcessDelete(void* ptr) {
LOG(INFO) << "Deallocating " << usable << " bytes (" << ptr << ")";
}
}
inside_tracker_ = false;
}

} // namespace dfly
1 change: 1 addition & 0 deletions src/core/allocation_tracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class AllocationTracker {

private:
absl::InlinedVector<TrackingInfo, 4> tracking_;
bool inside_tracker_ = false;
};

} // namespace dfly
Expand Down
19 changes: 14 additions & 5 deletions src/server/detail/save_stages_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ void RdbSnapshot::StartInShard(EngineShard* shard) {

SaveStagesController::SaveStagesController(SaveStagesInputs&& inputs)
: SaveStagesInputs{std::move(inputs)} {
start_time_ = absl::Now();
start_time_ = time(NULL);
}

SaveStagesController::~SaveStagesController() {
Expand Down Expand Up @@ -302,12 +302,12 @@ void SaveStagesController::SaveRdb() {
}

uint32_t SaveStagesController::GetCurrentSaveDuration() {
return (absl::Now() - start_time_) / absl::Seconds(1);
return time(nullptr) - start_time_;
}

SaveInfo SaveStagesController::GetSaveInfo() {
SaveInfo info;
info.save_time = absl::ToUnixSeconds(start_time_);
info.save_time = start_time_;
info.duration_sec = GetCurrentSaveDuration();

if (shared_err_) {
Expand Down Expand Up @@ -378,8 +378,17 @@ GenericError SaveStagesController::BuildFullPath() {

SubstituteFilenamePlaceholders(
&filename, {.ts = "%Y-%m-%dT%H:%M:%S", .year = "%Y", .month = "%m", .day = "%d"});
filename = absl::FormatTime(filename.string(), start_time_, absl::LocalTimeZone());
full_path_ = dir_path / filename;

tm time_tm;
localtime_r(&start_time_, &time_tm);
string src_format = filename.string();
string dest_buf(src_format.size() + 128, '\0');
size_t len = strftime(dest_buf.data(), dest_buf.size(), src_format.c_str(), &time_tm);
if (len == 0)
return {"invalid dbfilename format"};
dest_buf.resize(len);

full_path_ = dir_path / dest_buf;
is_cloud_ = IsCloudPath(full_path_.string());
return {};
}
Expand Down
2 changes: 1 addition & 1 deletion src/server/detail/save_stages_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ struct SaveStagesController : public SaveStagesInputs {
void RunStage(void (SaveStagesController::*cb)(unsigned));

private:
absl::Time start_time_;
time_t start_time_;
std::filesystem::path full_path_;
bool is_cloud_;

Expand Down

0 comments on commit 64faf87

Please sign in to comment.