log: Avoid pushing the same fields more than once on the log context

Let's try to optimize against pushing the same fields multiple times
onto the log context. To achieve this we make the log context reference
counted and return an existing context object if it's using the same
fields.

A consequence of this is that we have to make sure attaching/detaching
is coupled to the lifetime of the context object, so we make the attach
and detach functions private for now. If we need independent attach/detach
in the future, we can make that work with some extra complexity but since
we don't need it yet, let's not support it for now.

diff --git a/src/basic/log.c b/src/basic/log.c
index 8b973f9e0a905502529aa63c01c7f249f1f89196..c713b9ac88d28af99a2158a3ddc0f99d2d88b7ba 100644 (file)
--- a/src/basic/log.c
+++ b/src/basic/log.c
@@ -73,6 +73,7 @@ static bool prohibit_ipc = false;
 static char *log_abort_msg = NULL;
 
 typedef struct LogContext {
+        unsigned n_ref;
         /* Depending on which destructor is used (log_context_free() or log_context_detach()) the memory
          * referenced by this is freed or not */
         char **fields;
@@ -1562,7 +1563,7 @@ bool log_context_enabled(void) {
         return saved_log_context_enabled;
 }
 
-LogContext* log_context_attach(LogContext *c) {
+static LogContext* log_context_attach(LogContext *c) {
         assert(c);
 
         _log_context_num_fields += strv_length(c->fields);
@@ -1571,7 +1572,7 @@ LogContext* log_context_attach(LogContext *c) {
         return LIST_PREPEND(ll, _log_context, c);
 }
 
-LogContext* log_context_detach(LogContext *c) {
+static LogContext* log_context_detach(LogContext *c) {
         if (!c)
                 return NULL;
 
@@ -1584,11 +1585,21 @@ LogContext* log_context_detach(LogContext *c) {
 }
 
 LogContext* log_context_new(char **fields, bool owned) {
+        if (!fields)
+                return NULL;
+
+        LIST_FOREACH(ll, i, _log_context)
+                if (i->fields == fields) {
+                        assert(!owned);
+                        return log_context_ref(i);
+                }
+
         LogContext *c = new(LogContext, 1);
         if (!c)
                 return NULL;
 
         *c = (LogContext) {
+                .n_ref = 1,
                 .fields = fields,
                 .owned = owned,
         };
@@ -1598,13 +1609,20 @@ LogContext* log_context_new(char **fields, bool owned) {
 
 LogContext* log_context_newv(struct iovec *input_iovec, size_t n_input_iovec, bool owned) {
         if (!input_iovec || n_input_iovec == 0)
-                return NULL; /* Nothing to do */
+                return NULL;
+
+        LIST_FOREACH(ll, i, _log_context)
+                if (i->input_iovec == input_iovec && i->n_input_iovec == n_input_iovec) {
+                        assert(!owned);
+                        return log_context_ref(i);
+                }
 
         LogContext *c = new(LogContext, 1);
         if (!c)
                 return NULL;
 
         *c = (LogContext) {
+                .n_ref = 1,
                 .input_iovec = input_iovec,
                 .n_input_iovec = n_input_iovec,
                 .owned = owned,
@@ -1613,7 +1631,7 @@ LogContext* log_context_newv(struct iovec *input_iovec, size_t n_input_iovec, bo
         return log_context_attach(c);
 }
 
-LogContext* log_context_free(LogContext *c) {
+static LogContext* log_context_free(LogContext *c) {
         if (!c)
                 return NULL;
 
@@ -1627,6 +1645,8 @@ LogContext* log_context_free(LogContext *c) {
         return mfree(c);
 }
 
+DEFINE_TRIVIAL_REF_UNREF_FUNC(LogContext, log_context, log_context_free);
+
 LogContext* log_context_new_consume(char **fields) {
         LogContext *c = log_context_new(fields, /*owned=*/ true);
         if (!c)

diff --git a/src/basic/log.h b/src/basic/log.h
index c4ac73e27bcd5e2d2a2609cce5e132809d48e13f..f17a97ee370212164062e7fc357ab0daa8ff0794 100644 (file)
--- a/src/basic/log.h
+++ b/src/basic/log.h
@@ -457,25 +457,23 @@ typedef struct LogContext LogContext;
 
 bool log_context_enabled(void);
 
-LogContext* log_context_attach(LogContext *c);
-LogContext* log_context_detach(LogContext *c);
-
 LogContext* log_context_new(char **fields, bool owned);
 LogContext* log_context_newv(struct iovec *input_iovec, size_t n_input_iovec, bool owned);
-LogContext* log_context_free(LogContext *c);
 
 /* Same as log_context_new(), but frees the given fields strv/iovec on failure. */
 LogContext* log_context_new_consume(char **fields);
 LogContext* log_context_new_consumev(struct iovec *input_iovec, size_t n_input_iovec);
 
+LogContext *log_context_ref(LogContext *c);
+LogContext *log_context_unref(LogContext *c);
+
+DEFINE_TRIVIAL_CLEANUP_FUNC(LogContext*, log_context_unref);
+
 /* Returns the number of attached log context objects. */
 size_t log_context_num_contexts(void);
 /* Returns the number of fields in all attached log contexts. */
 size_t log_context_num_fields(void);
 
-DEFINE_TRIVIAL_CLEANUP_FUNC(LogContext*, log_context_detach);
-DEFINE_TRIVIAL_CLEANUP_FUNC(LogContext*, log_context_free);
-
 #define LOG_CONTEXT_PUSH(...) \
         LOG_CONTEXT_PUSH_STRV(STRV_MAKE(__VA_ARGS__))
 
@@ -483,13 +481,13 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(LogContext*, log_context_free);
         LOG_CONTEXT_PUSH(snprintf_ok((char[LINE_MAX]) {}, LINE_MAX, __VA_ARGS__))
 
 #define _LOG_CONTEXT_PUSH_STRV(strv, c) \
-        _unused_ _cleanup_(log_context_freep) LogContext *c = log_context_new(strv, /*owned=*/ false);
+        _unused_ _cleanup_(log_context_unrefp) LogContext *c = log_context_new(strv, /*owned=*/ false);
 
 #define LOG_CONTEXT_PUSH_STRV(strv) \
         _LOG_CONTEXT_PUSH_STRV(strv, UNIQ_T(c, UNIQ))
 
 #define _LOG_CONTEXT_PUSH_IOV(input_iovec, n_input_iovec, c) \
-        _unused_ _cleanup_(log_context_freep) LogContext *c = log_context_newv(input_iovec, n_input_iovec, /*owned=*/ false);
+        _unused_ _cleanup_(log_context_unrefp) LogContext *c = log_context_newv(input_iovec, n_input_iovec, /*owned=*/ false);
 
 #define LOG_CONTEXT_PUSH_IOV(input_iovec, n_input_iovec) \
         _LOG_CONTEXT_PUSH_IOV(input_iovec, n_input_iovec, UNIQ_T(c, UNIQ))
@@ -503,19 +501,19 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(LogContext*, log_context_free);
         _unused_ _cleanup_strv_free_ strv = strv_new(s);                                                \
         if (!strv)                                                                                      \
                 free(s);                                                                                \
-        _unused_ _cleanup_(log_context_freep) LogContext *c = log_context_new_consume(TAKE_PTR(strv))
+        _unused_ _cleanup_(log_context_unrefp) LogContext *c = log_context_new_consume(TAKE_PTR(strv))
 
 #define LOG_CONTEXT_CONSUME_STR(s) \
         _LOG_CONTEXT_CONSUME_STR(s, UNIQ_T(c, UNIQ), UNIQ_T(sv, UNIQ))
 
 #define _LOG_CONTEXT_CONSUME_STRV(strv, c) \
-        _unused_ _cleanup_(log_context_freep) LogContext *c = log_context_new_consume(strv);
+        _unused_ _cleanup_(log_context_unrefp) LogContext *c = log_context_new_consume(strv);
 
 #define LOG_CONTEXT_CONSUME_STRV(strv) \
         _LOG_CONTEXT_CONSUME_STRV(strv, UNIQ_T(c, UNIQ))
 
 #define _LOG_CONTEXT_CONSUME_IOV(input_iovec, n_input_iovec, c) \
-        _unused_ _cleanup_(log_context_freep) LogContext *c = log_context_new_consumev(input_iovec, n_input_iovec);
+        _unused_ _cleanup_(log_context_unrefp) LogContext *c = log_context_new_consumev(input_iovec, n_input_iovec);
 
 #define LOG_CONTEXT_CONSUME_IOV(input_iovec, n_input_iovec) \
         _LOG_CONTEXT_CONSUME_IOV(input_iovec, n_input_iovec, UNIQ_T(c, UNIQ))

diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c
index e3f71ca302c5958754b38a9c26b5a7416bb67253..4387db3841b53a05970a295e88389661f7402e38 100644 (file)
--- a/src/libsystemd/sd-bus/sd-bus.c
+++ b/src/libsystemd/sd-bus/sd-bus.c
@@ -2940,7 +2940,7 @@ static int process_fd_check(sd_bus *bus, sd_bus_message *m) {
 }
 
 static int process_message(sd_bus *bus, sd_bus_message *m) {
-        _unused_ _cleanup_(log_context_freep) LogContext *c = NULL;
+        _unused_ _cleanup_(log_context_unrefp) LogContext *c = NULL;
         int r;
 
         assert(bus);

diff --git a/src/libsystemd/sd-device/device-monitor.c b/src/libsystemd/sd-device/device-monitor.c
index 0d2eea5f59104d31b34b9b92f1f33bdf89bdc0f7..1d8b7550b66dfae66d51486b4d7715d59330139d 100644 (file)
--- a/src/libsystemd/sd-device/device-monitor.c
+++ b/src/libsystemd/sd-device/device-monitor.c
@@ -242,7 +242,7 @@ _public_ int sd_device_monitor_stop(sd_device_monitor *m) {
 
 static int device_monitor_event_handler(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
         _cleanup_(sd_device_unrefp) sd_device *device = NULL;
-        _unused_ _cleanup_(log_context_freep) LogContext *c = NULL;
+        _unused_ _cleanup_(log_context_unrefp) LogContext *c = NULL;
         sd_device_monitor *m = ASSERT_PTR(userdata);
 
         if (device_monitor_receive_device(m, &device) <= 0)

diff --git a/src/test/test-log.c b/src/test/test-log.c
index 68b5cb5092c1fc74a0d309bd2d38ec0a2104f02f..cfbd45c2e34f84850a15d5d20976394fc064a4c6 100644 (file)
--- a/src/test/test-log.c
+++ b/src/test/test-log.c
@@ -80,9 +80,10 @@ static void test_log_context(void) {
                 LOG_CONTEXT_PUSH_STRV(strv);
                 LOG_CONTEXT_PUSH_STRV(strv);
 
-                /* Test that the log context was set up correctly. */
-                assert_se(log_context_num_contexts() == 4);
-                assert_se(log_context_num_fields() == 6);
+                /* Test that the log context was set up correctly. The strv we pushed twice should only
+                 * result in one log context which is reused. */
+                assert_se(log_context_num_contexts() == 3);
+                assert_se(log_context_num_fields() == 4);
 
                 /* Test that everything still works with modifications to the log context. */
                 test_log_struct();
@@ -94,8 +95,8 @@ static void test_log_context(void) {
                         LOG_CONTEXT_PUSH_STRV(strv);
 
                         /* Check that our nested fields got added correctly. */
-                        assert_se(log_context_num_contexts() == 6);
-                        assert_se(log_context_num_fields() == 9);
+                        assert_se(log_context_num_contexts() == 4);
+                        assert_se(log_context_num_fields() == 5);
 
                         /* Test that everything still works in a nested block. */
                         test_log_struct();
@@ -104,15 +105,15 @@ static void test_log_context(void) {
                 }
 
                 /* Check that only the fields from the nested block got removed. */
-                assert_se(log_context_num_contexts() == 4);
-                assert_se(log_context_num_fields() == 6);
+                assert_se(log_context_num_contexts() == 3);
+                assert_se(log_context_num_fields() == 4);
         }
 
         assert_se(log_context_num_contexts() == 0);
         assert_se(log_context_num_fields() == 0);
 
         {
-                _cleanup_(log_context_freep) LogContext *ctx = NULL;
+                _cleanup_(log_context_unrefp) LogContext *ctx = NULL;
 
                 char **strv = STRV_MAKE("SIXTH=ijn", "SEVENTH=PRP");
                 assert_se(ctx = log_context_new(strv, /*owned=*/ false));
@@ -146,6 +147,7 @@ static void test_log_context(void) {
                 assert_se(iovw);
                 assert_se(iovw_consume(iovw, strdup("MNO=pqr"), STRLEN("MNO=pqr") + 1) == 0);
 
+                LOG_CONTEXT_PUSH_IOV(iov, ELEMENTSOF(iov));
                 LOG_CONTEXT_PUSH_IOV(iov, ELEMENTSOF(iov));
                 LOG_CONTEXT_CONSUME_IOV(iovw->iovec, iovw->count);
                 LOG_CONTEXT_PUSH("STU=vwx");