Skip to content

Switch Mem::Meter to use std::chrono API #2392

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.

Sign up for GitHub

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

Open
yadij wants to merge 1 commit into
base: master
Choose a base branch
from
+14 −6
Open

Switch Mem::Meter to use std::chrono API #2392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/mem/Meter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#ifndef SQUID_SRC_MEM_METER_H
#define SQUID_SRC_MEM_METER_H

#include "time/gadgets.h"
#include <chrono>

namespace Mem
{
Expand All @@ -20,12 +20,14 @@ namespace Mem
class Meter
{
public:
using Timestamp = std::chrono::time_point<std::chrono::system_clock>;

/// flush the meter level back to 0, but leave peak records
void flush() {level=0;}

ssize_t currentLevel() const {return level;}
ssize_t peak() const {return hwater_level;}
time_t peakTime() const {return hwater_stamp;}
const Timestamp &peakTime() const {return hwater_stamp;}
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not introduce unnecessary references. The size of this data member is usually 8 bytes, same as a reference size, but without the reference complications.

Suggested change
const Timestamp &peakTime() const {return hwater_stamp;}
auto peakTime() const { return hwater_stamp; }


Meter &operator ++() {++level; checkHighWater(); return *this;}
Meter &operator --() {--level; return *this;}
Expand All @@ -39,13 +41,13 @@ class Meter
void checkHighWater() {
if (hwater_level < level) {
hwater_level = level;
hwater_stamp = squid_curtime ? squid_curtime : time(nullptr);
hwater_stamp = std::chrono::system_clock::now();
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, we should continue using Squid current time "cache" in code like this. getCurrentTime() is already using std::chrono::system_clock::now() internally, so we need to remember/expose that value via a function. I did not check whether there are open PRs doing that already.

}
}

ssize_t level = 0; ///< current level (count or volume)
ssize_t hwater_level = 0; ///< high water mark
time_t hwater_stamp = 0; ///< timestamp of last high water mark change
Timestamp hwater_stamp; ///< timestamp of last high water mark change
};

/**
Expand Down
10 changes: 8 additions & 2 deletions src/mem/old_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,12 @@ Mem::PoolReport(const PoolStats *mp_st, const PoolMeter *AllMeter, std::ostream
stream << delim;
stream << delim;
}

auto AsHours = [](const auto &base) -> double {
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use lowercase for local variables, avoid unwanted references (see another change request), and use const/AAA as much as possible.

And since this is not an I/O manipulator but a casting function, let's call it "[convert] toX()" rather than "[print] asX()".

Here is a sketch:

Suggested change
auto AsHours = [](const auto &base) -> double {
const auto toHours = [](const auto base) {

const auto span = std::chrono::duration_cast<std::chrono::seconds>(std::chrono::system_clock::now() - base);
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope it is possible to avoid duration_cast in this math. If it is possible, we should avoid it. I do not have a ready-to-use recipe at this time.

Copy link
Copy Markdown
Contributor Author

@yadij yadij Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The system_clock operates in nanosecond ticks and our math is calculating in seconds. AFAIK, the duration cast is the "proper" (ie most efficient) way to change time scales.

return span.count() / 3600.0;
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of manual math is essentially a reinterpret_cast<> equivalent. It is error-prone. I hope it is possible to avoid this unchecked (by compiler) time "casting" when using std::chrono in new code.

};

/*
* Fragmentation calculation:
* needed = inuse.currentLevel() / chunk_capacity
Expand All @@ -524,13 +530,13 @@ Mem::PoolReport(const PoolStats *mp_st, const PoolMeter *AllMeter, std::ostream
stream << mp_st->items_alloc << delim;
stream << toKB(mp_st->obj_size * pm->alloc.currentLevel()) << delim;
stream << toKB(mp_st->obj_size * pm->alloc.peak()) << delim;
stream << std::setprecision(2) << ((squid_curtime - pm->alloc.peakTime()) / 3600.) << delim;
stream << std::setprecision(2) << AsHours(pm->alloc.peakTime()) << delim;
stream << std::setprecision(3) << xpercent(mp_st->obj_size * pm->alloc.currentLevel(), AllMeter->alloc.currentLevel()) << delim;
/* in use */
stream << mp_st->items_inuse << delim;
stream << toKB(mp_st->obj_size * pm->inuse.currentLevel()) << delim;
stream << toKB(mp_st->obj_size * pm->inuse.peak()) << delim;
stream << std::setprecision(2) << ((squid_curtime - pm->inuse.peakTime()) / 3600.) << delim;
stream << std::setprecision(2) << AsHours(pm->inuse.peakTime()) << delim;
stream << std::setprecision(3) << xpercent(pm->inuse.currentLevel(), pm->alloc.currentLevel()) << delim;
/* idle */
stream << mp_st->items_idle << delim;
Expand Down
Loading