From 252e4a676b80da3a4ea383517bd90cf7824554ea Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Tue, 15 Jun 2021 12:45:35 +0200
Subject: [PATCH] core: cache GVariant for result of GetSettings()

The GetSettings() call is not the only place where we convert a
NMConnection to D-Bus. However it is one of the most prominent ones
with a measurable performance overhead.

The connection seldom changes, so it makes sense to cache it.

Note that GetSettings() is the only caller that specifies an option,
thus it's the only caller that converts a NMConnection to variant
in this particular way. That means, other callers don't benefit from
this caching and we could not cache the variant in the NMConnection
instance itself, because those callers use different parameters.
---
 src/core/settings/nm-settings-connection.c | 67 ++++++++++++++++++++--
 1 file changed, 62 insertions(+), 5 deletions(-)

diff --git a/src/core/settings/nm-settings-connection.c b/src/core/settings/nm-settings-connection.c
index 520dcc24a0..71431f29d9 100644
--- a/src/core/settings/nm-settings-connection.c
+++ b/src/core/settings/nm-settings-connection.c
@@ -80,6 +80,11 @@ typedef struct _NMSettingsConnectionPrivate {
 
     NMConnection *connection;
 
+    struct {
+        NMConnectionSerializationOptions options;
+        GVariant *                       variant;
+    } getsettings_cached;
+
     NMSettingsStorage *storage;
 
     char *filename;
@@ -249,6 +254,57 @@ _seen_bssids_hash_new(void)
 
 /*****************************************************************************/
 
+static void
+_getsettings_cached_clear(NMSettingsConnectionPrivate *priv)
+{
+    if (nm_clear_pointer(&priv->getsettings_cached.variant, g_variant_unref)) {
+        priv->getsettings_cached.options.timestamp.has = FALSE;
+        priv->getsettings_cached.options.timestamp.val = 0;
+        nm_clear_g_free((gpointer *) &priv->getsettings_cached.options.seen_bssids);
+    }
+}
+
+static GVariant *
+_getsettings_cached_get(NMSettingsConnection *self, const NMConnectionSerializationOptions *options)
+{
+    NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE(self);
+    GVariant *                   variant;
+
+    if (priv->getsettings_cached.variant) {
+        if (nm_connection_serialization_options_equal(&priv->getsettings_cached.options, options)) {
+#if NM_MORE_ASSERTS > 10
+            gs_unref_variant GVariant *variant2 = NULL;
+
+            variant = nm_connection_to_dbus_full(priv->connection,
+                                                 NM_CONNECTION_SERIALIZE_WITH_NON_SECRET,
+                                                 options);
+            nm_assert(variant);
+            variant2 = g_variant_new("(@a{sa{sv}})", variant);
+            nm_assert(g_variant_equal(priv->getsettings_cached.variant, variant2));
+#endif
+            return priv->getsettings_cached.variant;
+        }
+        _getsettings_cached_clear(priv);
+    }
+
+    nm_assert(!priv->getsettings_cached.options.seen_bssids);
+
+    variant = nm_connection_to_dbus_full(priv->connection,
+                                         NM_CONNECTION_SERIALIZE_WITH_NON_SECRET,
+                                         options);
+    nm_assert(variant);
+
+    priv->getsettings_cached.variant = g_variant_ref_sink(g_variant_new("(@a{sa{sv}})", variant));
+
+    priv->getsettings_cached.options = *options;
+    priv->getsettings_cached.options.seen_bssids =
+        nm_utils_strv_dup_packed(priv->getsettings_cached.options.seen_bssids, -1);
+
+    return priv->getsettings_cached.variant;
+}
+
+/*****************************************************************************/
+
 NMConnection *
 nm_settings_connection_get_connection(NMSettingsConnection *self)
 {
@@ -280,6 +336,8 @@ _nm_settings_connection_set_connection(NMSettingsConnection *           self,
         priv->connection = g_object_ref(new_connection);
         nmtst_connection_assert_unchanging(priv->connection);
 
+        _getsettings_cached_clear(priv);
+
         /* note that we only return @connection_old if the new connection actually differs from
          * before.
          *
@@ -1243,7 +1301,6 @@ get_settings_auth_cb(NMSettingsConnection * self,
 {
     gs_free const char **            seen_bssids = NULL;
     NMConnectionSerializationOptions options     = {};
-    GVariant *                       settings;
 
     if (error) {
         g_dbus_method_invocation_return_gerror(context, error);
@@ -1270,10 +1327,8 @@ get_settings_auth_cb(NMSettingsConnection * self,
      * get returned by the GetSecrets method which can be better
      * protected against leakage of secrets to unprivileged callers.
      */
-    settings = nm_connection_to_dbus_full(nm_settings_connection_get_connection(self),
-                                          NM_CONNECTION_SERIALIZE_WITH_NON_SECRET,
-                                          &options);
-    g_dbus_method_invocation_return_value(context, g_variant_new("(@a{sa{sv}})", settings));
+
+    g_dbus_method_invocation_return_value(context, _getsettings_cached_get(self, &options));
 }
 
 static void
@@ -2620,6 +2675,8 @@ dispose(GObject *object)
 
     g_clear_object(&priv->connection);
 
+    _getsettings_cached_clear(priv);
+
     nm_clear_pointer(&priv->kf_db_timestamps, nm_key_file_db_unref);
     nm_clear_pointer(&priv->kf_db_seen_bssids, nm_key_file_db_unref);