| Message ID | 20250203175936.206161-1-pobrn@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Feb 03, 2025 at 05:59:39PM +0000, Barnabás Pőcze wrote: > Store the `LogCategory` objects in an `std::list`. This eliminates > the need for manually deleting them in the destructor while guaranteeing > address stability. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/libcamera/base/log.cpp | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp > index 69aa16c4c..003e2cf3a 100644 > --- a/src/libcamera/base/log.cpp > +++ b/src/libcamera/base/log.cpp > @@ -407,7 +407,7 @@ private: > static bool destroyed_; > > Mutex mutex_; > - std::vector<LogCategory *> categories_; > + std::list<LogCategory> categories_; looks like the file didn't include <vector> ! > std::list<std::pair<std::string, LogSeverity>> levels_; > > std::shared_ptr<LogOutput> output_; > @@ -524,9 +524,6 @@ void logSetLevel(const char *category, const char *level) > Logger::~Logger() > { > destroyed_ = true; > - > - for (LogCategory *category : categories_) > - delete category; > } > > /** > @@ -659,9 +656,9 @@ void Logger::logSetLevel(const char *category, const char *level) > > MutexLocker locker(mutex_); > > - for (LogCategory *c : categories_) { > - if (c->name() == category) { > - c->setSeverity(severity); > + for (LogCategory &c : categories_) { > + if (c.name() == category) { > + c.setSeverity(severity); > break; > } > } > @@ -717,12 +714,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name) > { > MutexLocker locker(mutex_); > > - for (LogCategory *category : categories_) { > - if (category->name() == name) > - return category; > + for (LogCategory &category : categories_) { > + if (category.name() == name) > + return &category; > } > > - LogCategory *category = categories_.emplace_back(new LogCategory(name)); > + LogCategory &category = categories_.emplace_back(name); > > for (const std::pair<std::string, LogSeverity> &level : levels_) { > bool match = true; > @@ -739,12 +736,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name) > } > > if (match) { > - category->setSeverity(level.second); > + category.setSeverity(level.second); > break; > } > } > > - return category; > + return &category; Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > } > > /** > -- > 2.48.1 > >
Hi Barnabás, Thank you for the patch. On Mon, Feb 03, 2025 at 05:59:39PM +0000, Barnabás Pőcze wrote: > Store the `LogCategory` objects in an `std::list`. This eliminates > the need for manually deleting them in the destructor while guaranteeing > address stability. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/libcamera/base/log.cpp | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp > index 69aa16c4c..003e2cf3a 100644 > --- a/src/libcamera/base/log.cpp > +++ b/src/libcamera/base/log.cpp > @@ -407,7 +407,7 @@ private: > static bool destroyed_; > > Mutex mutex_; > - std::vector<LogCategory *> categories_; > + std::list<LogCategory> categories_; Will this cause two allocations, one for the list node, and one for the LogCategory instance ? Iterating over a vector should also be (slightly) more efficient than iterating over a list. What is the gola of this patch ? If it is only to avoid manual deletion, we can also store unique pointers in the vector. > std::list<std::pair<std::string, LogSeverity>> levels_; > > std::shared_ptr<LogOutput> output_; > @@ -524,9 +524,6 @@ void logSetLevel(const char *category, const char *level) > Logger::~Logger() > { > destroyed_ = true; > - > - for (LogCategory *category : categories_) > - delete category; > } > > /** > @@ -659,9 +656,9 @@ void Logger::logSetLevel(const char *category, const char *level) > > MutexLocker locker(mutex_); > > - for (LogCategory *c : categories_) { > - if (c->name() == category) { > - c->setSeverity(severity); > + for (LogCategory &c : categories_) { > + if (c.name() == category) { > + c.setSeverity(severity); > break; > } > } > @@ -717,12 +714,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name) > { > MutexLocker locker(mutex_); > > - for (LogCategory *category : categories_) { > - if (category->name() == name) > - return category; > + for (LogCategory &category : categories_) { > + if (category.name() == name) > + return &category; > } > > - LogCategory *category = categories_.emplace_back(new LogCategory(name)); > + LogCategory &category = categories_.emplace_back(name); > > for (const std::pair<std::string, LogSeverity> &level : levels_) { > bool match = true; > @@ -739,12 +736,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name) > } > > if (match) { > - category->setSeverity(level.second); > + category.setSeverity(level.second); > break; > } > } > > - return category; > + return &category; > } > > /**
Hi 2025. február 6., csütörtök 18:16 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > Hi Barnabás, > > Thank you for the patch. > > On Mon, Feb 03, 2025 at 05:59:39PM +0000, Barnabás Pőcze wrote: > > Store the `LogCategory` objects in an `std::list`. This eliminates > > the need for manually deleting them in the destructor while guaranteeing > > address stability. > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > src/libcamera/base/log.cpp | 23 ++++++++++------------- > > 1 file changed, 10 insertions(+), 13 deletions(-) > > > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp > > index 69aa16c4c..003e2cf3a 100644 > > --- a/src/libcamera/base/log.cpp > > +++ b/src/libcamera/base/log.cpp > > @@ -407,7 +407,7 @@ private: > > static bool destroyed_; > > > > Mutex mutex_; > > - std::vector<LogCategory *> categories_; > > + std::list<LogCategory> categories_; > > Will this cause two allocations, one for the list node, and one for the > LogCategory instance ? With std::list there will be a single allocation for each category. > > Iterating over a vector should also be (slightly) more efficient than > iterating over a list. What is the gola of this patch ? If it is only to > avoid manual deletion, we can also store unique pointers in the vector. To avoid manual deletion. A vector was another option that I have considered, but in the end I went with a list because it's less code and the types look cleaner. I'm not sure if there is much difference between std::list<T> and std::vector<std::unique_ptr<T>> since both options kind of require one indirection when going to the next element. > > > std::list<std::pair<std::string, LogSeverity>> levels_; > > > > std::shared_ptr<LogOutput> output_; > > @@ -524,9 +524,6 @@ void logSetLevel(const char *category, const char *level) > > Logger::~Logger() > > { > > destroyed_ = true; > > - > > - for (LogCategory *category : categories_) > > - delete category; > > } > > > > /** > > @@ -659,9 +656,9 @@ void Logger::logSetLevel(const char *category, const char *level) > > > > MutexLocker locker(mutex_); > > > > - for (LogCategory *c : categories_) { > > - if (c->name() == category) { > > - c->setSeverity(severity); > > + for (LogCategory &c : categories_) { > > + if (c.name() == category) { > > + c.setSeverity(severity); > > break; > > } > > } > > @@ -717,12 +714,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name) > > { > > MutexLocker locker(mutex_); > > > > - for (LogCategory *category : categories_) { > > - if (category->name() == name) > > - return category; > > + for (LogCategory &category : categories_) { > > + if (category.name() == name) > > + return &category; > > } > > > > - LogCategory *category = categories_.emplace_back(new LogCategory(name)); > > + LogCategory &category = categories_.emplace_back(name); > > > > for (const std::pair<std::string, LogSeverity> &level : levels_) { > > bool match = true; > > @@ -739,12 +736,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name) > > } > > > > if (match) { > > - category->setSeverity(level.second); > > + category.setSeverity(level.second); > > break; > > } > > } > > > > - return category; > > + return &category; > > } > > > > /** > > -- > Regards, > > Laurent Pinchart >
On Thu, Feb 06, 2025 at 05:47:59PM +0000, Barnabás Pőcze wrote: > 2025. február 6., csütörtök 18:16 keltezéssel, Laurent Pinchart írta: > > On Mon, Feb 03, 2025 at 05:59:39PM +0000, Barnabás Pőcze wrote: > > > Store the `LogCategory` objects in an `std::list`. This eliminates > > > the need for manually deleting them in the destructor while guaranteeing > > > address stability. > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > --- > > > src/libcamera/base/log.cpp | 23 ++++++++++------------- > > > 1 file changed, 10 insertions(+), 13 deletions(-) > > > > > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp > > > index 69aa16c4c..003e2cf3a 100644 > > > --- a/src/libcamera/base/log.cpp > > > +++ b/src/libcamera/base/log.cpp > > > @@ -407,7 +407,7 @@ private: > > > static bool destroyed_; > > > > > > Mutex mutex_; > > > - std::vector<LogCategory *> categories_; > > > + std::list<LogCategory> categories_; > > > > Will this cause two allocations, one for the list node, and one for the > > LogCategory instance ? > > With std::list there will be a single allocation for each category. I haven't found this being documented in https://en.cppreference.com/w/cpp/container/list but I agree that's how the libstdc++ and libc++ implementations behave. > > Iterating over a vector should also be (slightly) more efficient than > > iterating over a list. What is the gola of this patch ? If it is only to > > avoid manual deletion, we can also store unique pointers in the vector. > > To avoid manual deletion. A vector was another option that I have considered, > but in the end I went with a list because it's less code and the types look cleaner. > I'm not sure if there is much difference between std::list<T> and > std::vector<std::unique_ptr<T>> since both options kind of require one indirection > when going to the next element. It probably won't make much of a difference indeed. I'm OK with this patch, but I think you'll need to call categories_.clear() explicitly in the destructor with the mutex locked, or TSA should complain. > > > std::list<std::pair<std::string, LogSeverity>> levels_; > > > > > > std::shared_ptr<LogOutput> output_; > > > @@ -524,9 +524,6 @@ void logSetLevel(const char *category, const char *level) > > > Logger::~Logger() > > > { > > > destroyed_ = true; > > > - > > > - for (LogCategory *category : categories_) > > > - delete category; > > > } > > > > > > /** > > > @@ -659,9 +656,9 @@ void Logger::logSetLevel(const char *category, const char *level) > > > > > > MutexLocker locker(mutex_); > > > > > > - for (LogCategory *c : categories_) { > > > - if (c->name() == category) { > > > - c->setSeverity(severity); > > > + for (LogCategory &c : categories_) { > > > + if (c.name() == category) { > > > + c.setSeverity(severity); > > > break; > > > } > > > } > > > @@ -717,12 +714,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name) > > > { > > > MutexLocker locker(mutex_); > > > > > > - for (LogCategory *category : categories_) { > > > - if (category->name() == name) > > > - return category; > > > + for (LogCategory &category : categories_) { > > > + if (category.name() == name) > > > + return &category; > > > } > > > > > > - LogCategory *category = categories_.emplace_back(new LogCategory(name)); > > > + LogCategory &category = categories_.emplace_back(name); > > > > > > for (const std::pair<std::string, LogSeverity> &level : levels_) { > > > bool match = true; > > > @@ -739,12 +736,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name) > > > } > > > > > > if (match) { > > > - category->setSeverity(level.second); > > > + category.setSeverity(level.second); > > > break; > > > } > > > } > > > > > > - return category; > > > + return &category; > > > } > > > > > > /**
diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp index 69aa16c4c..003e2cf3a 100644 --- a/src/libcamera/base/log.cpp +++ b/src/libcamera/base/log.cpp @@ -407,7 +407,7 @@ private: static bool destroyed_; Mutex mutex_; - std::vector<LogCategory *> categories_; + std::list<LogCategory> categories_; std::list<std::pair<std::string, LogSeverity>> levels_; std::shared_ptr<LogOutput> output_; @@ -524,9 +524,6 @@ void logSetLevel(const char *category, const char *level) Logger::~Logger() { destroyed_ = true; - - for (LogCategory *category : categories_) - delete category; } /** @@ -659,9 +656,9 @@ void Logger::logSetLevel(const char *category, const char *level) MutexLocker locker(mutex_); - for (LogCategory *c : categories_) { - if (c->name() == category) { - c->setSeverity(severity); + for (LogCategory &c : categories_) { + if (c.name() == category) { + c.setSeverity(severity); break; } } @@ -717,12 +714,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name) { MutexLocker locker(mutex_); - for (LogCategory *category : categories_) { - if (category->name() == name) - return category; + for (LogCategory &category : categories_) { + if (category.name() == name) + return &category; } - LogCategory *category = categories_.emplace_back(new LogCategory(name)); + LogCategory &category = categories_.emplace_back(name); for (const std::pair<std::string, LogSeverity> &level : levels_) { bool match = true; @@ -739,12 +736,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name) } if (match) { - category->setSeverity(level.second); + category.setSeverity(level.second); break; } } - return category; + return &category; } /**
Store the `LogCategory` objects in an `std::list`. This eliminates the need for manually deleting them in the destructor while guaranteeing address stability. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/libcamera/base/log.cpp | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)