| Message ID | 20220826113931.59323-3-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, Thank you for the patch. On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote: > Each declaration of a LogCategory will create a new LogCategory, and > will be stored in an unordered_set Logger::categories_. This means that > when a plugin .so is unloaded and loaded, as happens when destructing > and creating a CamereManager, we'll get duplicate categories. > > The Logger::registerCategory docs say "Log categories must have unique > names. If a category with the same name already exists this function > performs no operation.". The code does not comply with this. > > We solve the issue with two changes: > > Change the unordered_set to a vector for simplicity, as there's no need > for an unordered_set. > > Instead of using the LogCategory constructor to create new categories in > _LOG_CATEGORY() macro, use a factory method. The factory method will > return either an existing LogCategory if one exists with the given name, > or a newly created one. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > include/libcamera/base/log.h | 6 +++-- > src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++----- > 2 files changed, 48 insertions(+), 9 deletions(-) > > diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h > index 8b462767..76e01118 100644 > --- a/include/libcamera/base/log.h > +++ b/include/libcamera/base/log.h > @@ -29,7 +29,7 @@ enum LogSeverity { > class LogCategory > { > public: > - explicit LogCategory(const char *name); > + static LogCategory *create(const std::string &name); Let's pass a const char * to this function, to have a single construction of a std::string inside the function instead of in every caller. > > const std::string &name() const { return name_; } > LogSeverity severity() const { return severity_; } > @@ -38,6 +38,8 @@ public: > static const LogCategory &defaultCategory(); > > private: > + explicit LogCategory(const std::string &name); Is the change from const char * to std::string needed here ? If so, does it belong to 1/2 ? > + > const std::string name_; > LogSeverity severity_; > }; > @@ -49,7 +51,7 @@ extern const LogCategory &_LOG_CATEGORY(name)(); > const LogCategory &_LOG_CATEGORY(name)() \ > { \ > /* The instance will be deleted by the Logger destructor. */ \ > - static LogCategory *category = new LogCategory(#name); \ > + static LogCategory *category = LogCategory::create(#name); \ > return *category; \ > } > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp > index a4a5b452..c6500e96 100644 > --- a/src/libcamera/base/log.cpp > +++ b/src/libcamera/base/log.cpp > @@ -314,10 +314,11 @@ private: > > friend LogCategory; > void registerCategory(LogCategory *category); > + LogCategory *findCategory(const std::string &name) const; > > static bool destroyed_; > > - std::unordered_set<LogCategory *> categories_; > + std::vector<LogCategory *> categories_; > std::list<std::pair<std::string, LogSeverity>> levels_; > > std::shared_ptr<LogOutput> output_; > @@ -707,12 +708,14 @@ LogSeverity Logger::parseLogLevel(const std::string &level) > * \brief Register a log category with the logger > * \param[in] category The log category > * > - * Log categories must have unique names. If a category with the same name > - * already exists this function performs no operation. > + * Log categories must have unique names. It is illegal to call this function Nobody will arrest you for doing so, so you can write invalid instead of illegal :-) > + * if a log category with the same name already exists. > */ > void Logger::registerCategory(LogCategory *category) > { > - categories_.insert(category); > + ASSERT(!findCategory(category->name())); I wonder if this is overkill, as the caller checks that the category isn't found first. I suppose the performance penalty of the lookup is acceptable in debug mode, but maybe it's not needed ? > + > + categories_.push_back(category); This reminds me we need to add locking to the logger. That's out of scope for this patch of course. > > const std::string &name = category->name(); > for (const std::pair<std::string, LogSeverity> &level : levels_) { > @@ -736,6 +739,21 @@ void Logger::registerCategory(LogCategory *category) > } > } > > +/** > + * \brief Find an existing log category with the given name > + * \param[in] name The name of the log category * \return ... > + */ > +LogCategory *Logger::findCategory(const std::string &name) const > +{ > + if (auto it = std::find_if(categories_.begin(), categories_.end(), > + [&name](auto c) { return name == c->name(); }); Would it be more efficient to store categories in an unordered_map indexed by the category name ? > + it != categories_.end()) { > + return *it; > + } > + > + return nullptr; > +} > + > /** > * \enum LogSeverity > * Log message severity > @@ -760,14 +778,33 @@ void Logger::registerCategory(LogCategory *category) > * and is used to control the log level per group. > */ > > +/** > + * \brief Create a new LogCategory or return an existing one * \param[in] name ... > + * \return The pointer to the LogCategory The return documentation goes to the end of the comment. > + * > + * Create and return a new LogCategory with the given name if such a category > + * does not yet exist, or return the existing one. > + */ > +LogCategory *LogCategory::create(const std::string &name) > +{ > + LogCategory *category = Logger::instance()->findCategory(name); > + > + if (!category) { > + category = new LogCategory(name); > + You could drop the blank line. > + Logger::instance()->registerCategory(category); > + } > + > + return category; > +} > + > /** > * \brief Construct a log category > * \param[in] name The category name > */ > -LogCategory::LogCategory(const char *name) > +LogCategory::LogCategory(const std::string &name) > : name_(name), severity_(LogSeverity::LogInfo) > { > - Logger::instance()->registerCategory(this); > } > > /** > @@ -804,7 +841,7 @@ void LogCategory::setSeverity(LogSeverity severity) > */ > const LogCategory &LogCategory::defaultCategory() > { > - static const LogCategory *category = new LogCategory("default"); > + static const LogCategory *category = LogCategory::create("default"); > return *category; > } >
On 27/08/2022 04:27, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote: >> Each declaration of a LogCategory will create a new LogCategory, and >> will be stored in an unordered_set Logger::categories_. This means that >> when a plugin .so is unloaded and loaded, as happens when destructing >> and creating a CamereManager, we'll get duplicate categories. >> >> The Logger::registerCategory docs say "Log categories must have unique >> names. If a category with the same name already exists this function >> performs no operation.". The code does not comply with this. >> >> We solve the issue with two changes: >> >> Change the unordered_set to a vector for simplicity, as there's no need >> for an unordered_set. >> >> Instead of using the LogCategory constructor to create new categories in >> _LOG_CATEGORY() macro, use a factory method. The factory method will >> return either an existing LogCategory if one exists with the given name, >> or a newly created one. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> include/libcamera/base/log.h | 6 +++-- >> src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++----- >> 2 files changed, 48 insertions(+), 9 deletions(-) >> >> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h >> index 8b462767..76e01118 100644 >> --- a/include/libcamera/base/log.h >> +++ b/include/libcamera/base/log.h >> @@ -29,7 +29,7 @@ enum LogSeverity { >> class LogCategory >> { >> public: >> - explicit LogCategory(const char *name); >> + static LogCategory *create(const std::string &name); > > Let's pass a const char * to this function, to have a single > construction of a std::string inside the function instead of in every > caller. Well, I think we should never use char * in C++ code unless absolutely necessary as we have a proper class for string, which makes it much harder to write buggy code wrt. string operations. Here, I think that's a bit of a pointless optimization considering the amount of times these functions are called. Nevertheless, a better option compared to std::string (and, I think, char *) would be std::string_view. Is that fine for you? >> >> const std::string &name() const { return name_; } >> LogSeverity severity() const { return severity_; } >> @@ -38,6 +38,8 @@ public: >> static const LogCategory &defaultCategory(); >> >> private: >> + explicit LogCategory(const std::string &name); > > Is the change from const char * to std::string needed here ? If so, does > it belong to 1/2 ? I don't think the change from char * to string is needed in either patch. It just made sense to me to get rid of the char * use. It could be dropped or done before either patch. >> + >> const std::string name_; >> LogSeverity severity_; >> }; >> @@ -49,7 +51,7 @@ extern const LogCategory &_LOG_CATEGORY(name)(); >> const LogCategory &_LOG_CATEGORY(name)() \ >> { \ >> /* The instance will be deleted by the Logger destructor. */ \ >> - static LogCategory *category = new LogCategory(#name); \ >> + static LogCategory *category = LogCategory::create(#name); \ >> return *category; \ >> } >> >> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp >> index a4a5b452..c6500e96 100644 >> --- a/src/libcamera/base/log.cpp >> +++ b/src/libcamera/base/log.cpp >> @@ -314,10 +314,11 @@ private: >> >> friend LogCategory; >> void registerCategory(LogCategory *category); >> + LogCategory *findCategory(const std::string &name) const; >> >> static bool destroyed_; >> >> - std::unordered_set<LogCategory *> categories_; >> + std::vector<LogCategory *> categories_; >> std::list<std::pair<std::string, LogSeverity>> levels_; >> >> std::shared_ptr<LogOutput> output_; >> @@ -707,12 +708,14 @@ LogSeverity Logger::parseLogLevel(const std::string &level) >> * \brief Register a log category with the logger >> * \param[in] category The log category >> * >> - * Log categories must have unique names. If a category with the same name >> - * already exists this function performs no operation. >> + * Log categories must have unique names. It is illegal to call this function > > Nobody will arrest you for doing so, so you can write invalid instead of > illegal :-) Are you sure? ... Well, ok... >> + * if a log category with the same name already exists. >> */ >> void Logger::registerCategory(LogCategory *category) >> { >> - categories_.insert(category); >> + ASSERT(!findCategory(category->name())); > > I wonder if this is overkill, as the caller checks that the category > isn't found first. I suppose the performance penalty of the lookup is > acceptable in debug mode, but maybe it's not needed ? I can drop this. The function is called only inside this file, so I think we can expect the parameter to be correct. >> + >> + categories_.push_back(category); > > This reminds me we need to add locking to the logger. That's out of > scope for this patch of course. > >> >> const std::string &name = category->name(); >> for (const std::pair<std::string, LogSeverity> &level : levels_) { >> @@ -736,6 +739,21 @@ void Logger::registerCategory(LogCategory *category) >> } >> } >> >> +/** >> + * \brief Find an existing log category with the given name >> + * \param[in] name The name of the log category > > * \return ... Ok. >> + */ >> +LogCategory *Logger::findCategory(const std::string &name) const >> +{ >> + if (auto it = std::find_if(categories_.begin(), categories_.end(), >> + [&name](auto c) { return name == c->name(); }); > > Would it be more efficient to store categories in an unordered_map > indexed by the category name ? Depends what you mean with efficient... =) I think we'll usually have some tens of entries. My pure guess is that the memory use with unordered_map would be much larger than with a vector, and we would be constructing string instances for the keys. The lookup difference would probably be unnoticeable with these amount of entries. The for loops we have that iterate over the categories_ would probably be slower with an unordered_map. And considering we'll call this function a few tens of times, I again think trying to optimize this is an unnecessary optimization. I think a more valid question is if an unordered_map or map would suit better code-wise (readability, maintainability) for this case. With the current uses of 'categories_', I don't see a big difference either way. Using a map would simplify this lookup, but iterating with for would be a bit more complex. >> + it != categories_.end()) { >> + return *it; >> + } >> + >> + return nullptr; >> +} >> + >> /** >> * \enum LogSeverity >> * Log message severity >> @@ -760,14 +778,33 @@ void Logger::registerCategory(LogCategory *category) >> * and is used to control the log level per group. >> */ >> >> +/** >> + * \brief Create a new LogCategory or return an existing one > > * \param[in] name ... Ok. >> + * \return The pointer to the LogCategory > > The return documentation goes to the end of the comment. Ok. >> + * >> + * Create and return a new LogCategory with the given name if such a category >> + * does not yet exist, or return the existing one. >> + */ >> +LogCategory *LogCategory::create(const std::string &name) >> +{ >> + LogCategory *category = Logger::instance()->findCategory(name); >> + >> + if (!category) { >> + category = new LogCategory(name); >> + > > You could drop the blank line. Ok. Tomi
Hi Tomi, On Mon, Aug 29, 2022 at 11:18:29AM +0300, Tomi Valkeinen wrote: > On 27/08/2022 04:27, Laurent Pinchart wrote: > > On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote: > >> Each declaration of a LogCategory will create a new LogCategory, and > >> will be stored in an unordered_set Logger::categories_. This means that > >> when a plugin .so is unloaded and loaded, as happens when destructing > >> and creating a CamereManager, we'll get duplicate categories. > >> > >> The Logger::registerCategory docs say "Log categories must have unique > >> names. If a category with the same name already exists this function > >> performs no operation.". The code does not comply with this. > >> > >> We solve the issue with two changes: > >> > >> Change the unordered_set to a vector for simplicity, as there's no need > >> for an unordered_set. > >> > >> Instead of using the LogCategory constructor to create new categories in > >> _LOG_CATEGORY() macro, use a factory method. The factory method will > >> return either an existing LogCategory if one exists with the given name, > >> or a newly created one. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >> --- > >> include/libcamera/base/log.h | 6 +++-- > >> src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++----- > >> 2 files changed, 48 insertions(+), 9 deletions(-) > >> > >> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h > >> index 8b462767..76e01118 100644 > >> --- a/include/libcamera/base/log.h > >> +++ b/include/libcamera/base/log.h > >> @@ -29,7 +29,7 @@ enum LogSeverity { > >> class LogCategory > >> { > >> public: > >> - explicit LogCategory(const char *name); > >> + static LogCategory *create(const std::string &name); > > > > Let's pass a const char * to this function, to have a single > > construction of a std::string inside the function instead of in every > > caller. > > Well, I think we should never use char * in C++ code unless absolutely > necessary as we have a proper class for string, which makes it much > harder to write buggy code wrt. string operations. Here, I think that's > a bit of a pointless optimization considering the amount of times these > functions are called. > > Nevertheless, a better option compared to std::string (and, I think, > char *) would be std::string_view. Is that fine for you? It would, but I see you've kept const char * in v2 after falling into the rabbit hole of comparing performances :-) We don't use std::string_view in libcamera, but maybe we should consider it, I like the idea of bundling the string pointer and size together, much like we do in the Span class. > >> > >> const std::string &name() const { return name_; } > >> LogSeverity severity() const { return severity_; } > >> @@ -38,6 +38,8 @@ public: > >> static const LogCategory &defaultCategory(); > >> > >> private: > >> + explicit LogCategory(const std::string &name); > > > > Is the change from const char * to std::string needed here ? If so, does > > it belong to 1/2 ? > > I don't think the change from char * to string is needed in either > patch. It just made sense to me to get rid of the char * use. It could > be dropped or done before either patch. > > >> + > >> const std::string name_; > >> LogSeverity severity_; > >> }; > >> @@ -49,7 +51,7 @@ extern const LogCategory &_LOG_CATEGORY(name)(); > >> const LogCategory &_LOG_CATEGORY(name)() \ > >> { \ > >> /* The instance will be deleted by the Logger destructor. */ \ > >> - static LogCategory *category = new LogCategory(#name); \ > >> + static LogCategory *category = LogCategory::create(#name); \ > >> return *category; \ > >> } > >> > >> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp > >> index a4a5b452..c6500e96 100644 > >> --- a/src/libcamera/base/log.cpp > >> +++ b/src/libcamera/base/log.cpp > >> @@ -314,10 +314,11 @@ private: > >> > >> friend LogCategory; > >> void registerCategory(LogCategory *category); > >> + LogCategory *findCategory(const std::string &name) const; > >> > >> static bool destroyed_; > >> > >> - std::unordered_set<LogCategory *> categories_; > >> + std::vector<LogCategory *> categories_; > >> std::list<std::pair<std::string, LogSeverity>> levels_; > >> > >> std::shared_ptr<LogOutput> output_; > >> @@ -707,12 +708,14 @@ LogSeverity Logger::parseLogLevel(const std::string &level) > >> * \brief Register a log category with the logger > >> * \param[in] category The log category > >> * > >> - * Log categories must have unique names. If a category with the same name > >> - * already exists this function performs no operation. > >> + * Log categories must have unique names. It is illegal to call this function > > > > Nobody will arrest you for doing so, so you can write invalid instead of > > illegal :-) > > Are you sure? ... Well, ok... > > >> + * if a log category with the same name already exists. > >> */ > >> void Logger::registerCategory(LogCategory *category) > >> { > >> - categories_.insert(category); > >> + ASSERT(!findCategory(category->name())); > > > > I wonder if this is overkill, as the caller checks that the category > > isn't found first. I suppose the performance penalty of the lookup is > > acceptable in debug mode, but maybe it's not needed ? > > I can drop this. The function is called only inside this file, so I > think we can expect the parameter to be correct. > > >> + > >> + categories_.push_back(category); > > > > This reminds me we need to add locking to the logger. That's out of > > scope for this patch of course. > > > >> > >> const std::string &name = category->name(); > >> for (const std::pair<std::string, LogSeverity> &level : levels_) { > >> @@ -736,6 +739,21 @@ void Logger::registerCategory(LogCategory *category) > >> } > >> } > >> > >> +/** > >> + * \brief Find an existing log category with the given name > >> + * \param[in] name The name of the log category > > > > * \return ... > > Ok. > > >> + */ > >> +LogCategory *Logger::findCategory(const std::string &name) const > >> +{ > >> + if (auto it = std::find_if(categories_.begin(), categories_.end(), > >> + [&name](auto c) { return name == c->name(); }); > > > > Would it be more efficient to store categories in an unordered_map > > indexed by the category name ? > > Depends what you mean with efficient... =) > > I think we'll usually have some tens of entries. My pure guess is that > the memory use with unordered_map would be much larger than with a > vector, and we would be constructing string instances for the keys. The > lookup difference would probably be unnoticeable with these amount of > entries. The for loops we have that iterate over the categories_ would > probably be slower with an unordered_map. > > And considering we'll call this function a few tens of times, I again > think trying to optimize this is an unnecessary optimization. > > I think a more valid question is if an unordered_map or map would suit > better code-wise (readability, maintainability) for this case. With the > current uses of 'categories_', I don't see a big difference either way. > Using a map would simplify this lookup, but iterating with for would be > a bit more complex. OK. > >> + it != categories_.end()) { > >> + return *it; > >> + } > >> + > >> + return nullptr; > >> +} > >> + > >> /** > >> * \enum LogSeverity > >> * Log message severity > >> @@ -760,14 +778,33 @@ void Logger::registerCategory(LogCategory *category) > >> * and is used to control the log level per group. > >> */ > >> > >> +/** > >> + * \brief Create a new LogCategory or return an existing one > > > > * \param[in] name ... > > Ok. > > >> + * \return The pointer to the LogCategory > > > > The return documentation goes to the end of the comment. > > Ok. > > >> + * > >> + * Create and return a new LogCategory with the given name if such a category > >> + * does not yet exist, or return the existing one. > >> + */ > >> +LogCategory *LogCategory::create(const std::string &name) > >> +{ > >> + LogCategory *category = Logger::instance()->findCategory(name); > >> + > >> + if (!category) { > >> + category = new LogCategory(name); > >> + > > > > You could drop the blank line. > > Ok.
On 29/08/2022 17:16, Laurent Pinchart wrote: > Hi Tomi, > > On Mon, Aug 29, 2022 at 11:18:29AM +0300, Tomi Valkeinen wrote: >> On 27/08/2022 04:27, Laurent Pinchart wrote: >>> On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote: >>>> Each declaration of a LogCategory will create a new LogCategory, and >>>> will be stored in an unordered_set Logger::categories_. This means that >>>> when a plugin .so is unloaded and loaded, as happens when destructing >>>> and creating a CamereManager, we'll get duplicate categories. >>>> >>>> The Logger::registerCategory docs say "Log categories must have unique >>>> names. If a category with the same name already exists this function >>>> performs no operation.". The code does not comply with this. >>>> >>>> We solve the issue with two changes: >>>> >>>> Change the unordered_set to a vector for simplicity, as there's no need >>>> for an unordered_set. >>>> >>>> Instead of using the LogCategory constructor to create new categories in >>>> _LOG_CATEGORY() macro, use a factory method. The factory method will >>>> return either an existing LogCategory if one exists with the given name, >>>> or a newly created one. >>>> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>> --- >>>> include/libcamera/base/log.h | 6 +++-- >>>> src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++----- >>>> 2 files changed, 48 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h >>>> index 8b462767..76e01118 100644 >>>> --- a/include/libcamera/base/log.h >>>> +++ b/include/libcamera/base/log.h >>>> @@ -29,7 +29,7 @@ enum LogSeverity { >>>> class LogCategory >>>> { >>>> public: >>>> - explicit LogCategory(const char *name); >>>> + static LogCategory *create(const std::string &name); >>> >>> Let's pass a const char * to this function, to have a single >>> construction of a std::string inside the function instead of in every >>> caller. >> >> Well, I think we should never use char * in C++ code unless absolutely >> necessary as we have a proper class for string, which makes it much >> harder to write buggy code wrt. string operations. Here, I think that's >> a bit of a pointless optimization considering the amount of times these >> functions are called. >> >> Nevertheless, a better option compared to std::string (and, I think, >> char *) would be std::string_view. Is that fine for you? > > It would, but I see you've kept const char * in v2 after falling into > the rabbit hole of comparing performances :-) We don't use Yes, and with string's small-strings-optimization it gets "interesting"... > std::string_view in libcamera, but maybe we should consider it, I like > the idea of bundling the string pointer and size together, much like we > do in the Span class. Yes, I haven't used string_view very much yet, but I think I like it a lot. Especially cases where you want to, e.g., pass a substring to a function, and instead of creating a new string you just pass a string_view which points to the original one. Or, say, splitting a string into tokens, with string_views no new strings are created. Tomi
Hi Tomi, On Mon, Aug 29, 2022 at 05:27:57PM +0300, Tomi Valkeinen wrote: > On 29/08/2022 17:16, Laurent Pinchart wrote: > > On Mon, Aug 29, 2022 at 11:18:29AM +0300, Tomi Valkeinen wrote: > >> On 27/08/2022 04:27, Laurent Pinchart wrote: > >>> On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote: > >>>> Each declaration of a LogCategory will create a new LogCategory, and > >>>> will be stored in an unordered_set Logger::categories_. This means that > >>>> when a plugin .so is unloaded and loaded, as happens when destructing > >>>> and creating a CamereManager, we'll get duplicate categories. > >>>> > >>>> The Logger::registerCategory docs say "Log categories must have unique > >>>> names. If a category with the same name already exists this function > >>>> performs no operation.". The code does not comply with this. > >>>> > >>>> We solve the issue with two changes: > >>>> > >>>> Change the unordered_set to a vector for simplicity, as there's no need > >>>> for an unordered_set. > >>>> > >>>> Instead of using the LogCategory constructor to create new categories in > >>>> _LOG_CATEGORY() macro, use a factory method. The factory method will > >>>> return either an existing LogCategory if one exists with the given name, > >>>> or a newly created one. > >>>> > >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >>>> --- > >>>> include/libcamera/base/log.h | 6 +++-- > >>>> src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++----- > >>>> 2 files changed, 48 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h > >>>> index 8b462767..76e01118 100644 > >>>> --- a/include/libcamera/base/log.h > >>>> +++ b/include/libcamera/base/log.h > >>>> @@ -29,7 +29,7 @@ enum LogSeverity { > >>>> class LogCategory > >>>> { > >>>> public: > >>>> - explicit LogCategory(const char *name); > >>>> + static LogCategory *create(const std::string &name); > >>> > >>> Let's pass a const char * to this function, to have a single > >>> construction of a std::string inside the function instead of in every > >>> caller. > >> > >> Well, I think we should never use char * in C++ code unless absolutely > >> necessary as we have a proper class for string, which makes it much > >> harder to write buggy code wrt. string operations. Here, I think that's > >> a bit of a pointless optimization considering the amount of times these > >> functions are called. > >> > >> Nevertheless, a better option compared to std::string (and, I think, > >> char *) would be std::string_view. Is that fine for you? > > > > It would, but I see you've kept const char * in v2 after falling into > > the rabbit hole of comparing performances :-) We don't use > > Yes, and with string's small-strings-optimization it gets "interesting"... > > > std::string_view in libcamera, but maybe we should consider it, I like > > the idea of bundling the string pointer and size together, much like we > > do in the Span class. > > Yes, I haven't used string_view very much yet, but I think I like it a > lot. Especially cases where you want to, e.g., pass a substring to a > function, and instead of creating a new string you just pass a > string_view which points to the original one. Or, say, splitting a > string into tokens, with string_views no new strings are created. Indeed. StringSplitter::iterator (in include/libcamera/base/utils.h) would be a good candidate for our first use of std::string_view.
diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h index 8b462767..76e01118 100644 --- a/include/libcamera/base/log.h +++ b/include/libcamera/base/log.h @@ -29,7 +29,7 @@ enum LogSeverity { class LogCategory { public: - explicit LogCategory(const char *name); + static LogCategory *create(const std::string &name); const std::string &name() const { return name_; } LogSeverity severity() const { return severity_; } @@ -38,6 +38,8 @@ public: static const LogCategory &defaultCategory(); private: + explicit LogCategory(const std::string &name); + const std::string name_; LogSeverity severity_; }; @@ -49,7 +51,7 @@ extern const LogCategory &_LOG_CATEGORY(name)(); const LogCategory &_LOG_CATEGORY(name)() \ { \ /* The instance will be deleted by the Logger destructor. */ \ - static LogCategory *category = new LogCategory(#name); \ + static LogCategory *category = LogCategory::create(#name); \ return *category; \ } diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp index a4a5b452..c6500e96 100644 --- a/src/libcamera/base/log.cpp +++ b/src/libcamera/base/log.cpp @@ -314,10 +314,11 @@ private: friend LogCategory; void registerCategory(LogCategory *category); + LogCategory *findCategory(const std::string &name) const; static bool destroyed_; - std::unordered_set<LogCategory *> categories_; + std::vector<LogCategory *> categories_; std::list<std::pair<std::string, LogSeverity>> levels_; std::shared_ptr<LogOutput> output_; @@ -707,12 +708,14 @@ LogSeverity Logger::parseLogLevel(const std::string &level) * \brief Register a log category with the logger * \param[in] category The log category * - * Log categories must have unique names. If a category with the same name - * already exists this function performs no operation. + * Log categories must have unique names. It is illegal to call this function + * if a log category with the same name already exists. */ void Logger::registerCategory(LogCategory *category) { - categories_.insert(category); + ASSERT(!findCategory(category->name())); + + categories_.push_back(category); const std::string &name = category->name(); for (const std::pair<std::string, LogSeverity> &level : levels_) { @@ -736,6 +739,21 @@ void Logger::registerCategory(LogCategory *category) } } +/** + * \brief Find an existing log category with the given name + * \param[in] name The name of the log category + */ +LogCategory *Logger::findCategory(const std::string &name) const +{ + if (auto it = std::find_if(categories_.begin(), categories_.end(), + [&name](auto c) { return name == c->name(); }); + it != categories_.end()) { + return *it; + } + + return nullptr; +} + /** * \enum LogSeverity * Log message severity @@ -760,14 +778,33 @@ void Logger::registerCategory(LogCategory *category) * and is used to control the log level per group. */ +/** + * \brief Create a new LogCategory or return an existing one + * \return The pointer to the LogCategory + * + * Create and return a new LogCategory with the given name if such a category + * does not yet exist, or return the existing one. + */ +LogCategory *LogCategory::create(const std::string &name) +{ + LogCategory *category = Logger::instance()->findCategory(name); + + if (!category) { + category = new LogCategory(name); + + Logger::instance()->registerCategory(category); + } + + return category; +} + /** * \brief Construct a log category * \param[in] name The category name */ -LogCategory::LogCategory(const char *name) +LogCategory::LogCategory(const std::string &name) : name_(name), severity_(LogSeverity::LogInfo) { - Logger::instance()->registerCategory(this); } /** @@ -804,7 +841,7 @@ void LogCategory::setSeverity(LogSeverity severity) */ const LogCategory &LogCategory::defaultCategory() { - static const LogCategory *category = new LogCategory("default"); + static const LogCategory *category = LogCategory::create("default"); return *category; }
Each declaration of a LogCategory will create a new LogCategory, and will be stored in an unordered_set Logger::categories_. This means that when a plugin .so is unloaded and loaded, as happens when destructing and creating a CamereManager, we'll get duplicate categories. The Logger::registerCategory docs say "Log categories must have unique names. If a category with the same name already exists this function performs no operation.". The code does not comply with this. We solve the issue with two changes: Change the unordered_set to a vector for simplicity, as there's no need for an unordered_set. Instead of using the LogCategory constructor to create new categories in _LOG_CATEGORY() macro, use a factory method. The factory method will return either an existing LogCategory if one exists with the given name, or a newly created one. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- include/libcamera/base/log.h | 6 +++-- src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++----- 2 files changed, 48 insertions(+), 9 deletions(-)