Re:[PATCH] tzset did not catch changes to localtime [BZ #21060 ]

Re:[PATCH] tzset did not catch changes to localtime [BZ #21060 ]

[ma.jiang]

[-- Attachment #1.1: Type: text/plain, Size: 4761 bytes --]

Hi Calos,
  Sorry for the delay. We are busy doing other stuffs this year.
  I have write a small benchmark as you suggested. The result shows that the patch 
  will not hurt performance in normal cases(When TZ is not set).
#include <time.h>
volatile time_t re;
int main()
{
        const unsigned long int tt = 1024*1024;
        const unsigned li = 1000*1000 * 5;
        unsigned int i = 0;

        struct tm * x = gmtime((time_t *)&tt);
        while( i++ < li)
        {
                re = mktime (x);
        }
        return 0;
}
This test takes about 10 seconds to finish, and is not affected by the patch.
real    0m11.113s
user    0m3.592s
sys     0m7.508s
The behaviour seems strange, so I have done some investigations. 
I found most time were used by stat (in __tzfile_read). 
This is caused by following codes in tzset_internal (in tzset.c):

  /* Check whether the value changed since the last run.  */
  if (old_tz != NULL && tz != NULL && strcmp (tz, old_tz) == 0)
    /* No change, simply return.  */
    return;

  if (tz == NULL)
    /* No user specification; use the site-wide default.  */
    tz = TZDEFAULT;
	
When TZ is not set, old_tz is TZDEFAULT (which is not NULL), but tz is NULL.
So, tzset will read the TZDEFAULT file when TZ is not set. Fortunately, there 
have been codes that use stat to avoid unnecessary reloads since 2004(in __tzfile_read).

When TZ is set to "/etc/localtime", the test will end in about 1 second, this time 
tzset will not try stat localtime). Although much faster, this is not right as 
the /etc/localtime could be modified silently by others.

I have build a new patch that make use of __use_tzfile to avoid the wrong behaviour.
After patched, tzset should run as fast as the current one when TZ is set to something 
that does not rely on tzfile.
Is this patch OK now?


--- a/time/tzset.c
+++ b/time/tzset.c
@@ -386,22 +386,28 @@ tzset_internal (int always)
   if (tz && *tz == ':')
     ++tz;

-  /* Check whether the value changed since the last run.  */
-  if (old_tz != NULL && tz != NULL && strcmp (tz, old_tz) == 0)
-    /* No change, simply return.  */
-    return;
-
   if (tz == NULL)
     /* No user specification; use the site-wide default.  */
     tz = TZDEFAULT;

+  /* Check whether the tz value changed since the last run.  */
+  int tz_changed = old_tz ? strcmp (tz, old_tz) : 1;
+  if ((old_tz == NULL)
+      || (old_tz != NULL && tz_changed))
+  {
+    /* Save the value of `tz'. */
+    free (old_tz);
+    old_tz = __strdup (tz);
+  }
+  
+  /* When using tzfile, must check whether it changed. */
+  if (!__use_tzfile && !tz_changed)
+    return;
+
+  /* Going to recaculate values. */
   tz_rules[0].name = NULL;
   tz_rules[1].name = NULL;

-  /* Save the value of `tz'.  */
-  free (old_tz);
-  old_tz = tz ? __strdup (tz) : NULL;
-
   /* Try to read a data file.  */
   __tzfile_read (tz, 0, NULL);
   if (__use_tzfile)





------------------原始邮件------------------
发件人：CarlosO'Donell <carlos@redhat.com>
收件人：马江10100629;libc-alpha@sourceware.org <libc-alpha@sourceware.org>;
日 期 ：2017年11月02日 23:33
主 题 ：Re: [PATCH]   tzset did not catch changes to localtime  [BZ #21060 ]
On 11/02/2017 02:03 AM, ma.jiang@zte.com.cn wrote:
> Hi, If users modified the local timezone file (usually
> /etc/localtime) while keep the TZ environment variable untouched,
> tzset will not recalculate tzname. This is not right. Patch attached
> should fix this problem, is it OK for trunk?

Thank you for the patch! The idea you present is interesting, but it
makes tzset() expensive when run in a loop, and several interfaces
call tzset() unconditionally to update the timezone in the event the
API call is the first in the sequence. For example mktime must call
tzset, and previously this might have been fast, but now it re-reads
the file every time. I don't think this is an acceptable solution
to the problem, but without a microbenchmark we don't know. So one
way to make your point would be to contribute a microbenchmark for
tzset or mktime, and show that the performance difference is
negligible. The other alternative is to go a layer down and stat
/etc/localtime (if that file is going to be used), and therefore
provide a quick way to avoid reloading the file if it hasn't changed.
All of this also will need a test case, which may or may not be
possible e.g. test starts in one timezone, updates the file used
(via TZ to full path of timezone), and then calls tzset, and verifies
that timezone has changed.

Lastly, this patch is a non-trivial number of lines and the copyright
status of ZTE is not yet clear. I will continue the copyright
discussion off-list with you directly.

Thank you again for your contribution.

--
Cheers,
Carlos.

Re: [PATCH] tzset did not catch changes to localtime [BZ #21060 ]

[Andreas Schwab]

On Dez 17 2018, <ma.jiang@zte.com.cn> wrote:

> +  /* Check whether the tz value changed since the last run.  */
> +  int tz_changed = old_tz ? strcmp (tz, old_tz) : 1;

This should be written as old_tz == NULL || strcmp (tz, old_tz) != 0,
and tz_changed should be bool.

> +  if ((old_tz == NULL)
> +      || (old_tz != NULL && tz_changed))

This is the same as tz_changed.

> +  {
> +    /* Save the value of `tz'. */
> +    free (old_tz);
> +    old_tz = __strdup (tz);
> +  }

Wrong indentation.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re:[PATCH] tzset did not catch changes tolocaltime [BZ #21060 ]

[ma.jiang]

[-- Attachment #1.1: Type: text/plain, Size: 2307 bytes --]

Hi schwab,
  Thanks for the quick reply.
  I have changed the patch into the following one, as you suggested.   Is it OK now?

diff --git a/time/tzset.c b/time/tzset.c
index b517867..1431097 100644
--- a/time/tzset.c
+++ b/time/tzset.c
@@ -386,22 +386,27 @@ tzset_internal (int always)
   if (tz && *tz == ':')
     ++tz;

-  /* Check whether the value changed since the last run.  */
-  if (old_tz != NULL && tz != NULL && strcmp (tz, old_tz) == 0)
-    /* No change, simply return.  */
-    return;
-
   if (tz == NULL)
     /* No user specification; use the site-wide default.  */
     tz = TZDEFAULT;

+  /* Check whether the tz value changed since the last run.  */
+  bool tz_changed = (old_tz == NULL) || (strcmp (tz, old_tz) != 0);
+  if (old_tz == NULL || tz_changed)
+    {
+      /* Save the value of `tz'. */
+      free (old_tz);
+      old_tz = __strdup (tz);
+    }
+  
+  /* When using tzfile, must check whether it changed in  __tzfile_read.  */
+  if (!__use_tzfile && !tz_changed)
+    return;
+
+  /* Going to recaculate values. */
   tz_rules[0].name = NULL;
   tz_rules[1].name = NULL;

-  /* Save the value of `tz'.  */
-  free (old_tz);
-  old_tz = tz ? __strdup (tz) : NULL;
-
   /* Try to read a data file.  */
   __tzfile_read (tz, 0, NULL);
   if (__use_tzfile)




------------------原始邮件------------------
发件人：AndreasSchwab <schwab@suse.de>
收件人：马江10100629;
抄送人：carlos@redhat.com <carlos@redhat.com>;libc-alpha@sourceware.org <libc-alpha@sourceware.org>;
日 期 ：2018年12月17日 17:40
主 题 ：Re: [PATCH]   tzset did not catch changes tolocaltime  [BZ #21060 ]
On Dez 17 2018, <ma.jiang@zte.com.cn> wrote:

> +  /* Check whether the tz value changed since the last run.  */
> +  int tz_changed = old_tz ? strcmp (tz, old_tz) : 1;

This should be written as old_tz == NULL || strcmp (tz, old_tz) != 0,
and tz_changed should be bool.

> +  if ((old_tz == NULL)
> +      || (old_tz != NULL && tz_changed))

This is the same as tz_changed.

> +  {
> +    /* Save the value of `tz'. */
> +    free (old_tz);
> +    old_tz = __strdup (tz);
> +  }

Wrong indentation.

Andreas.

--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] tzset did not catch changes tolocaltime [BZ #21060 ]

[Paul Eggert]

I followed up in the bug report, here:

https://sourceware.org/bugzilla/show_bug.cgi?id=21060#c1

Re: [PATCH] tzset did not catch changes tolocaltime [BZ #21060 ]

[Carlos O'Donell]

On 12/17/18 5:23 PM, Paul Eggert wrote:
> I followed up in the bug report, here:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=21060#c1
> 

In the case of reloading if TZ or /etc/localtime has changed we are in
the same realm of problem as /etc/resolv.conf reloading, it's something
we would like to be able to do, but you're going to need more cleverness
to avoid the costly operations, otherwise it's lost performance for
little to no gain. Most developers don't change /etc/localtime, and in
many countries it is a stable set of daylight savings transitions (and
if they change you need to restart).

The naive implementation is not going to be sufficient.

-- 
Cheers,
Carlos.

Re:[PATCH] tzset did not catch changes tolocaltime [BZ #21060 ]

[ma.jiang]

[-- Attachment #1.1: Type: text/plain, Size: 2305 bytes --]

I agree that we should  make more effort to avoid the costly file system operations(as the test shows a 10x slowdown).  
It's certainly not wise to pay a 10x slowdown to detect  changes that probably never happen.  
But it seems very difficult to keep tzset things run fast and correctly... 
A simple solution might be to add a new interface that does not reload tzdata.

By the way, there is another small problem. When TZ is not set, tzset will get into 
__tzfile_read everytime, and become very slow. If we do not care tzdata changes, 
and want a fast tzset, we could do a simple swap as following.

diff --git a/time/tzset.c b/time/tzset.c
index b517867..94ae747 100644
--- a/time/tzset.c
+++ b/time/tzset.c
@@ -386,15 +386,15 @@ tzset_internal (int always)
   if (tz && *tz == ':')
     ++tz;

+  if (tz == NULL)
+    /* No user specification; use the site-wide default.  */
+    tz = TZDEFAULT;
+
   /* Check whether the value changed since the last run.  */
   if (old_tz != NULL && tz != NULL && strcmp (tz, old_tz) == 0)
     /* No change, simply return.  */
     return;

-  if (tz == NULL)
-    /* No user specification; use the site-wide default.  */
-    tz = TZDEFAULT;
-
   tz_rules[0].name = NULL;
   tz_rules[1].name = NULL;









------------------原始邮件------------------
发件人：CarlosO'Donell <carlos@redhat.com>
收件人：Paul Eggert <eggert@cs.ucla.edu>;马江10100629;schwab@suse.de <schwab@suse.de>;
抄送人：libc-alpha@sourceware.org <libc-alpha@sourceware.org>;
日 期 ：2018年12月18日 12:24
主 题 ：Re: [PATCH]   tzset did not catch changes tolocaltime  [BZ #21060 ]
On 12/17/18 5:23 PM, Paul Eggert wrote:
> I followed up in the bug report, here:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=21060#c1
>

In the case of reloading if TZ or /etc/localtime has changed we are in
the same realm of problem as /etc/resolv.conf reloading, it's something
we would like to be able to do, but you're going to need more cleverness
to avoid the costly operations, otherwise it's lost performance for
little to no gain. Most developers don't change /etc/localtime, and in
many countries it is a stable set of daylight savings transitions (and
if they change you need to restart).

The naive implementation is not going to be sufficient.

--
Cheers,
Carlos.

Re: [PATCH] tzset did not catch changes tolocaltime [BZ #21060 ]

[Paul Eggert]

On 12/17/18 11:12 PM, ma.jiang@zte.com.cn wrote:
> By the way, there is another small problem. When TZ is not set, tzset will get into
> __tzfile_read everytime, and become very slow.

I noticed the same thing, along with a related problem where TZ=":" is 
treated differently from TZ="". I filed a bug report (along with a 
proposed patch) here:

https://sourceware.org/bugzilla/show_bug.cgi?id=24004