Integrated: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 06:45:26 GMT, Wu Yan wrote: > Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan This pull request has now been integrated. Changeset: 88bbf3c2 Author:Wu Yan Committer: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/88bbf3c2e6ac9f6d88cbb361cfbb4c16bb8eafc1 Stats: 352 lines in 4 files changed: 203 ins; 145 del; 4 mod 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux Co-authored-by: Sun Jianye Reviewed-by: naoto, mli - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 13:39:46 GMT, Naoto Sato wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > The change looks reasonable. Please test your fix with macOS as well. Hi, @naotoj, I pushed the new commit, do you need to review again? - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v4]
On Fri, 15 Oct 2021 07:03:18 GMT, Hamlin Li wrote:
>> Wu Yan has updated the pull request with a new target base due to a merge or
>> a rebase. The incremental webrev excludes the unrelated changes brought in
>> by the merge/rebase. The pull request contains four additional commits since
>> the last revision:
>>
>> - Merge branch 'master' into timezone
>> - change functions to be static
>> - replace realpath
>> - 8273111: Default timezone should return zone ID if /etc/localtime is
>> valid but not canonicalization on linux
>
> src/java.base/unix/native/libjava/TimeZone_md.c line 82:
>
>> 80:
>> 81: /*
>> 82: * remove repeated path separators ('/') in the giving 'path'.
>
> given?
Thanks, fix it.
> src/java.base/unix/native/libjava/TimeZone_md.c line 89:
>
>> 87: char *left = path;
>> 88: char *right = path;
>> 89: char *end = path + strlen(path);
>
> "char* end"? better to align with existing code style
OK, fixed it.
> src/java.base/unix/native/libjava/TimeZone_md.c line 95:
>
>> 93: for (; right < end; right++) {
>> 94: if (*right == '/' && *(right + 1) == '/') break;
>> 95: }
>
> Is this for loop necessary? Seems it's ok to merge with the nested loop below.
Thanks for your suggestion, this for loop is indeed unnecessary. Removed it.
-
PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v5]
> Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan Wu Yan has updated the pull request incrementally with one additional commit since the last revision: fix code style - Changes: - all: https://git.openjdk.java.net/jdk/pull/5327/files - new: https://git.openjdk.java.net/jdk/pull/5327/files/93cff3d1..ba9cc656 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5327&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5327&range=03-04 Stats: 9 lines in 1 file changed: 0 ins; 6 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327 PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v4]
> Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan Wu Yan has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'master' into timezone - change functions to be static - replace realpath - 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux - Changes: - all: https://git.openjdk.java.net/jdk/pull/5327/files - new: https://git.openjdk.java.net/jdk/pull/5327/files/38177cd0..93cff3d1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5327&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5327&range=02-03 Stats: 1449026 lines in 13982 files changed: 735454 ins; 659127 del; 54445 mod Patch: https://git.openjdk.java.net/jdk/pull/5327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327 PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v3]
> Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan Wu Yan has updated the pull request incrementally with one additional commit since the last revision: change functions to be static - Changes: - all: https://git.openjdk.java.net/jdk/pull/5327/files - new: https://git.openjdk.java.net/jdk/pull/5327/files/19cc634d..38177cd0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5327&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5327&range=01-02 Stats: 6 lines in 2 files changed: 0 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327 PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v2]
On Mon, 11 Oct 2021 18:18:02 GMT, Naoto Sato wrote: >> Wu Yan has updated the pull request incrementally with one additional commit >> since the last revision: >> >> replace realpath > > src/java.base/unix/native/libjava/path_util.h line 31: > >> 29: int collapsible(char *names); >> 30: void splitNames(char *names, char **ix); >> 31: void joinNames(char *names, int nc, char **ix); > > Are these functions, `collapsible`, `splitNames` and `joinNames` have to be > non-static? You are right, thanks for your suggestions, I'll change these functions to static in next commit. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v2]
On Mon, 11 Oct 2021 18:16:28 GMT, Naoto Sato wrote: >> Wu Yan has updated the pull request incrementally with one additional commit >> since the last revision: >> >> replace realpath > > src/java.base/unix/native/libjava/TimeZone_md.c line 113: > >> 111: } >> 112: } >> 113: > > There are a few `*(right + 1)` references in the loops. Is there any > possibility that it would run over the buffer? It wouldn't run over the buffer. Here `end` points to `'\0'`. At line 94 and line 99, `right` is less than `end`, so `right + 1` is at most `end`. At line 103, if `right` equals `end`, `*right != '\0'` will be false and `!(*right == '/' && *(right + 1) == '/')` will not executed. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 13:39:46 GMT, Naoto Sato wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > The change looks reasonable. Please test your fix with macOS as well. Hi, @naotoj, Could you do me a favor to review the patch? - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v6]
On Sun, 5 Sep 2021 13:23:21 GMT, Andrew Haley wrote: >> Thanks, I'll fix it. > > It's fine. I don't think it'll affect any real programs, so it's rather > pointless. I don't know if that's any reason not to approve it. Andrew, can you help us to approve this? - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v2]
On Thu, 9 Sep 2021 08:25:44 GMT, Wu Yan wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > Wu Yan has updated the pull request incrementally with one additional commit > since the last revision: > > replace realpath Hi, we directly process the characters in `linkbuf` instead of realpath. And we reuse [collapse()](https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/libjava/canonicalize_md.c#L129) to process the '.' and '..' in `linkbuf`. We tested some cases about /etc/localtime, and it works well. Test cases and results are in the attachment. [testcase.log](https://github.com/openjdk/jdk/files/7135028/testcase.log) [result.log](https://github.com/openjdk/jdk/files/7135030/result.log) - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v2]
> Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan Wu Yan has updated the pull request incrementally with one additional commit since the last revision: replace realpath - Changes: - all: https://git.openjdk.java.net/jdk/pull/5327/files - new: https://git.openjdk.java.net/jdk/pull/5327/files/9b29edfc..19cc634d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5327&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5327&range=00-01 Stats: 395 lines in 4 files changed: 241 ins; 145 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/5327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327 PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v7]
On Tue, 7 Sep 2021 01:38:02 GMT, Nick Gasson wrote: > Please check the Windows tier1 failure: > https://github.com/Wanghuang-Huawei/jdk/runs/3459332995 > > Seems unlikely that it's anything to do with this patch so you may just want > to re-run it or merge from master. OK, The rerun of presubmit test show that it passed all tests. The result is here: https://github.com/Wanghuang-Huawei/jdk/actions/runs/1181122290 - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v4]
On Wed, 28 Jul 2021 08:51:38 GMT, Andrew Haley wrote: >> I don't think we want to keep two copies of the compareTo intrinsic. If >> there are no cases where the LDP version is worse than the original version >> then we should just delete the old one and replace it with this. > >> I don't think we want to keep two copies of the compareTo intrinsic. If >> there are no cases where the LDP version is worse than the original version >> then we should just delete the old one and replace it with this. > > I agree. The trouble is, what does "worse" mean? I'm looking at SDEN-1982442, > Neoverse N2 errata, 2001293, and I see that LDP has to be slowed down on > streaming workloads, which will affect this. (That's just an example: I'm > making the point that implementations differ.) > > The trouble with this patch is that it (probably) makes things better for > long strings, which are very rare. What we actually need to care about is > performance for a large number of typical-sized strings, which are names, > identifiers, passwords, and so on: about 10-30 characters. @theRealAph do you have any other questions about this patch? - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 12:38:38 GMT, Alan Bateman wrote: > I haven't come across this configuration like but changing it to use realpath > seem reasonable. Thanks, this scenario comes from our customers. > Using `realpath` instead of `readlink` will change results on systems which > use symbolic links instead of hard links to de-duplicate the timezone files. > For example, on Debian, if `UTC` is configured (`/etc/localtime` points to > `/usr/share/zoneinfo/UTC`), I think the result will be `Etc/UTC` instead of > `UTC` after this change. But I have not actually tested this. We tested it on ubuntu, it does have this kind of problem, thank you for your reminder. `realpath` doesn't seem to be suitable here, maybe we could directly process the previous `linkbuf` to remove the extra'.' and'/'. > The change looks reasonable. Please test your fix with macOS as well. Thanks, the test on macOS is passed. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 12:51:15 GMT, Florian Weimer wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > src/java.base/unix/native/libjava/TimeZone_md.c line 292: > >> 290: /* canonicalize the path */ >> 291: char resolvedpath[PATH_MAX + 1]; >> 292: char *path = realpath(DEFAULT_ZONEINFO_FILE, resolvedpath); > > You really should use `realpath` with `NULL` as the second argument, to avoid > any risk of buffer overflow. Future C library headers may warn about non-null > arguments here. Ok, Thanks. I will refer to the implementation of `os::Posix::realpath` to fix it if we still use `realpath`. https://github.com/openjdk/jdk/blob/5245c1cf0260a78ca5f8ab4e7d13107f21faf071/src/hotspot/os/posix/os_posix.cpp#L805 - PR: https://git.openjdk.java.net/jdk/pull/5327
RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
Hi, Please help me review the change to enhance getting time zone ID from /etc/localtime on linux. We use `realpath` instead of `readlink` to obtain the link name of /etc/localtime, because `readlink` can only read the value of a symbolic of link, not the canonicalized absolute pathname. For example, the value of /etc/localtime is "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. Thanks, wuyan - Commit messages: - 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux Changes: https://git.openjdk.java.net/jdk/pull/5327/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5327&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273111 Stats: 10 lines in 1 file changed: 2 ins; 1 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/5327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327 PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v6]
On Wed, 25 Aug 2021 07:40:56 GMT, Nick Gasson wrote: > I've run the benchmark on several different machines and didn't see any > performance regressions, and the speed-up for longer strings looks quite > good. I also ran jtreg tier1-3 with no new failures so I think this is ok. > > If you fix the Windows build I'll approve it. But please wait for another > review, preferably from @theRealAph. OK, Thank you very much! > Note that JDK-8269559 (#5129) is also adding a JMH benchmark for this > intrinsic: it would be good if we could merge them, either now or later. The JMH benchmark added by JDK-8269559 (#5129) can cover our test items (compareToLL and compareToUU), and can show the improvement of our patch, so we decided to delete our JMH benchmark in the next commit. The test results using that JMH benchmark in JDK-8269559 are as follows: Raspberry Pi 4B base: Benchmark (delta) (size) Mode Cnt Score Error Units StringCompareToDifferentLength.compareToLL2 24 avgt3 2.310 ? 0.050 ms/op StringCompareToDifferentLength.compareToLL2 36 avgt3 2.818 ? 0.185 ms/op StringCompareToDifferentLength.compareToLL2 72 avgt3 3.151 ? 0.215 ms/op StringCompareToDifferentLength.compareToLL2 128 avgt3 4.171 ? 1.320 ms/op StringCompareToDifferentLength.compareToLL2 256 avgt3 6.169 ? 0.653 ms/op StringCompareToDifferentLength.compareToLL2 512 avgt3 10.911 ? 0.175 ms/op StringCompareToDifferentLength.compareToLU2 24 avgt3 3.312 ? 0.102 ms/op StringCompareToDifferentLength.compareToLU2 36 avgt3 4.162 ? 0.032 ms/op StringCompareToDifferentLength.compareToLU2 72 avgt3 5.705 ? 0.152 ms/op StringCompareToDifferentLength.compareToLU2 128 avgt3 9.301 ? 0.749 ms/op StringCompareToDifferentLength.compareToLU2 256 avgt3 16.507 ? 1.353 ms/op StringCompareToDifferentLength.compareToLU2 512 avgt3 30.160 ? 0.377 ms/op StringCompareToDifferentLength.compareToUL2 24 avgt3 3.366 ? 0.280 ms/op StringCompareToDifferentLength.compareToUL2 36 avgt3 4.308 ? 0.037 ms/op StringCompareToDifferentLength.compareToUL2 72 avgt3 5.674 ? 0.210 ms/op StringCompareToDifferentLength.compareToUL2 128 avgt3 9.358 ? 0.158 ms/op StringCompareToDifferentLength.compareToUL2 256 avgt3 16.165 ? 0.158 ms/op StringCompareToDifferentLength.compareToUL2 512 avgt3 29.857 ? 0.277 ms/op StringCompareToDifferentLength.compareToUU2 24 avgt3 3.149 ? 0.209 ms/op StringCompareToDifferentLength.compareToUU2 36 avgt3 3.157 ? 0.102 ms/op StringCompareToDifferentLength.compareToUU2 72 avgt3 4.415 ? 0.073 ms/op StringCompareToDifferentLength.compareToUU2 128 avgt3 6.244 ? 0.224 ms/op StringCompareToDifferentLength.compareToUU2 256 avgt3 11.032 ? 0.080 ms/op StringCompareToDifferentLength.compareToUU2 512 avgt3 20.942 ? 3.973 ms/op opt： Benchmark (delta) (size) Mode Cnt Score Error Units StringCompareToDifferentLength.compareToLL2 24 avgt3 2.319 ? 0.121 ms/op StringCompareToDifferentLength.compareToLL2 36 avgt3 2.820 ? 0.096 ms/op StringCompareToDifferentLength.compareToLL2 72 avgt3 2.511 ? 0.024 ms/op StringCompareToDifferentLength.compareToLL2 128 avgt3 3.496 ? 0.382 ms/op StringCompareToDifferentLength.compareToLL2 256 avgt3 5.215 ? 0.210 ms/op StringCompareToDifferentLength.compareToLL2 512 avgt3 7.772 ? 0.448 ms/op StringCompareToDifferentLength.compareToLU2 24 avgt3 3.432 ? 0.249 ms/op StringCompareToDifferentLength.compareToLU2 36 avgt3 4.156 ? 0.052 ms/op StringCompareToDifferentLength.compareToLU2 72 avgt3 5.735 ? 0.043 ms/op StringCompareToDifferentLength.compareToLU2 128 avgt3 9.215 ? 0.394 ms/op StringCompareToDifferentLength.compareToLU2 256 avgt3 16.373 ? 0.515 ms/op StringCompareToDifferentLength.compareToLU2 512 avgt3 29.906 ? 0.245 ms/op StringCompareToDifferentLength.compareToUL2 24 avgt3 3.361 ? 0.116 ms/op StringCompareToDifferentLength.compareToUL2 36 avgt3 4.253 ? 0.061 ms/op StringCompareToDifferentLength.compareToUL2 72 avgt3 5.744 ? 0.082 ms/op StringCompareToDifferentLength.compareToUL2 128 avgt3 9.167 ? 0.343 ms/op StringCompareToDifferentLength.compareToUL2 256 avgt3 16.591 ? 0.999 ms/op StringCompareToDifferen
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v4]
On Wed, 28 Jul 2021 08:51:38 GMT, Andrew Haley wrote: >> I don't think we want to keep two copies of the compareTo intrinsic. If >> there are no cases where the LDP version is worse than the original version >> then we should just delete the old one and replace it with this. > >> I don't think we want to keep two copies of the compareTo intrinsic. If >> there are no cases where the LDP version is worse than the original version >> then we should just delete the old one and replace it with this. > > I agree. The trouble is, what does "worse" mean? I'm looking at SDEN-1982442, > Neoverse N2 errata, 2001293, and I see that LDP has to be slowed down on > streaming workloads, which will affect this. (That's just an example: I'm > making the point that implementations differ.) > > The trouble with this patch is that it (probably) makes things better for > long strings, which are very rare. What we actually need to care about is > performance for a large number of typical-sized strings, which are names, > identifiers, passwords, and so on: about 10-30 characters. Hi, @theRealAph @nick-arm, The test data looks OK on Raspberry Pi 4B and Hisilicon, do you have any other questions about this patch? - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v5]
On Tue, 3 Aug 2021 13:33:07 GMT, Wang Huang wrote: >> Dear all, >> Can you do me a favor to review this patch. This patch use `ldp` to >> implement String.compareTo. >> >> * We add a JMH test case >> * Here is the result of this test case >> >> Benchmark |(size)| Mode| Cnt|Score | Error |Units >> -|--|-||--||- >> StringCompare.compareLL | 64 | avgt| 5 |7.992 | ± 0.005|us/op >> StringCompare.compareLL | 72 | avgt| 5 |15.029| ± 0.006|us/op >> StringCompare.compareLL | 80 | avgt| 5 |14.655| ± 0.011|us/op >> StringCompare.compareLL | 91 | avgt| 5 |16.363| ± 0.12 |us/op >> StringCompare.compareLL | 101 | avgt| 5 |16.966| ± 0.007|us/op >> StringCompare.compareLL | 121 | avgt| 5 |19.276| ± 0.006|us/op >> StringCompare.compareLL | 181 | avgt| 5 |19.002| ± 0.417|us/op >> StringCompare.compareLL | 256 | avgt| 5 |24.707| ± 0.041|us/op >> StringCompare.compareLLWithLdp| 64 | avgt| 5 |8.001 | ± >> 0.121|us/op >> StringCompare.compareLLWithLdp| 72 | avgt| 5 |11.573| ± 0.003|us/op >> StringCompare.compareLLWithLdp| 80 | avgt| 5 |6.861 | ± 0.004|us/op >> StringCompare.compareLLWithLdp| 91 | avgt| 5 |12.774| ± 0.201|us/op >> StringCompare.compareLLWithLdp| 101 | avgt| 5 |8.691 | ± 0.004|us/op >> StringCompare.compareLLWithLdp| 121 | avgt| 5 |11.091| ± 1.342|us/op >> StringCompare.compareLLWithLdp| 181 | avgt| 5 |14.64 | ± 0.581|us/op >> StringCompare.compareLLWithLdp| 256 | avgt| 5 |25.879| ± 1.775|us/op >> StringCompare.compareUU | 64 | avgt| 5 |13.476| ± 0.01 |us/op >> StringCompare.compareUU | 72 | avgt| 5 |15.078| ± 0.006|us/op >> StringCompare.compareUU | 80 | avgt| 5 |23.512| ± 0.011|us/op >> StringCompare.compareUU | 91 | avgt| 5 |24.284| ± 0.008|us/op >> StringCompare.compareUU | 101 | avgt| 5 |20.707| ± 0.017|us/op >> StringCompare.compareUU | 121 | avgt| 5 |29.302| ± 0.011|us/op >> StringCompare.compareUU | 181 | avgt| 5 |39.31 | ± >> 0.016|us/op >> StringCompare.compareUU | 256 | avgt| 5 |54.592| ± 0.392|us/op >> StringCompare.compareUUWithLdp| 64 | avgt| 5 |16.389| ± 0.008|us/op >> StringCompare.compareUUWithLdp| 72 | avgt| 5 |10.71 | ± 0.158|us/op >> StringCompare.compareUUWithLdp| 80 | avgt| 5 |11.488| ± 0.024|us/op >> StringCompare.compareUUWithLdp| 91 | avgt| 5 |13.412| ± 0.006|us/op >> StringCompare.compareUUWithLdp| 101 | avgt| 5 |16.245| ± 0.434|us/op >> StringCompare.compareUUWithLdp| 121 | avgt| 5 |16.597| ± 0.016|us/op >> StringCompare.compareUUWithLdp| 181 | avgt| 5 |27.373| ± 0.017|us/op >> StringCompare.compareUUWithLdp| 256 | avgt| 5 |41.74 | ± 3.5 |us/op >> >> From this table, we can see that in most cases, our patch is better than old >> one. >> >> Thank you for your review. Any suggestions are welcome. > > Wang Huang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - fix bugs > - Merge branch 'master' of https://gitee.com/ustc-wh/jdk into JDK-8268231 > - fix style and add unalign test case > - refact codes > - draft of refactor > - 8268231: Aarch64: Use ldp in intrinsics for String.compareTo We also tested this version on Hisilicon, increasing the count of each test and the length of the string because the test data fluctuates more. When diff_pos is greater than 255, the improvement will be more obvious. And in all cases there was no significant decline. base: Benchmark (diff_pos) (size) Mode Cnt Score Error Units StringCompare.compareLLDiffStrings 7 512 avgt 50 5.481 ? 1.230 us/op StringCompare.compareLLDiffStrings31 512 avgt 50 6.944 ? 0.962 us/op StringCompare.compareLLDiffStrings63 512 avgt 50 10.129 ? 0.973 us/op StringCompare.compareLLDiffStrings 127 512 avgt 50 15.944 ? 0.786 us/op StringCompare.compareLLDiffStrings 255 512 avgt 50 28.233 ? 0.737 us/op StringCompare.compareLLDiffStrings 511 512 avgt 50 51.612 ? 1.357 us/op StringCompare.compareUUDiffStrings 7 512 avgt 50 5.552 ? 0.809 us/op StringCompare.compareUUDiffStrings31 512 avgt 50 12.024 ? 1.499 us/op StringCompare.compareUUDiffStrings63 512 avgt 50 15.368 ? 0.009 us/op StringCompare.compareUUDiffStrings 127 512 avgt 50 28.354 ? 0.655 us/op StringCompare.compareUUDiffStrings 255 512 avgt 50 52.9
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v5]
On Wed, 4 Aug 2021 03:27:49 GMT, Nick Gasson wrote: >> Wang Huang has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains six additional >> commits since the last revision: >> >> - fix bugs >> - Merge branch 'master' of https://gitee.com/ustc-wh/jdk into JDK-8268231 >> - fix style and add unalign test case >> - refact codes >> - draft of refactor >> - 8268231: Aarch64: Use ldp in intrinsics for String.compareTo > > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 4892: > >> 4890: __ cmp(tmp1, tmp2); >> 4891: __ ccmp(tmp1h, tmp2h, 0, Assembler::EQ); >> 4892: __ br(__ NE, DIFF); > > The line above uses `Assembler::EQ` for the condition code but this line uses > `__ NE`. Better to be consistent and use `Assembler::` everywhere. Thanks, I'll fix it. - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v5]
On Wed, 4 Aug 2021 03:29:40 GMT, Nick Gasson wrote: > Please provide the updated benchmark results for this version. Are you able > to test it on several different machines? We tested this version on Raspberry Pi 4B. base: Benchmark (diff_pos) (size) Mode Cnt ScoreError Units StringCompare.compareLLDiffStrings 7 256 avgt5 14.882 ? 0.157 us/op StringCompare.compareLLDiffStrings15 256 avgt5 15.514 ? 0.094 us/op StringCompare.compareLLDiffStrings31 256 avgt5 16.756 ? 0.050 us/op StringCompare.compareLLDiffStrings47 256 avgt5 18.196 ? 0.727 us/op StringCompare.compareLLDiffStrings63 256 avgt5 20.110 ? 0.075 us/op StringCompare.compareLLDiffStrings 127 256 avgt5 31.458 ? 0.032 us/op StringCompare.compareLLDiffStrings 255 256 avgt5 53.099 ? 1.212 us/op StringCompare.compareUUDiffStrings 7 256 avgt5 15.419 ? 0.012 us/op StringCompare.compareUUDiffStrings15 256 avgt5 16.761 ? 0.078 us/op StringCompare.compareUUDiffStrings31 256 avgt5 20.132 ? 0.112 us/op StringCompare.compareUUDiffStrings47 256 avgt5 27.492 ? 0.104 us/op StringCompare.compareUUDiffStrings63 256 avgt5 32.147 ? 0.028 us/op StringCompare.compareUUDiffStrings 127 256 avgt5 56.208 ? 0.016 us/op StringCompare.compareUUDiffStrings 255 256 avgt5 100.439 ? 0.782 us/op StringCompare.compareUUDiffStringsTurnOffCCP 7 256 avgt5 15.441 ? 0.071 us/op StringCompare.compareUUDiffStringsTurnOffCCP 15 256 avgt5 16.781 ? 0.192 us/op StringCompare.compareUUDiffStringsTurnOffCCP 31 256 avgt5 20.109 ? 0.010 us/op StringCompare.compareUUDiffStringsTurnOffCCP 47 256 avgt5 27.463 ? 0.068 us/op StringCompare.compareUUDiffStringsTurnOffCCP 63 256 avgt5 32.168 ? 0.064 us/op StringCompare.compareUUDiffStringsTurnOffCCP 127 256 avgt5 56.283 ? 0.551 us/op StringCompare.compareUUDiffStringsTurnOffCCP 255 256 avgt5 100.419 ? 0.914 us/op opt: Benchmark (diff_pos) (size) Mode Cnt Score Error Units StringCompare.compareLLDiffStrings 7 256 avgt5 14.064 ? 0.048 us/op StringCompare.compareLLDiffStrings15 256 avgt5 16.079 ? 0.041 us/op StringCompare.compareLLDiffStrings31 256 avgt5 17.413 ? 0.033 us/op StringCompare.compareLLDiffStrings47 256 avgt5 18.750 ? 0.012 us/op StringCompare.compareLLDiffStrings63 256 avgt5 20.093 ? 0.052 us/op StringCompare.compareLLDiffStrings 127 256 avgt5 27.432 ? 0.009 us/op StringCompare.compareLLDiffStrings 255 256 avgt5 44.832 ? 0.173 us/op StringCompare.compareUUDiffStrings 7 256 avgt5 16.071 ? 0.028 us/op StringCompare.compareUUDiffStrings15 256 avgt5 18.082 ? 0.015 us/op StringCompare.compareUUDiffStrings31 256 avgt5 20.753 ? 0.006 us/op StringCompare.compareUUDiffStrings47 256 avgt5 25.427 ? 0.051 us/op StringCompare.compareUUDiffStrings63 256 avgt5 28.170 ? 0.091 us/op StringCompare.compareUUDiffStrings 127 256 avgt5 42.809 ? 0.143 us/op StringCompare.compareUUDiffStrings 255 256 avgt5 75.056 ? 0.741 us/op StringCompare.compareUUDiffStringsTurnOffCCP 7 256 avgt5 16.132 ? 0.195 us/op StringCompare.compareUUDiffStringsTurnOffCCP 15 256 avgt5 17.423 ? 0.023 us/op StringCompare.compareUUDiffStringsTurnOffCCP 31 256 avgt5 20.102 ? 0.112 us/op StringCompare.compareUUDiffStringsTurnOffCCP 47 256 avgt5 25.529 ? 0.367 us/op StringCompare.compareUUDiffStringsTurnOffCCP 63 256 avgt5 26.804 ? 0.051 us/op StringCompare.compareUUDiffStringsTurnOffCCP 127 256 avgt5 40.988 ? 0.425 us/op StringCompare.compareUUDiffStringsTurnOffCCP 255 256 avgt5 77.157 ? 0.187 us/op On the Raspberry Pi, the improvement is more obvious when the diff_pos is above 127. > I meant the earlier String.compareTo that this is partially replacing. This > one might be fine but I just wanted to check it had be thoroughly tested. For > reference they were: > > https://bugs.openjdk
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v4]
On Wed, 28 Jul 2021 09:55:18 GMT, Nick Gasson wrote: > Adding prefetches was one of the reasons to introduce the separate stub for > long strings, see the mail below: > > https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-April/02.html Thank you for pointing this out, we didn't find that adding prefetches was one of the reasons for that optimization before. > Did you find there's no benefit to that? In fact, at first we tested and found that adding prefetch would make it worse in some cases, so we removed prefetch in the LDP version, but after more testing, we found that prefetch is not the cause of the performance degradation. Sorry for this, please ignore the prefetch problem, I will add prefetch back next. > We don't really want to have different implementations for each > microarchitecture, that would be a testing nightmare. I aggree. This is the compromise solution that the optimization has no effect (or even slowdown) on some platforms. In addition, I found that in [JDK-8202326](https://bugs.openjdk.java.net/browse/JDK-8202326), adding prefetches is only for long strings (the rare cases), maybe we can further optimize longs string with LDP. So should I continue this optimization or close it. - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v4]
On Wed, 28 Jul 2021 08:51:38 GMT, Andrew Haley wrote: > The trouble is, what does "worse" mean? I'm looking at SDEN-1982442, Neoverse > N2 errata, 2001293, and I see that LDP has to be slowed down on streaming > workloads, which will affect this. (That's just an example: I'm making the > point that implementations differ.) We are testing on HiSilicon TSV110, maybe we can enable this optimization by default on the verified platforms. - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v2]
On Mon, 12 Jul 2021 15:36:29 GMT, Andrew Haley wrote: >> Wang Huang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> draft of refactor > > And with longer strings, M1 and ThunderX2: > > > Benchmark (diff_pos) (size) Mode Cnt > Score Error Units > StringCompare.compareLLDiffStrings10231024 avgt3 > 50.849 ± 0.087 us/op > StringCompare.compareLLDiffStringsWithLdp 10231024 avgt3 > 23.676 ± 0.015 us/op > StringCompare.compareLLDiffStringsWithRefactor10231024 avgt3 > 28.967 ± 0.168 us/op > > > StringCompare.compareLLDiffStrings10231024 avgt3 > 98.681 ± 0.026 us/op > StringCompare.compareLLDiffStringsWithLdp 10231024 avgt3 > 82.576 ± 0.656 us/op > StringCompare.compareLLDiffStringsWithRefactor10231024 avgt3 > 98.801 ± 0.321 us/op > > LDP wins on M1 here, but on ThunderX2 it makes almost no difference at all. > And how often are we comparing such long strings? > I don't know what to think, really. It seems that we're near to a place where > we're optimizing for micro-architecture, and I don't want to see that here. > On the other hand, using LDP is not worse anywhere, so we should allow it. Could you do me a favor to review the patch? @theRealAph @nick-arm Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4722
