Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v2]
> 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 incrementally with one additional commit since the last revision: draft of refactor - Changes: - all: https://git.openjdk.java.net/jdk/pull/4722/files - new: https://git.openjdk.java.net/jdk/pull/4722/files/c5e29b9f..2ae667b9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4722&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4722&range=00-01 Stats: 155 lines in 3 files changed: 153 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4722.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4722/head:pull/4722 PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo
On Fri, 9 Jul 2021 09:15:18 GMT, Andrew Haley wrote: > I'm quite tempted to approve this. It looks generally better, simpler, and > easier to understand than what we have today. However, the improvement isn't > great, and I suspect is mostly because of the reduction in traffic between > Base and Vector registers. > What happens if you rewrite `compare_string_16_bytes_same()` to use `ldp` ? I refacted `compare_string_16_bytes_same()` as a draft, the performance comparision is listed here, Benchmark |(diff_pos)|(size) | Mode | Cnt | Score| Error | Units ---|--|---|--|-|---||-- StringCompare.compareLLDiffStrings | 7| 128 | avgt | 5 | 4.252|± 0.001 | us/op StringCompare.compareLLDiffStrings |15| 128 | avgt | 5 | 4.714|± 0.001 | us/op StringCompare.compareLLDiffStrings |31| 128 | avgt | 5 | 6.139|± 0.445 | us/op StringCompare.compareLLDiffStrings |47| 128 | avgt | 5 | 13.861|± 0.001 | us/op StringCompare.compareLLDiffStrings |63| 128 | avgt | 5 | 8.823|± 0.007 | us/op StringCompare.compareLLDiffStringsWithLdp | 7| 128 | avgt | 5 | 3.867|± 0.001 | us/op StringCompare.compareLLDiffStringsWithLdp |15| 128 | avgt | 5 | 5.571|± 0.756 | us/op StringCompare.compareLLDiffStringsWithLdp |31| 128 | avgt | 5 | 5.408|± 0.001 | us/op StringCompare.compareLLDiffStringsWithLdp |47| 128 | avgt | 5 | 6.896|± 0.825 | us/op StringCompare.compareLLDiffStringsWithLdp |63| 128 | avgt | 5 | 6.787|± 0.001 | us/op StringCompare.compareLLDiffStringsWithRefactor | 7| 128 | avgt | 5 | 3.481|± 0.001 | us/op StringCompare.compareLLDiffStringsWithRefactor |15| 128 | avgt | 5 | 10.023|± 0.012 | us/op StringCompare.compareLLDiffStringsWithRefactor |31| 128 | avgt | 5 | 5.627|± 0.017 | us/op StringCompare.compareLLDiffStringsWithRefactor |47| 128 | avgt | 5 | 13.369|± 0.544 | us/op StringCompare.compareLLDiffStringsWithRefactor |63| 128 | avgt | 5 | 8.382|± 0.988 | us/op - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8266578: Disambiguate BigDecimal description of scale
On Fri, 9 Oct 2020 16:14:59 GMT, Ignasi Marimon-Clos wrote: > The API Docs for `BigDecimal` introduce the meaning of `scale`. The current > writeup can be missleading when presenting the meaning of a `scale` value > that's negative. Hopefully, with this PR, the sentence is more clear. > > The ambiguity is on this sentence: > >> If negative, the unscaled value of the number is ... > > Instead, I suggest the more verbose: > >> If the scale is negative, the unscaled value of the number is ... > > To keep symmetry, I've also reviewed the positive case from: > >> If zero or positive, the scale is the number of digits ... > > to: > >> If the scale is zero or positive, the scale is the number of digits ... Apologies on the incredible delay. I've been offline on sick leave and barely returning to work. - PR: https://git.openjdk.java.net/jdk/pull/582
[jdk17] RFR: JDK-8270075 SplittableRandom extends AbstractSplittableGenerator
Random.AbstractSplittableGenerator is an internal class that should not be exposed in a public API. - Commit messages: - Remove public exposure of AbstractSplittableGenerator Changes: https://git.openjdk.java.net/jdk17/pull/243/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17&pr=243&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270075 Stats: 48 lines in 1 file changed: 29 ins; 1 del; 18 mod Patch: https://git.openjdk.java.net/jdk17/pull/243.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/243/head:pull/243 PR: https://git.openjdk.java.net/jdk17/pull/243
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v2]
On Mon, 12 Jul 2021 09:14:25 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 incrementally with one additional > commit since the last revision: > > draft of refactor Two machines to compare here, Apple M1 and ThunderX2: Benchmark (diff_pos) (size) Mode Cnt Score Error Units StringCompare.compareLLDiffStrings 7 128 avgt3 2.194 ± 0.001 us/op StringCompare.compareLLDiffStrings 15 128 avgt3 2.195 ± 0.018 us/op StringCompare.compareLLDiffStrings 31 128 avgt3 2.508 ± 0.003 us/op StringCompare.compareLLDiffStrings 47 128 avgt3 2.821 ± 0.001 us/op StringCompare.compareLLDiffStrings 63 128 avgt3 3.446 ± 0.003 us/op StringCompare.compareLLDiffStringsWithLdp7 128 avgt3 2.194 ± 0.001 us/op StringCompare.compareLLDiffStringsWithLdp 15 128 avgt3 2.195 ± 0.001 us/op StringCompare.compareLLDiffStringsWithLdp 31 128 avgt3 2.508 ± 0.001 us/op StringCompare.compareLLDiffStringsWithLdp 47 128 avgt3 2.510 ± 0.006 us/op StringCompare.compareLLDiffStringsWithLdp 63 128 avgt3 2.824 ± 0.003 us/op StringCompare.compareLLDiffStringsWithRefactor 7 128 avgt3 1.882 ± 0.018 us/op StringCompare.compareLLDiffStringsWithRefactor 15 128 avgt3 2.019 ± 0.002 us/op StringCompare.compareLLDiffStringsWithRefactor 31 128 avgt3 2.355 ± 0.003 us/op StringCompare.compareLLDiffStringsWithRefactor 47 128 avgt3 2.821 ± 0.010 us/op StringCompare.compareLLDiffStringsWithRefactor 63 128 avgt3 3.135 ± 0.002 us/op Benchmark (diff_pos) (size) Mode Cnt Score Error Units StringCompare.compareLLDiffStrings
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v2]
On Mon, 12 Jul 2021 09:14:25 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 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. - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8260265: UTF-8 by Default
On Thu, 8 Jul 2021 21:23:00 GMT, Naoto Sato wrote: > This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 I would have expected `System.out` and `.err` to use the console encoding, but application-constructed `PrintStream`s to use UTF-8 by default. - PR: https://git.openjdk.java.net/jdk/pull/4733
Re: [jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE
On Sat, 10 Jul 2021 19:03:36 GMT, Vicente Romero wrote: > Please review this PR that is fixing a mismatch between the implementation > for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its > implementation. I made a mistake while working on a recent CSR > [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed the > API but mistakenly thought that the implementation was in sync with the spec. > This is why this change is also including a unit test of the API for > `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc` > which is covered in test `IndyDescTest` in the same test suite. Also this > change needs a CSR as while fixing the implementation of method `::withArgs` > I realized that the API of the varargs overloaded version of method `::of` > needed some rewording too as it is invoking the same private constructor > `::withArgs` is invoking. So it didn't make sense for the API of one method > to be more restrictive than the other. Please review also the accompanying > CSR. > > Thanks, > Vicente LGTM - Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/242
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v11]
On Mon, 12 Jul 2021 05:21:34 GMT, Yi Yang wrote: > I'm not familiar with the review process of core-lib group. Is it sufficient > for merging with one approval? Should I have more reviews for this? 🤔 It depends on the change. For this patch, it's good to get another reviewer to look through it. - PR: https://git.openjdk.java.net/jdk/pull/4507
Integrated: 8266578: Disambiguate BigDecimal description of scale
On Fri, 9 Oct 2020 16:14:59 GMT, Ignasi Marimon-Clos wrote: > The API Docs for `BigDecimal` introduce the meaning of `scale`. The current > writeup can be missleading when presenting the meaning of a `scale` value > that's negative. Hopefully, with this PR, the sentence is more clear. > > The ambiguity is on this sentence: > >> If negative, the unscaled value of the number is ... > > Instead, I suggest the more verbose: > >> If the scale is negative, the unscaled value of the number is ... > > To keep symmetry, I've also reviewed the positive case from: > >> If zero or positive, the scale is the number of digits ... > > to: > >> If the scale is zero or positive, the scale is the number of digits ... This pull request has now been integrated. Changeset: 1aef372e Author:Ignasi Marimon-Clos Committer: Brian Burkhalter URL: https://git.openjdk.java.net/jdk/commit/1aef372ed89a48f4eac0ac03b2b3520348713efb Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod 8266578: Disambiguate BigDecimal description of scale Reviewed-by: darcy, bpb - PR: https://git.openjdk.java.net/jdk/pull/582
Re: [jdk17] RFR: 8269281: java/foreign/Test{Down, Up}call.java time out
On Fri, 9 Jul 2021 15:01:15 GMT, Maurizio Cimadamore wrote: > After some more investigation, I have been able to at least partially > reproduce on my Linux box. While I can't get to same slowdown as we're seeing > in test machines, I did notice that in fastdebug mode, TestUpcall is a lot > slower than in release mode. > > The underlying issue is that these tests are generating a lot of "throwaway" > method handles. Method handles are typically stored in fields - when that > happens, creating a method handle with same shape as one that has already > been created typically works ok, w/o overhead, because there's a cache in the > MethodType class which stores already generated lambda forms (by method > handle kind). This cache is a SofteReference cahce, so, if the system is > under memory pressure, the GC is of course free to null out all these cached > values, in which case a new lambda form will be generated, and a new class > will be loaded. > > When looking at both TestDowncall and TestUpcall, in their current form, it > is possible to observe a fairly large amount of classes being loaded and > unloaded. Since the test generates many garbage, the GC is under pressure, > and that causes entries in the lambda form cache to be evicted. The fact that > one of the tests also explicitly calls out to `System::gc` doesn't help, and > probably adds to the churn. > > Since it is very hard, if not impossible, to fix the test so that all the > required method handle are retained for the entire duration of the test > (since we create different variations of same handles, based on the test > being run), I believe the best solution would be to reduce the number of test > combinations being executed. > > This patch adds the ability to specify a sampling factor - which reduces the > size of the test combinations being executed. Note that we also have a fuller > test (which is never run automatically, but can be ran by hand: `TestMatrix`) > - this test does not specify any sampling factor, so when developers run this > stress test manually they will still run the whole shebang (which is what you > want if you are tweaking the logic in the foreign support). LGTM - Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/238
Re: [jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE
On Sat, 10 Jul 2021 19:03:36 GMT, Vicente Romero wrote:
> Please review this PR that is fixing a mismatch between the implementation
> for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its
> implementation. I made a mistake while working on a recent CSR
> [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed the
> API but mistakenly thought that the implementation was in sync with the spec.
> This is why this change is also including a unit test of the API for
> `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc`
> which is covered in test `IndyDescTest` in the same test suite. Also this
> change needs a CSR as while fixing the implementation of method `::withArgs`
> I realized that the API of the varargs overloaded version of method `::of`
> needed some rewording too as it is invoking the same private constructor
> `::withArgs` is invoking. So it didn't make sense for the API of one method
> to be more restrictive than the other. Please review also the accompanying
> CSR.
>
> Thanks,
> Vicente
Looks good. Minor suggestions in the test.
test/jdk/java/lang/constant/DynamicCallSiteDescTest.java line 60:
> 58: "bootstrap",
> 59: ClassDesc.of("java.lang.invoke.CallSite")
> 60: ),
A minor suggestion: you can have the return `DirectMethodHandleDesc` in a local
variable to be used in all `DynamicCallSiteDesc::of` calls that would avoid
copy-n-paste of same `ConstantDescs::ofCallsiteBootstrap` call.
test/jdk/java/lang/constant/DynamicCallSiteDescTest.java line 64:
> 62: MethodTypeDesc.ofDescriptor("()I")
> 63: );
> 64: throw new AssertionError("IllegalArgumentException expected");
Suggestion:
fail("IllegalArgumentException expected");
Same for other `throw new AssertionError(...)`
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk17/pull/242
Re: [jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE
On Mon, 12 Jul 2021 17:11:01 GMT, Mandy Chung wrote:
>> Please review this PR that is fixing a mismatch between the implementation
>> for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its
>> implementation. I made a mistake while working on a recent CSR
>> [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed
>> the API but mistakenly thought that the implementation was in sync with the
>> spec. This is why this change is also including a unit test of the API for
>> `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc`
>> which is covered in test `IndyDescTest` in the same test suite. Also this
>> change needs a CSR as while fixing the implementation of method `::withArgs`
>> I realized that the API of the varargs overloaded version of method `::of`
>> needed some rewording too as it is invoking the same private constructor
>> `::withArgs` is invoking. So it didn't make sense for the API of one method
>> to be more restrictive than the other. Please review also the accompanying
>> CSR.
>>
>> Thanks,
>> Vicente
>
> test/jdk/java/lang/constant/DynamicCallSiteDescTest.java line 60:
>
>> 58: "bootstrap",
>> 59: ClassDesc.of("java.lang.invoke.CallSite")
>> 60: ),
>
> A minor suggestion: you can have the return `DirectMethodHandleDesc` in a
> local variable to be used in all `DynamicCallSiteDesc::of` calls that would
> avoid copy-n-paste of same `ConstantDescs::ofCallsiteBootstrap` call.
will do
-
PR: https://git.openjdk.java.net/jdk17/pull/242
Re: RFR: 8270056: Generated lambda class can not access protected static method of target class [v3]
On Mon, 12 Jul 2021 02:57:26 GMT, Yi Yang wrote: >> Generated lambda class can not access protected static method of the target >> class. The following exception is thrown when executing the attached >> reproducible program: >> >> >> Exception in thread "main" java.lang.IllegalAccessError: class >> AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 tried >> to access protected method 'void >> AccessProtectedStaticMethodFromSuper$A.func()' >> (AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 is in >> unnamed module of loader AccessProtectedStaticMethodFromSuper$1Loader >> @71dac704; AccessProtectedStaticMethodFromSuper$A is in unnamed module of >> loader AccessProtectedStaticMethodFromSuper$1Loader @39ed3c8d) >> at >> AccessProtectedStaticMethodFromSuper.main(AccessProtectedStaticMethodFromSuper.java:51) >> >> >> This issue is similar to JDK-8254975(#767) with slight differences: >> generated lambda proxy calls static protected method rather than protected >> member method. >> >> The proposed fix 1) tries to use MethodHandle instead of invoking forwardee >> directly(since the lambda class has no access to the resolved method) and 2) >> does not force accepting an implClass as the first argument when invoking a >> static method. >> >> Testing: >> - test/jdk/java/ with release mode >> - presubmit tests > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > mistake Looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4714
Re: [jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE [v2]
> Please review this PR that is fixing a mismatch between the implementation > for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its > implementation. I made a mistake while working on a recent CSR > [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed the > API but mistakenly thought that the implementation was in sync with the spec. > This is why this change is also including a unit test of the API for > `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc` > which is covered in test `IndyDescTest` in the same test suite. Also this > change needs a CSR as while fixing the implementation of method `::withArgs` > I realized that the API of the varargs overloaded version of method `::of` > needed some rewording too as it is invoking the same private constructor > `::withArgs` is invoking. So it didn't make sense for the API of one method > to be more restrictive than the other. Please review also the accompanying > CSR. > > Thanks, > Vicente Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk17/pull/242/files - new: https://git.openjdk.java.net/jdk17/pull/242/files/602a85c4..f7fddc99 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=242&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=242&range=00-01 Stats: 42 lines in 1 file changed: 8 ins; 20 del; 14 mod Patch: https://git.openjdk.java.net/jdk17/pull/242.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/242/head:pull/242 PR: https://git.openjdk.java.net/jdk17/pull/242
Re: RFR: 6506405: Math.abs(float) is slow [v9]
> Please consider this change to make the `float` and `double` versions of > `java.lang.Math.abs()` branch-free. Brian Burkhalter has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 6506405: More test cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/4711/files - new: https://git.openjdk.java.net/jdk/pull/4711/files/f23fa829..c061d49f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4711&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4711&range=07-08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4711.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4711/head:pull/4711 PR: https://git.openjdk.java.net/jdk/pull/4711
Re: [jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE [v3]
> Please review this PR that is fixing a mismatch between the implementation > for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its > implementation. I made a mistake while working on a recent CSR > [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed the > API but mistakenly thought that the implementation was in sync with the spec. > This is why this change is also including a unit test of the API for > `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc` > which is covered in test `IndyDescTest` in the same test suite. Also this > change needs a CSR as while fixing the implementation of method `::withArgs` > I realized that the API of the varargs overloaded version of method `::of` > needed some rewording too as it is invoking the same private constructor > `::withArgs` is invoking. So it didn't make sense for the API of one method > to be more restrictive than the other. Please review also the accompanying > CSR. > > Thanks, > Vicente Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.java.net/jdk17/pull/242/files - new: https://git.openjdk.java.net/jdk17/pull/242/files/f7fddc99..6702a42f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=242&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=242&range=01-02 Stats: 13 lines in 1 file changed: 0 ins; 3 del; 10 mod Patch: https://git.openjdk.java.net/jdk17/pull/242.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/242/head:pull/242 PR: https://git.openjdk.java.net/jdk17/pull/242
Re: [jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE [v3]
On Mon, 12 Jul 2021 18:02:54 GMT, Vicente Romero wrote: >> Please review this PR that is fixing a mismatch between the implementation >> for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its >> implementation. I made a mistake while working on a recent CSR >> [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed >> the API but mistakenly thought that the implementation was in sync with the >> spec. This is why this change is also including a unit test of the API for >> `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc` >> which is covered in test `IndyDescTest` in the same test suite. Also this >> change needs a CSR as while fixing the implementation of method `::withArgs` >> I realized that the API of the varargs overloaded version of method `::of` >> needed some rewording too as it is invoking the same private constructor >> `::withArgs` is invoking. So it didn't make sense for the API of one method >> to be more restrictive than the other. Please review also the accompanying >> CSR. >> >> Thanks, >> Vicente > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > review comments Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/242
Re: RFR: 8266936: Add a finalization JFR event
On Thu, 8 Jul 2021 19:47:26 GMT, Markus Grönlund wrote: > Greetings, > > Object.finalize() was deprecated in JDK9. There is an ongoing effort to > replace and mitigate Object.finalize() uses in the JDK libraries; please see > https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. > > We also like to assist users in replacing and mitigating uses in non-JDK code. > > Hence, this changeset adds a periodic JFR event to help identify which > classes are overriding Object.finalize(). > > Thanks > Markus This approach inspecting the loaded classes periodically as well as tracking the classes being unloaded looks good. A class with the same class name may be loaded from different class loader and could also be loaded from different code source. It'd be useful for the finalizer event to include the code source which is passed from defineClass to help identify where the class was loaded from. - PR: https://git.openjdk.java.net/jdk/pull/4731
RFR: 8211002: test/jdk/java/lang/Math/PowTests.java skips testing for non-corner-case values
Please consider this proposal to add some test coverage comparing `Math` and `StrictMath` results of `pow()`. - Commit messages: - 8211002: test/jdk/java/lang/Math/PowTests.java skips testing for non-corner-case values Changes: https://git.openjdk.java.net/jdk/pull/4758/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4758&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8211002 Stats: 18 lines in 1 file changed: 17 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4758.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4758/head:pull/4758 PR: https://git.openjdk.java.net/jdk/pull/4758
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v11]
On Thu, 8 Jul 2021 03:12:24 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has refreshed the contents of this pull request, and previous commits > have been removed. The incremental views will show differences compared to > the previous content of the PR. The pull request contains one new commit > since the last revision: > > restore JSR166; restore java.desktop Looks good. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4507
Re: [jdk17] RFR: JDK-8270075 SplittableRandom extends AbstractSplittableGenerator
On Mon, 12 Jul 2021 14:26:23 GMT, Jim Laskey wrote: > Random.AbstractSplittableGenerator is an internal class that should not be > exposed in a public API. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/243
Re: [jdk17] RFR: JDK-8270075 SplittableRandom extends AbstractSplittableGenerator
On Mon, 12 Jul 2021 14:26:23 GMT, Jim Laskey wrote: > Random.AbstractSplittableGenerator is an internal class that should not be > exposed in a public API. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/243
Re: RFR: 8270056: Generated lambda class can not access protected static method of target class [v3]
On Mon, 12 Jul 2021 02:57:26 GMT, Yi Yang wrote: >> Generated lambda class can not access protected static method of the target >> class. The following exception is thrown when executing the attached >> reproducible program: >> >> >> Exception in thread "main" java.lang.IllegalAccessError: class >> AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 tried >> to access protected method 'void >> AccessProtectedStaticMethodFromSuper$A.func()' >> (AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 is in >> unnamed module of loader AccessProtectedStaticMethodFromSuper$1Loader >> @71dac704; AccessProtectedStaticMethodFromSuper$A is in unnamed module of >> loader AccessProtectedStaticMethodFromSuper$1Loader @39ed3c8d) >> at >> AccessProtectedStaticMethodFromSuper.main(AccessProtectedStaticMethodFromSuper.java:51) >> >> >> This issue is similar to JDK-8254975(#767) with slight differences: >> generated lambda proxy calls static protected method rather than protected >> member method. >> >> The proposed fix 1) tries to use MethodHandle instead of invoking forwardee >> directly(since the lambda class has no access to the resolved method) and 2) >> does not force accepting an implClass as the first argument when invoking a >> static method. >> >> Testing: >> - test/jdk/java/ with release mode >> - presubmit tests > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > mistake Thanks Mandy for review! - PR: https://git.openjdk.java.net/jdk/pull/4714
Integrated: 8270056: Generated lambda class can not access protected static method of target class
On Thu, 8 Jul 2021 02:32:45 GMT, Yi Yang wrote: > Generated lambda class can not access protected static method of the target > class. The following exception is thrown when executing the attached > reproducible program: > > > Exception in thread "main" java.lang.IllegalAccessError: class > AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 tried to > access protected method 'void AccessProtectedStaticMethodFromSuper$A.func()' > (AccessProtectedStaticMethodFromSuper$B$$Lambda$15/0x000800b8ea48 is in > unnamed module of loader AccessProtectedStaticMethodFromSuper$1Loader > @71dac704; AccessProtectedStaticMethodFromSuper$A is in unnamed module of > loader AccessProtectedStaticMethodFromSuper$1Loader @39ed3c8d) > at > AccessProtectedStaticMethodFromSuper.main(AccessProtectedStaticMethodFromSuper.java:51) > > > This issue is similar to JDK-8254975(#767) with slight differences: generated > lambda proxy calls static protected method rather than protected member > method. > > The proposed fix 1) tries to use MethodHandle instead of invoking forwardee > directly(since the lambda class has no access to the resolved method) and 2) > does not force accepting an implClass as the first argument when invoking a > static method. > > Testing: > - test/jdk/java/ with release mode > - presubmit tests This pull request has now been integrated. Changeset: 07e90524 Author:Yi Yang URL: https://git.openjdk.java.net/jdk/commit/07e90524576f159fc16523430f1db62327c89a3b Stats: 324 lines in 3 files changed: 181 ins; 141 del; 2 mod 8270056: Generated lambda class can not access protected static method of target class Co-authored-by: NekoCaffeine Reviewed-by: mchung - PR: https://git.openjdk.java.net/jdk/pull/4714
Integrated: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base
On Wed, 16 Jun 2021 08:08:47 GMT, Yi Yang wrote:
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex
> by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in
> the whole JDK codebase.
This pull request has now been integrated.
Changeset: afe957cd
Author:Yi Yang
URL:
https://git.openjdk.java.net/jdk/commit/afe957cd9741810a113ea165a635a117c0ea556f
Stats: 357 lines in 40 files changed: 73 ins; 171 del; 113 mod
8268698: Use Objects.check{Index,FromToIndex,FromIndexSize} for java.base
Reviewed-by: mchung, rriggs
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v11]
On Thu, 8 Jul 2021 03:12:24 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has refreshed the contents of this pull request, and previous commits > have been removed. The incremental views will show differences compared to > the previous content of the PR. Thanks Mandy and Roger for reviews! - PR: https://git.openjdk.java.net/jdk/pull/4507
[jdk17] RFR: 8270056: Generated lambda class can not access protected static method of target class
Hi all, this pull request contains a backport of commit 07e90524 from the openjdk/jdk repository. The commit being backported was authored by Yi Yang on 13 Jul 2021 and was reviewed by Mandy Chung. Thanks! - Commit messages: - Backport 07e90524576f159fc16523430f1db62327c89a3b Changes: https://git.openjdk.java.net/jdk17/pull/245/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17&pr=245&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270056 Stats: 324 lines in 3 files changed: 181 ins; 141 del; 2 mod Patch: https://git.openjdk.java.net/jdk17/pull/245.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/245/head:pull/245 PR: https://git.openjdk.java.net/jdk17/pull/245
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v2]
On Mon, 12 Jul 2021 15:36:29 GMT, Andrew Haley wrote: > 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. Thank you for your suggestion. I inspect the result and find that the result of my first commit (c5e29b9fedae7e1d24056a6fae8aff04afeb3889) is better. Because of that , I will choose the version without refacting `compare_string_16_bytes_same` as the final version. - PR: https://git.openjdk.java.net/jdk/pull/4722
