[libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix LogCategory creation issues

[Laurent Pinchart]

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 at 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.

-- 
Regards,

Laurent Pinchart