glib2/timezone-madness.patch

From b4138bd4acc04ad572dfa042a384aaaa6e4621ad Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= <antoniof@gnome.org>
Date: Wed, 23 Sep 2020 18:13:47 +0100
Subject: [PATCH 1/4] gtimezone: Split out fallback timezone identification for
 unix

When the TZ environment variable is not set, we get the local timezone
identifier by reading specific files.

We are going to need these identifiers earlier, so split this logic into
its own function, in preparation for the next commit.

Based on idea proposed by Sebastian Keller <skeller@gnome.org>.
---
 glib/gtimezone.c | 127 +++++++++++++++++++++++++++--------------------
 1 file changed, 73 insertions(+), 54 deletions(-)

diff --git a/glib/gtimezone.c b/glib/gtimezone.c
index ef67ec50b..d90a9bb73 100644
--- a/glib/gtimezone.c
+++ b/glib/gtimezone.c
@@ -438,11 +438,80 @@ zone_for_constant_offset (GTimeZone *gtz, const gchar *name)
 }
 
 #ifdef G_OS_UNIX
+static gchar *
+zone_identifier_unix (void)
+{
+  gchar *resolved_identifier = NULL;
+  gsize prefix_len = 0;
+  gchar *canonical_path = NULL;
+  GError *read_link_err = NULL;
+  const gchar *tzdir;
+
+  /* Resolve the actual timezone pointed to by /etc/localtime. */
+  resolved_identifier = g_file_read_link ("/etc/localtime", &read_link_err);
+  if (resolved_identifier == NULL)
+    {
+      gboolean not_a_symlink = g_error_matches (read_link_err,
+                                                G_FILE_ERROR,
+                                                G_FILE_ERROR_INVAL);
+      g_clear_error (&read_link_err);
+
+      /* Fallback to the content of /var/db/zoneinfo or /etc/timezone
+       * if /etc/localtime is not a symlink. /var/db/zoneinfo is
+       * where 'tzsetup' program on FreeBSD and DragonflyBSD stores
+       * the timezone chosen by the user. /etc/timezone is where user
+       * choice is expressed on Gentoo OpenRC and others. */
+      if (not_a_symlink && (g_file_get_contents ("/var/db/zoneinfo",
+                                                 &resolved_identifier,
+                                                 NULL, NULL) ||
+                            g_file_get_contents ("/etc/timezone",
+                                                 &resolved_identifier,
+                                                 NULL, NULL)))
+        g_strchomp (resolved_identifier);
+      else
+        {
+          /* Error */
+          g_assert (resolved_identifier == NULL);
+          goto out;
+        }
+    }
+  else
+    {
+      /* Resolve relative path */
+      canonical_path = g_canonicalize_filename (resolved_identifier, "/etc");
+      g_free (resolved_identifier);
+      resolved_identifier = g_steal_pointer (&canonical_path);
+    }
+
+  tzdir = g_getenv ("TZDIR");
+  if (tzdir == NULL)
+    tzdir = "/usr/share/zoneinfo";
+
+  /* Strip the prefix and slashes if possible. */
+  if (g_str_has_prefix (resolved_identifier, tzdir))
+    {
+      prefix_len = strlen (tzdir);
+      while (*(resolved_identifier + prefix_len) == '/')
+        prefix_len++;
+    }
+
+  if (prefix_len > 0)
+    memmove (resolved_identifier, resolved_identifier + prefix_len,
+             strlen (resolved_identifier) - prefix_len + 1  /* nul terminator */);
+
+  g_assert (resolved_identifier != NULL);
+
+out:
+  g_free (canonical_path);
+
+  return resolved_identifier;
+}
+
 static GBytes*
 zone_info_unix (const gchar  *identifier,
                 gchar       **out_identifier)
 {
-  gchar *filename;
+  gchar *filename = NULL;
   GMappedFile *file = NULL;
   GBytes *zoneinfo = NULL;
   gchar *resolved_identifier = NULL;
@@ -470,61 +539,11 @@ zone_info_unix (const gchar  *identifier,
     }
   else
     {
-      gsize prefix_len = 0;
-      gchar *canonical_path = NULL;
-      GError *read_link_err = NULL;
-
-      filename = g_strdup ("/etc/localtime");
-
-      /* Resolve the actual timezone pointed to by /etc/localtime. */
-      resolved_identifier = g_file_read_link (filename, &read_link_err);
+      resolved_identifier = zone_identifier_unix ();
       if (resolved_identifier == NULL)
-        {
-          gboolean not_a_symlink = g_error_matches (read_link_err,
-                                                    G_FILE_ERROR,
-                                                    G_FILE_ERROR_INVAL);
-          g_clear_error (&read_link_err);
-
-          /* Fallback to the content of /var/db/zoneinfo or /etc/timezone
-           * if /etc/localtime is not a symlink. /var/db/zoneinfo is
-           * where 'tzsetup' program on FreeBSD and DragonflyBSD stores
-           * the timezone chosen by the user. /etc/timezone is where user
-           * choice is expressed on Gentoo OpenRC and others. */
-          if (not_a_symlink && (g_file_get_contents ("/var/db/zoneinfo",
-                                                     &resolved_identifier,
-                                                     NULL, NULL) ||
-                                g_file_get_contents ("/etc/timezone",
-                                                     &resolved_identifier,
-                                                     NULL, NULL)))
-            g_strchomp (resolved_identifier);
-          else
-            {
-              /* Error */
-              g_assert (resolved_identifier == NULL);
-              goto out;
-            }
-        }
-      else
-        {
-          /* Resolve relative path */
-          canonical_path = g_canonicalize_filename (resolved_identifier, "/etc");
-          g_free (resolved_identifier);
-          resolved_identifier = g_steal_pointer (&canonical_path);
-        }
+        goto out;
 
-      /* Strip the prefix and slashes if possible. */
-      if (g_str_has_prefix (resolved_identifier, tzdir))
-        {
-          prefix_len = strlen (tzdir);
-          while (*(resolved_identifier + prefix_len) == '/')
-            prefix_len++;
-        }
-
-      if (prefix_len > 0)
-        memmove (resolved_identifier, resolved_identifier + prefix_len,
-                 strlen (resolved_identifier) - prefix_len + 1  /* nul terminator */);
-
-      g_free (canonical_path);
+      filename = g_strdup ("/etc/localtime");
     }
 
   file = g_mapped_file_new (filename, FALSE, NULL);
-- 
GitLab


From 7e59a4c0d5ab8e08fe2cf596fb9512708e183de8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= <antoniof@gnome.org>
Date: Wed, 23 Sep 2020 18:28:40 +0100
Subject: [PATCH 2/4] gtimezone: Set resolved_identifier earlier

We have been passing a &resolved_identifier address around for multiple
functions to set it. Each function may either:

    1.  leaving it for the next function to set, if returning early;
    2.  set it to a duplicate of the passed identifier, if not NULL;
    3.  get a fallback value and set it, otherwise.

This can be simplified by setting it early to either:

    1.  a duplicate of the passed identifier, if not NULL;
    2.  a fallback value, otherwise.

This way we can avoid some unnecessary string duplication and freeing.
Also, on Windows, we avoid calling windows_default_tzname() twice.

But the main motivation for this change is enabling the performance
optimization in the next commit.
---
 glib/gtimezone.c | 76 ++++++++++++++++--------------------------------
 1 file changed, 25 insertions(+), 51 deletions(-)

diff --git a/glib/gtimezone.c b/glib/gtimezone.c
index d90a9bb73..40064c9ab 100644
--- a/glib/gtimezone.c
+++ b/glib/gtimezone.c
@@ -508,13 +508,12 @@ out:
 }
 
 static GBytes*
