<i18n dev> RFR: 8223490: Optimize search algorithm for determining default time zone
Seán Coffey
sean.coffey at oracle.com
Fri Sep 13 13:44:57 UTC 2019
Thanks Roger,
I've reverted the header file changes then and added this to the .c file:
@@ -42,6 +42,8 @@
#include "jvm.h"
#include "TimeZone_md.h"
+static char *isFileIdentical(char* buf, size_t size, char *pathname);
+
#define SKIP_SPACE(p) while (*p == ' ' || *p == '\t') p++;
#define RESTARTABLE(_cmd, _result) do { \
Regards,
Sean.
On 13/09/19 13:01, Roger Riggs wrote:
> 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/>
>>>>>
>>>
>
More information about the core-libs-dev
mailing list