From patchwork Mon Dec 17 09:17:58 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: ma.jiang@zte.com.cn X-Patchwork-Id: 30690 Received: (qmail 99026 invoked by alias); 17 Dec 2018 09:18:17 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 99010 invoked by uid 89); 17 Dec 2018 09:18:16 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=BAYES_00, BODY_8BITS, GARBLED_BODY, GARBLED_SUBJECT, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS, UNPARSEABLE_RELAY autolearn=ham version=3.3.2 spammy=02, H*M:sk:2018121, bz, U*libc-alpha X-HELO: mxhk.zte.com.cn Date: Mon, 17 Dec 2018 17:17:58 +0800 (CST) X-Zmail-TransId: 2b055c1769c67c60ddef Message-ID: <201812171717582865803@zte.com.cn> Mime-Version: 1.0 From: To: Cc: Subject: =?UTF-8?B?UmU6W1BBVENIXSDCoMKgdHpzZXQgZGlkIG5vdCBjYXRjaCBjaGFuZ2VzIHRvIGxvY2FsdGltZcKgIFtCWsKgIzIxMDYwwqBd?= X-MAIL: mse02.zte.com.cn wBH9Htwh020036 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 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? ------------------原始邮件------------------ 发件人：CarlosO'Donell 收件人：马江10100629;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. --- 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)