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

[naoto.sato at oracle.com]

Much better now. One nit: label "freedata" at line 236 should not be 
indented.

As to the fastpath candidate zones, I was just curious. UTC/GMT is fine 
by me.

Naoto

On 9/13/19 1:32 AM, Seán Coffey wrote:
> Thanks for the review Naoto. The edits certainly did need some tidying 
> up. Comments inline.
> 
> On 12/09/2019 17:42, 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?
>>
> I'm open to suggestions. I think these two are very common and good for 
> starting with.
> 
>> 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.
> All above corrected.
>> - 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.
>>
> I thought it was a useful approach given that it's the last function to 
> use 'pathname'. However, it's not in keeping with normal design I guess. 
> I've reverted and now free pathname at other call sites instead.
> 
> new webrev at http://cr.openjdk.java.net/~coffeys/webrev.8223490.v2/webrev/
> 
> regards,
> Sean.
> 
>> 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/>
>>>