-zone_info_unix (const gchar  *identifier,
-                gchar       **out_identifier)
+zone_info_unix (const gchar *identifier,
+                const gchar *resolved_identifier)
 {
   gchar *filename = NULL;
   GMappedFile *file = NULL;
   GBytes *zoneinfo = NULL;
-  gchar *resolved_identifier = NULL;
   const gchar *tzdir;
 
   tzdir = g_getenv ("TZDIR");
@@ -527,8 +526,6 @@ zone_info_unix (const gchar  *identifier,
      glibc allows both syntaxes, so we should too */
   if (identifier != NULL)
     {
-      resolved_identifier = g_strdup (identifier);
-
       if (*identifier == ':')
         identifier ++;
 
@@ -539,7 +536,6 @@ zone_info_unix (const gchar  *identifier,
     }
   else
     {
-      resolved_identifier = zone_identifier_unix ();
       if (resolved_identifier == NULL)
         goto out;
 
@@ -559,10 +555,6 @@ zone_info_unix (const gchar  *identifier,
   g_assert (resolved_identifier != NULL);
 
 out:
-  if (out_identifier != NULL)
-    *out_identifier = g_steal_pointer (&resolved_identifier);
-
-  g_free (resolved_identifier);
   g_free (filename);
 
   return zoneinfo;
@@ -834,14 +826,13 @@ register_tzi_to_tzi (RegTZI *reg, TIME_ZONE_INFORMATION *tzi)
 
 static guint
 rules_from_windows_time_zone (const gchar   *identifier,
-                              gchar        **out_identifier,
-                              TimeZoneRule **rules,
-                              gboolean       copy_identifier)
+                              const gchar   *resolved_identifier,
+                              TimeZoneRule **rules)
 {
   HKEY key;
   gchar *subkey = NULL;
   gchar *subkey_dynamic = NULL;
-  gchar *key_name = NULL;
+  const gchar *key_name;
   const gchar *reg_key =
     "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion\\Time Zones\\";
   TIME_ZONE_INFORMATION tzi;
@@ -856,19 +847,15 @@ rules_from_windows_time_zone (const gchar   *identifier,
   if (GetSystemDirectoryW (winsyspath, MAX_PATH) == 0)
     return 0;
 
-  g_assert (copy_identifier == FALSE || out_identifier != NULL);
   g_assert (rules != NULL);
 
-  if (copy_identifier)
-    *out_identifier = NULL;
-
   *rules = NULL;
   key_name = NULL;
 
   if (!identifier)
-    key_name = windows_default_tzname ();
+    key_name = resolved_identifier;
   else
-    key_name = g_strdup (identifier);
+    key_name = identifier;
 
   if (!key_name)
     return 0;
@@ -1011,16 +998,9 @@ utf16_conv_failed:
       else
         (*rules)[rules_num - 1].start_year = (*rules)[rules_num - 2].start_year + 1;
 
-      if (copy_identifier)
-        *out_identifier = g_steal_pointer (&key_name);
-      else
-        g_free (key_name);
-
       return rules_num;
     }
 
-  g_free (key_name);
-
   return 0;
 }
 
@@ -1521,16 +1501,13 @@ parse_identifier_boundaries (gchar **pos, TimeZoneRule *tzr)
  */
 static guint
 rules_from_identifier (const gchar   *identifier,
-                       gchar        **out_identifier,
                        TimeZoneRule **rules)
 {
   gchar *pos;
   TimeZoneRule tzr;
 
-  g_assert (out_identifier != NULL);
   g_assert (rules != NULL);
 
-  *out_identifier = NULL;
   *rules = NULL;
 
   if (!identifier)
@@ -1545,7 +1522,6 @@ rules_from_identifier (const gchar   *identifier,
 
   if (*pos == 0)
     {
-      *out_identifier = g_strdup (identifier);
       return create_ruleset_from_rule (rules, &tzr);
     }
 
@@ -1566,14 +1542,8 @@ rules_from_identifier (const gchar   *identifier,
       /* Use US rules, Windows' default is Pacific Standard Time */
       if ((rules_num = rules_from_windows_time_zone ("Pacific Standard Time",
                                                      NULL,
-                                                     rules,
-                                                     FALSE)))
+                                                     rules)))
         {
-          /* We don't want to hardcode our identifier here as
-           * "Pacific Standard Time", use what was passed in
-           */
-          *out_identifier = g_strdup (identifier);
-
           for (i = 0; i < rules_num - 1; i++)
             {
               (*rules)[i].std_offset = - tzr.std_offset;
@@ -1594,7 +1564,6 @@ rules_from_identifier (const gchar   *identifier,
   if (!parse_identifier_boundaries (&pos, &tzr))
     return 0;
 
-  *out_identifier = g_strdup (identifier);
   return create_ruleset_from_rule (rules, &tzr);
 }
 
@@ -1605,17 +1574,13 @@ parse_footertz (const gchar *footer, size_t footerlen)
   gchar *tzstring = g_strndup (footer + 1, footerlen - 2);
   GTimeZone *footertz = NULL;
 
-  /* FIXME: it might make sense to modify rules_from_identifier to
-     allow NULL to be passed instead of &ident, saving the strdup/free
-     pair.  The allocation for tzstring could also be avoided by
+  /* FIXME: The allocation for tzstring could be avoided by
      passing a gsize identifier_len argument to rules_from_identifier
      and changing the code in that function to stop assuming that
      identifier is nul-terminated.  */
-  gchar *ident;
   TimeZoneRule *rules;
-  guint rules_num = rules_from_identifier (tzstring, &ident, &rules);
+  guint rules_num = rules_from_identifier (tzstring, &rules);
 
-  g_free (ident);
   g_free (tzstring);
   if (rules_num > 1)
     {
@@ -1723,6 +1688,16 @@ g_time_zone_new (const gchar *identifier)
           G_UNLOCK (time_zones);
           return tz;
         }
+      else
+        resolved_identifier = g_strdup (identifier);
+    }
+  else
+    {
+#ifdef G_OS_UNIX
+      resolved_identifier = zone_identifier_unix ();
+#elif defined (G_OS_WIN32)
+      resolved_identifier = windows_default_tzname ();
+#endif
     }
 
   tz = g_slice_new0 (GTimeZone);
@@ -1731,7 +1706,7 @@ g_time_zone_new (const gchar *identifier)
   zone_for_constant_offset (tz, identifier);
 
   if (tz->t_info == NULL &&
-      (rules_num = rules_from_identifier (identifier, &resolved_identifier, &rules)))
+      (rules_num = rules_from_identifier (identifier, &rules)))
     {
       init_zone_from_rules (tz, rules, rules_num, g_steal_pointer (&resolved_identifier));
       g_free (rules);
@@ -1740,7 +1715,7 @@ g_time_zone_new (const gchar *identifier)
   if (tz->t_info == NULL)
     {
 #ifdef G_OS_UNIX
-      GBytes *zoneinfo = zone_info_unix (identifier, &resolved_identifier);
+      GBytes *zoneinfo = zone_info_unix (identifier, resolved_identifier);
       if (zoneinfo != NULL)
         {
           init_zone_from_iana_info (tz, zoneinfo, g_steal_pointer (&resolved_identifier));
@@ -1748,9 +1723,8 @@ g_time_zone_new (const gchar *identifier)
         }
 #elif defined (G_OS_WIN32)
       if ((rules_num = rules_from_windows_time_zone (identifier,
-                                                     &resolved_identifier,
-                                                     &rules,
-                                                     TRUE)))
+                                                     resolved_identifier,
+                                                     &rules)))
         {
           init_zone_from_rules (tz, rules, rules_num, g_steal_pointer (&resolved_identifier));
           g_free (rules);
@@ -1777,7 +1751,7 @@ g_time_zone_new (const gchar *identifier)
                   rules[0].start_year = MIN_TZYEAR;
                   rules[1].start_year = MAX_TZYEAR;
 
-                  init_zone_from_rules (tz, rules, 2, windows_default_tzname ());
+                  init_zone_from_rules (tz, rules, 2, g_steal_pointer (&resolved_identifier));
                 }
 
               g_free (rules);
-- 
GitLab


From 5237b4984306847dff05db54b5c12c83662f2f1d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= <antoniof@gnome.org>
Date: Thu, 1 Oct 2020 21:11:44 +0100
Subject: [PATCH 3/4] gtimezone: Cache default timezone indefinitely

We cache GTimeZone instances to avoid expensive construction when the
same id is requested again.

However, if the NULL id is passed to g_time_zone_new(), we always
construct a new instance for the default/fallback timezone.

With the recent introduction of some heavy calculations[1], repeated
instance construction in such cases has visible performance impact in
nautilus list view and other such GtkTreeView consumers.

To avoid this, cache reference to a constructed default timezone and
use it the next time g_time_zone_new() is called with NULL argument,
as long as the default identifier doesn't change. We already did the
same for the local timezone[2].

Fixes: https://gitlab.gnome.org/GNOME/glib/-/issues/2204

Based on idea proposed by Sebastian Keller <skeller@gnome.org>.

[1] 25d950b61f92f25cc9ab20d683aa4d6969f93098
[2] 551e83662de9815d161a82c760cfa77995905740
---
 glib/gtimezone.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/glib/gtimezone.c b/glib/gtimezone.c
index 40064c9ab..035da2f45 100644
--- a/glib/gtimezone.c
+++ b/glib/gtimezone.c
@@ -195,6 +195,8 @@ struct _GTimeZone
 
 G_LOCK_DEFINE_STATIC (time_zones);
 static GHashTable/*<string?, GTimeZone>*/ *time_zones;
+G_LOCK_DEFINE_STATIC (tz_default);
+static GTimeZone *tz_default = NULL;
 G_LOCK_DEFINE_STATIC (tz_local);
 static gchar *tzenv_cached = NULL;
 static GTimeZone *tz_local = NULL;
@@ -1675,12 +1677,12 @@ g_time_zone_new (const gchar *identifier)
   gint rules_num;
   gchar *resolved_identifier = NULL;
 
-  G_LOCK (time_zones);
-  if (time_zones == NULL)
-    time_zones = g_hash_table_new (g_str_hash, g_str_equal);
-
   if (identifier)
     {
+      G_LOCK (time_zones);
+      if (time_zones == NULL)
+        time_zones = g_hash_table_new (g_str_hash, g_str_equal);
+
       tz = g_hash_table_lookup (time_zones, identifier);
       if (tz)
         {
@@ -1693,11 +1695,26 @@ g_time_zone_new (const gchar *identifier)
     }
   else
     {
+      G_LOCK (tz_default);
 #ifdef G_OS_UNIX
       resolved_identifier = zone_identifier_unix ();
 #elif defined (G_OS_WIN32)
       resolved_identifier = windows_default_tzname ();
 #endif
+      if (tz_default)
+        {
+          /* Flush default if changed */
+          if (g_strcmp0 (tz_default->name, resolved_identifier))
+            g_clear_pointer (&tz_default, g_time_zone_unref);
+          else
+            {
+              tz = g_time_zone_ref (tz_default);
+              G_UNLOCK (tz_default);
+
+              g_free (resolved_identifier);
+              return tz;
+            }
+        }
     }
 
   tz = g_slice_new0 (GTimeZone);
@@ -1773,9 +1790,19 @@ g_time_zone_new (const gchar *identifier)
     {
       if (identifier)
         g_hash_table_insert (time_zones, tz->name, tz);
+      else if (tz->name)
+        {
+          /* Caching reference */
+          g_atomic_int_inc (&tz->ref_count);
+          tz_default = tz;
+        }
     }
   g_atomic_int_inc (&tz->ref_count);
-  G_UNLOCK (time_zones);
+
+  if (identifier)
+    G_UNLOCK (time_zones);
+  else
+    G_UNLOCK (tz_default);
 
   return tz;
 }
-- 
GitLab


From 02753644b33660a175d1a9b1ea9e8717c314ef23 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= <antoniof@gnome.org>
Date: Wed, 23 Sep 2020 19:36:49 +0100
Subject: [PATCH 4/4] Revert "gtimezone: Cache timezones based on the
 identifier they were created by"

This reverts commit 851241f19a3fd9ec693b3dd8f37a84c7f970984a.

That commit avoids a performance regression but introduces a behavior regression:
changes to /etc/localtime have no effect for the remaining of the application's
runtime.

With the optimization introduced by the previous commit, we can pass NULL to
g_time_zone_new() repeatedly with no performance drawback, so we no longer have
to workaround this case.
---
 glib/gtimezone.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/glib/gtimezone.c b/glib/gtimezone.c
index 035da2f45..8e38bb549 100644
--- a/glib/gtimezone.c
+++ b/glib/gtimezone.c
@@ -198,7 +198,6 @@ static GHashTable/*<string?, GTimeZone>*/ *time_zones;
 G_LOCK_DEFINE_STATIC (tz_default);
 static GTimeZone *tz_default = NULL;
 G_LOCK_DEFINE_STATIC (tz_local);
-static gchar *tzenv_cached = NULL;
 static GTimeZone *tz_local = NULL;
 
 #define MIN_TZYEAR 1916 /* Daylight Savings started in WWI */
@@ -1863,17 +1862,11 @@ g_time_zone_new_local (void)
   G_LOCK (tz_local);
 
   /* Is time zone changed and must be flushed? */
-  if (tz_local && g_strcmp0 (tzenv, tzenv_cached) != 0)
-    {
-      g_clear_pointer (&tz_local, g_time_zone_unref);
-      g_clear_pointer (&tzenv_cached, g_free);
-    }
+  if (tz_local && g_strcmp0 (g_time_zone_get_identifier (tz_local), tzenv))
+    g_clear_pointer (&tz_local, g_time_zone_unref);
 
   if (tz_local == NULL)
-    {
-      tz_local = g_time_zone_new (tzenv);
-      tzenv_cached = g_strdup (tzenv);
-    }
+    tz_local = g_time_zone_new (tzenv);
 
   tz = g_time_zone_ref (tz_local);
 
-- 
GitLab