common/perf_counters: enabling 'find()' by logger name#60426
common/perf_counters: enabling 'find()' by logger name#60426
Conversation
|
Failing on a C++-20 feature... That's strange. Update: the feature was added in p0408.
|
3226d90 to
d98b70a
Compare
d9cb256 to
61cc850
Compare
rzarzynski
left a comment
There was a problem hiding this comment.
Basically LGTM; just a question to ensure understanding.
src/common/perf_counters.cc
Outdated
| ceph_assert(i != m_loggers.end()); | ||
| m_loggers.erase(i); | ||
| [[maybe_unused]] const auto rm_cnt = m_loggers.erase(l); | ||
| ceph_assert(rm_cnt > 0); |
There was a problem hiding this comment.
It looks there is a change in behavior. Previously remove() was dropping only a single item from m_loggers; now it must be at least 1. Is this intentional?
UPDATE: it seems just confusing; as the name should already be unique, I don't expect to remove more than 1.
There was a problem hiding this comment.
I do not wish to change the 'erase', but I'll change the assert to make it plain.
| while (i != m_loggers.end()) { | ||
| ostringstream ss; | ||
| ss << l->get_name() << "-" << (void*)l; | ||
| l->set_name(ss.str()); |
There was a problem hiding this comment.
Huh, we were extending the name by the -{address} even indefinitely, till the uniqueness is achieved.
| l->set_name(ss.str()); | ||
| i = m_loggers.find(l); | ||
| while (m_loggers.contains(l)) { | ||
| l->set_name(fmt::format("{}-{:p}", l->get_name(), (void *)l)); |
There was a problem hiding this comment.
OK, no change; same algorithm.
| } else { | ||
| // dump only specified logger | ||
| auto l = m_loggers.find(logger); | ||
| if (l != m_loggers.end()) { |
There was a problem hiding this comment.
nit: if (auto l = ...; ...)
There was a problem hiding this comment.
Not sure it would be an improvement. The outer block ends immediately after the 'if', so the variable
scope is limited anyway. And it's easier to read this way.
Multiple style and performance tweaks. The important one: Modifying reset() to directly locate the named logger. As most of the code here predates C++14, "transparent comparators" were not used. Instead, we looped through the list of loggers. With the updated C++ we are using today, the comparator can be fixed to allow a find() based on the name. Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
Some counters' dump functions accept 3 bool parameters to determine some dumping behavior. And multiple 'bool' parameters are considered a code smell. This is a minor refactor to replace one of these bools with a 'select_labeled_t' enum. Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
61cc850 to
d2b910b
Compare
|
jenkins test api |
|
Passed formal QA. Also tested on my Teu runs, including in the jumbo wip-rf-combined-30j |
Multiple style and performance tweaks. The important one:
Modifying reset() to directly locate the named logger.
As most of the code here predates C++14, "transparent
comparators" were not used. Instead, we looped through
the list of loggers.
With the updated C++ we are using today, the comparator
can be fixed to allow a find() based on the name.
Also: avoiding the use of 3 consecutive 'bool's as arguments to
some dump functions.