<i18n dev> RFR: 8223490: Optimize search algorithm for determining default time zone

[Roger Riggs]

Hi Sean,

The declaration can be up in the _md.c file to satify the compiler about 
forward declarations.

$.02, Roger

On 9/13/19 4:34 AM, Seán Coffey wrote:
> Thanks for the review Roger. I run into compiler issues if I don't 
> declare the new function in the header file.
>
> /ws/jdk-jdk/open/src/java.base/unix/native/libjava/TimeZone_md.c:143:18: 
> error: implicit declaration of function ‘isFileIdentical’ 
> [-Werror=implicit-function-declaration]
>              tz = isFileIdentical(buf, size, pathname);
>
> If I try to declare it earlier in the main code,  I also run into 
> issues since this new function calls other implicitly declared functions.
>
> regards,
> Sean.
>
> On 12/09/2019 18:47, Roger Riggs wrote:
>> Hi Sean,
>>
>> In addition to Naoto's comments.
>>
>> The change to TimeZone_md.h should not be needed.
>> A 'static' declaration doesn't need to be visible outside its source 
>> file.
>>
>> Roger
>>
>>
>> On 9/12/19 12:42 PM, naoto.sato at oracle.com wrote:
>>> Hi Seán,
>>>
>>> I like your approach to provide the fast path to determine the 
>>> system time zone. One general question is, is UTC/GMT the right set 
>>> of fast path candidates? Should we add some more common ones?
>>>
>>> Other comments to the code:
>>>
>>> TimeZone_md.c
>>>
>>> - Should fast path search come after "dir" validation, i.e., line 
>>> 146-148?
>>> - Line 126: "statbuf" can be removed.
>>> - Line 134: 'i' is not size_of_something, so 'int' type should 
>>> suffice (and its initialization is done in the for-loop).
>>> - Line 138: the fast path search should "continue" with the next 
>>> name, instead of "break".
>>> - Line 142, 182: I'd wrap this line with parens for the if statement.
>>> - Line 232-242: "pathname" is an argument to this function, so 
>>> freeing it inside the function seems odd. Also, no need to reset 
>>> dbuf/fd since they are no longer reused in the loop.
>>>
>>> Naoto
>>>
>>> On 9/11/19 3:50 AM, Seán Coffey wrote:
>>>> The current algorithm for determining the default timezone on 
>>>> (non-AIX) unix systems goes something like :
>>>>
>>>> 1. If TZ environment variable is defined, use it
>>>> 2. else if /etc/timezone exists, use the value contained within it
>>>> 3. else if /etc/localtime exists and is a symbolic link, use the 
>>>> name pointed to
>>>> 4. else if /etc/localtime is a binary, find the first identical 
>>>> time zone binary file in /usr/share/zoneinfo/
>>>>
>>>> Step 4 is a bit crude in that the zoneinfo directory can contain 
>>>> over 1,800 files on today's systems. I'd like to change the logic 
>>>> so that common timezones are first checked for buffer matching 
>>>> before a full directory traversal is performed. It should be a 
>>>> performance gain and it should also lead to more consistent results 
>>>> for reasons outlined in the bug report.
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8223490
>>>> webrev: http://cr.openjdk.java.net/~coffeys/webrev.8223490/webrev/ 
>>>> <http://cr.openjdk.java.net/%7Ecoffeys/webrev.8223490/webrev/>
>>>>
>>