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

[Roger Riggs]

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/>
>>