Re: RFR: 8294696 - BufferedInputStream.transferTo should drain buffer when mark set [v2]
On Sun, 23 Oct 2022 08:40:35 GMT, Markus KARG wrote: > Silly me, you are certainly right! I have modified the code as you proposed, > so now we should be safe, finally. :-) The updated version (7350a533) looks safe but it makes for a more complicated transferTo so it would be good to see updated performance results. One thing to look at is dropping the allocation of the new buffer after the writing of the buffered bytes. The transferTo method will read to EOF and should be rare to continue reading after EOF - if there is reading after EOF then this should cause the internal buffer to resize. The optimization is limited to the case where BIS is not extended so there are no subclasses that can observe that the internal buf has shrunk. - PR: https://git.openjdk.org/jdk/pull/10525
Re: RFR: 8295970: Add jdk_vector tests in GHA
On Fri, 28 Oct 2022 07:21:05 GMT, Jie Fu wrote: > Good suggestion! > And the `jdk_vector_sanity` test group had been added. In general, running a few fast sanity tests from several areas in tier1 seems a good idea. Having test lists in the TEST.group files isn't very appealing as they easily get out of the sync with the tests in the tree. I realise there are already some test lists in both the hotspot and jdk TEST.groups but it feels like it needs something better so that RunTests.gmk/jtreg can select the sanity tests to run. This is not an objection to this change proposed here, just a comment that this type of configuration is annoying to maintain. - PR: https://git.openjdk.org/jdk/pull/10879
Re: RFR: 8282664: Unroll by hand StringUTF16 and StringLatin1 polynomial hash loops
On Fri, 28 Oct 2022 20:43:04 GMT, Claes Redestad wrote: > Porting to aarch64 and other platforms can be done as follow-ups and > shouldn't block integration. I'm not an expert in JVM internals, but there's an already seemingly working String.hashCode intrinsification that's ISA independent: https://github.com/openjdk/jdk/pull/6658 It operates on higher level than direct assembly instructions, i.e. it operates on the ISA-independent vector nodes, so that all hardware platforms that support vectorization would get speedup (i.e. x86-64, x86-32, arm32, arm64, etc), therefore reducing manual work to get all of them working. I wonder why that pull request got no visible interest? Forgive me if I got something wrong :) - PR: https://git.openjdk.org/jdk/pull/10847
Re: RFR: 8282664: Unroll by hand StringUTF16 and StringLatin1 polynomial hash loops
On Tue, 25 Oct 2022 10:37:40 GMT, Claes Redestad wrote: > Continuing the work initiated by @luhenry to unroll and then intrinsify > polynomial hash loops. > > I've rewired the library changes to route via a single `@IntrinsicCandidate` > method. To make this work I've harmonized how they are invoked so that > there's less special handling and checks in the intrinsic. Mainly do the > null-check outside of the intrinsic for `Arrays.hashCode` cases. > > Having a centralized entry point means it'll be easier to parameterize the > factor and start values which are now hard-coded (always 31, and a start > value of either one for `Arrays` or zero for `String`). It seems somewhat > premature to parameterize this up front. > > The current implementation is performance neutral on microbenchmarks on all > tested platforms (x64, aarch64) when not enabling the intrinsic. We do add a > few trivial method calls which increase the call stack depth, so surprises > cannot be ruled out on complex workloads. > > With the most recent fixes the x64 intrinsic results on my workstation look > like this: > > Benchmark (size) Mode Cnt ScoreError > Units > StringHashCode.Algorithm.defaultLatin1 1 avgt5 2.199 ± 0.017 > ns/op > StringHashCode.Algorithm.defaultLatin1 10 avgt5 6.933 ± 0.049 > ns/op > StringHashCode.Algorithm.defaultLatin1 100 avgt529.935 ± 0.221 > ns/op > StringHashCode.Algorithm.defaultLatin1 1 avgt5 1596.982 ± 7.020 > ns/op > > Baseline: > > Benchmark (size) Mode Cnt ScoreError > Units > StringHashCode.Algorithm.defaultLatin1 1 avgt5 2.200 ± 0.013 > ns/op > StringHashCode.Algorithm.defaultLatin1 10 avgt5 9.424 ± 0.122 > ns/op > StringHashCode.Algorithm.defaultLatin1 100 avgt590.541 ± 0.512 > ns/op > StringHashCode.Algorithm.defaultLatin1 1 avgt5 9425.321 ± 67.630 > ns/op > > I.e. no measurable overhead compared to baseline even for `size == 1`. > > The vectorized code now nominally works for all unsigned cases as well as > ints, though more testing would be good. > > Benchmark for `Arrays.hashCode`: > > Benchmark (size) Mode Cnt ScoreError Units > ArraysHashCode.bytes1 avgt5 1.884 ± 0.013 ns/op > ArraysHashCode.bytes 10 avgt5 6.955 ± 0.040 ns/op > ArraysHashCode.bytes 100 avgt587.218 ± 0.595 ns/op > ArraysHashCode.bytes1 avgt5 9419.591 ± 38.308 ns/op > ArraysHashCode.chars1 avgt5 2.200 ± 0.010 ns/op > ArraysHashCode.chars 10 avgt5 6.935 ± 0.034 ns/op > ArraysHashCode.chars 100 avgt530.216 ± 0.134 ns/op > ArraysHashCode.chars1 avgt5 1601.629 ± 6.418 ns/op > ArraysHashCode.ints 1 avgt5 2.200 ± 0.007 ns/op > ArraysHashCode.ints10 avgt5 6.936 ± 0.034 ns/op > ArraysHashCode.ints 100 avgt529.412 ± 0.268 ns/op > ArraysHashCode.ints 1 avgt5 1610.578 ± 7.785 ns/op > ArraysHashCode.shorts 1 avgt5 1.885 ± 0.012 ns/op > ArraysHashCode.shorts 10 avgt5 6.961 ± 0.034 ns/op > ArraysHashCode.shorts 100 avgt587.095 ± 0.417 ns/op > ArraysHashCode.shorts 1 avgt5 9420.617 ± 50.089 ns/op > > Baseline: > > Benchmark (size) Mode Cnt ScoreError Units > ArraysHashCode.bytes1 avgt5 3.213 ± 0.207 ns/op > ArraysHashCode.bytes 10 avgt5 8.483 ± 0.040 ns/op > ArraysHashCode.bytes 100 avgt590.315 ± 0.655 ns/op > ArraysHashCode.bytes1 avgt5 9422.094 ± 62.402 ns/op > ArraysHashCode.chars1 avgt5 3.040 ± 0.066 ns/op > ArraysHashCode.chars 10 avgt5 8.497 ± 0.074 ns/op > ArraysHashCode.chars 100 avgt590.074 ± 0.387 ns/op > ArraysHashCode.chars1 avgt5 9420.474 ± 41.619 ns/op > ArraysHashCode.ints 1 avgt5 2.827 ± 0.019 ns/op > ArraysHashCode.ints10 avgt5 7.727 ± 0.043 ns/op > ArraysHashCode.ints 100 avgt589.405 ± 0.593 ns/op > ArraysHashCode.ints 1 avgt5 9426.539 ± 51.308 ns/op > ArraysHashCode.shorts 1 avgt5 3.071 ± 0.062 ns/op > ArraysHashCode.shorts 10 avgt5 8.168 ± 0.049 ns/op > ArraysHashCode.shorts 100 avgt590.399 ± 0.292 ns/op > ArraysHashCode.shorts 1 avgt5 9420.171 ± 44.474 ns/op > > > As we can see the `Arrays` intrinsics are faster for small inputs, and faster > on large inputs for `char` and `int` (the ones currently vectorized). I aim > to fix `byte` and `short` cases before integrating, though it might be > acceptable to hand that off as follow-up enhancements to not further delay > integration of this enhancement. > > Porting to a
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Thu, 27 Oct 2022 11:24:32 GMT, Daniel Fuchs wrote: >> How about `_unused` or `_unused1`, `_unused2` then in the meantime? > > I'd be happy to make the change. Let's wait to see if anybody has a better > naming suggestion. Hello Daniel, I think calling it `unused` is fine. I did a quick search in the JDK code and we already have places where we use such a variable name for similar usecase. I don't see the need for using an underscore as a prefix, but it is OK with me if you and others prefer to use it. - PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Wed, 26 Oct 2022 17:51:31 GMT, Daniel Fuchs wrote: >> I see your point. It may be more appropriate if URI.toURL was designed as >> URL.fromURL. >> >> I was wondering to have application developers a consistent way to get an >> URL instance. Now there are two methods in different classes URI.toURL and >> URL.fromURI. It might be easier to use the old URI.toURL form. >> >> Never mind, it is just my personal preference. It is fine to me to have a >> new URL.fromURI method. > > One thing we might do is change the name of the method into `URL.of(URI, > StreamHandler)`. It's named `fromURI` merely because there was a pre-existing > package protected `fromURI` method. However since we're making it public now, > it could be fair game to change its name. Possibly adding an overload > `URL::of(URI)` method could be considered, but then there really would be two > paths to do the same thing - so I'd rather not add such an overload - unless > I get some more feedback on that from the CSR/PR review. I think `URL.of(URI, URLStreamHandler)` is fine. As for `URL.of(URI)`, whose implementation I suspect will just do `uri.toURL()` on the passed `URI`, I don't think we need it. It might add to unnecesary confusion on whether/when to use `URL.of(URI)` and when to use `URI.toURL()` In the case of `URL.of(URI, URLStreamHandler)` it's pretty clear that you use it (only) when you have a specific `URLStreamHandler` to use for the constructed `URL`. - PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8295729: Add jcheck whitespace checking for properties files [v3]
On Mon, 24 Oct 2022 19:29:41 GMT, Andy Goryachev wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Revert "Remove check for .properties from jcheck" >> >>This reverts commit c91fdaa19dc06351598bd1c0614e1af3bfa08ae2. >> - Change trailing space and tab in values to unicode encoding > > src/java.sql.rowset/share/classes/com/sun/rowset/RowSetResourceBundle.properties > line 134: > >> 132: >> 133: #SyncResolverImpl exceptions >> 134: syncrsimpl.indexval = Index value out of range\u0020\u0020 > > prob. unnecessary This case is similar the one you mentioned below. If this value is used in a string template or concatenation, the trailing white-space could be necessary; however, one space is probably enough. - PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8294696 - BufferedInputStream.transferTo should drain buffer when mark set [v5]
> This PR implements JDK-8294696. Markus KARG has updated the pull request incrementally with one additional commit since the last revision: No immediate buffer resize; in the rare case of read-past-EOF automatic buffer grow will happen anyways - Changes: - all: https://git.openjdk.org/jdk/pull/10525/files - new: https://git.openjdk.org/jdk/pull/10525/files/7350a533..9c2f502c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10525&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10525&range=03-04 Stats: 16 lines in 1 file changed: 0 ins; 15 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10525.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10525/head:pull/10525 PR: https://git.openjdk.org/jdk/pull/10525
Re: RFR: 8295729: Add jcheck whitespace checking for properties files [v3]
On Mon, 24 Oct 2022 19:21:07 GMT, Magnus Ihse Bursie wrote: >> Properties files is essentially source code. It should have the same >> whitespace checks as all other source code, so we don't get spurious >> trailing whitespace changes. >> >> With the new Skara jcheck, it is possible to increase the coverage of the >> whitespace checks (in the old mercurial version, this was more or less >> impossible). >> >> The only manual change is to `.jcheck/conf`. All other changes were made by >> running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ >> \t]*$//'`. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Revert "Remove check for .properties from jcheck" > >This reverts commit c91fdaa19dc06351598bd1c0614e1af3bfa08ae2. > - Change trailing space and tab in values to unicode encoding Trailing spaces in `LocaleNames_*` are only in two files: - `src/jdk.localedata/share/classes/sun/util/resources/ext/LocaleNames_de.properties` - `src/jdk.localedata/share/classes/sun/util/resources/ext/LocaleNames_sv.properties` It is very unlikely these spaces are part of a country or language name. The former file contains a few trailing spaces, the latter — only one. src/jdk.localedata/share/classes/sun/util/resources/ext/LocaleNames_de.properties line 238: > 236: cpp=Kreolisch-Portugiesische Sprache > 237: crh=Krimtatarisch > 238: crp=Kreolische Sprache\u0020 I'm pretty sure locale names shouldn't contain trailing spaces. - Marked as reviewed by aivanov (Reviewer). PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Fri, 28 Oct 2022 14:54:26 GMT, Daniel Fuchs wrote: >> Deprecate URL constructors. Developers are encouraged to use `java.net.URI` >> to parse or construct any URL. >> >> The `java.net.URL` class does not itself encode or decode any URL components >> according to the escaping mechanism defined in RFC2396. It is the >> responsibility of the caller to encode any fields, which need to be escaped >> prior to calling URL, and also to decode any escaped fields, that are >> returned from URL. >> >> This has lead to many issues in the past. Indeed, if used improperly, there >> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a >> URL string that can be parsed back into the same URL. This can lead to >> constructing misleading URLs. Another issue is with `equals()` and >> `hashCode()` which may have to perform a lookup, and do not take >> encoding/escaping into account. >> >> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some >> of the shortcoming of `java.net.URL`. Conversion methods to create a URL >> from a URI were also added. However, it was left up to the developers to use >> `java.net.URI`, or not. This RFE proposes to deprecate all public >> constructors of `java.net.URL`, in order to provide a stronger warning about >> their potential misuses. To construct a URL, using `URI::toURL` should be >> preferred. >> >> In order to provide an alternative to the constructors that take a stream >> handler as parameter, a new factory method `URL::fromURI(java.net.URI, >> java.net.URLStreamHandler)` is provided as part of this change. >> >> Places in the JDK code base that were constructing `java.net.URL` have been >> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related >> issues will be logged to revisit the calling code. >> >> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949 > > Daniel Fuchs 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: > > - Updated after review comments. In particular var tmp => var => _unused - > and avoid var in java.xml > - Merge branch 'master' into deprecate-url-ctor-8294241 > - Fix whitespace issues > - 8294241 src/java.base/share/classes/java/net/URL.java line 133: > 131: * specified. The optional fragment is not inherited. > 132: * > 133: * Constructing instances of > {@code URL} Would it be better to move the anchor to line 164 (the line where it says that the URL constructors are deprecated? src/java.base/share/classes/java/net/URL.java line 157: > 155: * The URL constructors are specified to throw > 156: * {@link MalformedURLException} but the actual parsing/validation > 157: * that are performed is implementation dependent. Some > parsing/validation "the ... are performed" -> "the ... is performed". src/java.base/share/classes/java/net/URL.java line 166: > 164: * The {@code java.net.URL} constructors are deprecated. > 165: * Developers are encouraged to use {@link URI java.net.URI} to parse > 166: * or construct any {@code URL}. In cases where an instance of {@code "any URL" -> "a URL" or "all URLs". src/java.base/share/classes/java/net/URL.java line 168: > 166: * or construct any {@code URL}. In cases where an instance of {@code > 167: * java.net.URL} is needed to open a connection, {@link URI} can be used > 168: * to construct or parse the URL string, possibly calling {@link I wonder if it might be clearer to say "url string", only to avoid anyone thinking they call URL::toString. src/java.base/share/classes/java/net/URL.java line 852: > 850: * @since 20 > 851: */ > 852: public static URL of(URI uri, URLStreamHandler streamHandler) The parameter is named "handler" rather than "streamHandler" in constructors so we should probably keep it the same to avoid any confusion. src/java.base/share/classes/java/net/URL.java line 885: > 883: > 884: @SuppressWarnings("deprecation") > 885: var result = new URL("jrt", host, port, file, null); The URL scheme for jrt does have a port so we should look at that some time. - PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8282664: Unroll by hand StringUTF16 and StringLatin1 polynomial hash loops
On Tue, 25 Oct 2022 10:37:40 GMT, Claes Redestad wrote: > Continuing the work initiated by @luhenry to unroll and then intrinsify > polynomial hash loops. > > I've rewired the library changes to route via a single `@IntrinsicCandidate` > method. To make this work I've harmonized how they are invoked so that > there's less special handling and checks in the intrinsic. Mainly do the > null-check outside of the intrinsic for `Arrays.hashCode` cases. > > Having a centralized entry point means it'll be easier to parameterize the > factor and start values which are now hard-coded (always 31, and a start > value of either one for `Arrays` or zero for `String`). It seems somewhat > premature to parameterize this up front. > > The current implementation is performance neutral on microbenchmarks on all > tested platforms (x64, aarch64) when not enabling the intrinsic. We do add a > few trivial method calls which increase the call stack depth, so surprises > cannot be ruled out on complex workloads. > > With the most recent fixes the x64 intrinsic results on my workstation look > like this: > > Benchmark (size) Mode Cnt ScoreError > Units > StringHashCode.Algorithm.defaultLatin1 1 avgt5 2.199 ± 0.017 > ns/op > StringHashCode.Algorithm.defaultLatin1 10 avgt5 6.933 ± 0.049 > ns/op > StringHashCode.Algorithm.defaultLatin1 100 avgt529.935 ± 0.221 > ns/op > StringHashCode.Algorithm.defaultLatin1 1 avgt5 1596.982 ± 7.020 > ns/op > > Baseline: > > Benchmark (size) Mode Cnt ScoreError > Units > StringHashCode.Algorithm.defaultLatin1 1 avgt5 2.200 ± 0.013 > ns/op > StringHashCode.Algorithm.defaultLatin1 10 avgt5 9.424 ± 0.122 > ns/op > StringHashCode.Algorithm.defaultLatin1 100 avgt590.541 ± 0.512 > ns/op > StringHashCode.Algorithm.defaultLatin1 1 avgt5 9425.321 ± 67.630 > ns/op > > I.e. no measurable overhead compared to baseline even for `size == 1`. > > The vectorized code now nominally works for all unsigned cases as well as > ints, though more testing would be good. > > Benchmark for `Arrays.hashCode`: > > Benchmark (size) Mode Cnt ScoreError Units > ArraysHashCode.bytes1 avgt5 1.884 ± 0.013 ns/op > ArraysHashCode.bytes 10 avgt5 6.955 ± 0.040 ns/op > ArraysHashCode.bytes 100 avgt587.218 ± 0.595 ns/op > ArraysHashCode.bytes1 avgt5 9419.591 ± 38.308 ns/op > ArraysHashCode.chars1 avgt5 2.200 ± 0.010 ns/op > ArraysHashCode.chars 10 avgt5 6.935 ± 0.034 ns/op > ArraysHashCode.chars 100 avgt530.216 ± 0.134 ns/op > ArraysHashCode.chars1 avgt5 1601.629 ± 6.418 ns/op > ArraysHashCode.ints 1 avgt5 2.200 ± 0.007 ns/op > ArraysHashCode.ints10 avgt5 6.936 ± 0.034 ns/op > ArraysHashCode.ints 100 avgt529.412 ± 0.268 ns/op > ArraysHashCode.ints 1 avgt5 1610.578 ± 7.785 ns/op > ArraysHashCode.shorts 1 avgt5 1.885 ± 0.012 ns/op > ArraysHashCode.shorts 10 avgt5 6.961 ± 0.034 ns/op > ArraysHashCode.shorts 100 avgt587.095 ± 0.417 ns/op > ArraysHashCode.shorts 1 avgt5 9420.617 ± 50.089 ns/op > > Baseline: > > Benchmark (size) Mode Cnt ScoreError Units > ArraysHashCode.bytes1 avgt5 3.213 ± 0.207 ns/op > ArraysHashCode.bytes 10 avgt5 8.483 ± 0.040 ns/op > ArraysHashCode.bytes 100 avgt590.315 ± 0.655 ns/op > ArraysHashCode.bytes1 avgt5 9422.094 ± 62.402 ns/op > ArraysHashCode.chars1 avgt5 3.040 ± 0.066 ns/op > ArraysHashCode.chars 10 avgt5 8.497 ± 0.074 ns/op > ArraysHashCode.chars 100 avgt590.074 ± 0.387 ns/op > ArraysHashCode.chars1 avgt5 9420.474 ± 41.619 ns/op > ArraysHashCode.ints 1 avgt5 2.827 ± 0.019 ns/op > ArraysHashCode.ints10 avgt5 7.727 ± 0.043 ns/op > ArraysHashCode.ints 100 avgt589.405 ± 0.593 ns/op > ArraysHashCode.ints 1 avgt5 9426.539 ± 51.308 ns/op > ArraysHashCode.shorts 1 avgt5 3.071 ± 0.062 ns/op > ArraysHashCode.shorts 10 avgt5 8.168 ± 0.049 ns/op > ArraysHashCode.shorts 100 avgt590.399 ± 0.292 ns/op > ArraysHashCode.shorts 1 avgt5 9420.171 ± 44.474 ns/op > > > As we can see the `Arrays` intrinsics are faster for small inputs, and faster > on large inputs for `char` and `int` (the ones currently vectorized). I aim > to fix `byte` and `short` cases before integrating, though it might be > acceptable to hand that off as follow-up enhancements to not further delay > integration of this enhancement. I am planning to
Re: RFR: 8295670: Remove duplication in java/util/Formatter/Basic*.java
On Fri, 28 Oct 2022 21:51:03 GMT, Justin Lu wrote: > Issue: Duplication of methods between Basic*.java test classes, due to auto > generation by genBasic.sh > > Fix: Reorganize parts of Basic-X.java.template into base class in Basic.java. > Toggled -nel flag for generation script (genBasic.sh) to remove excessive > white space. Moved a previous direct change to BasicDateTime.java into the > template. > > Note: Main files to look at are Basic.java and Basic-X.java.template, as the > rest are a reflection of the auto generation test/jdk/java/util/Formatter/Basic-X.java.template line 1608: > 1606: // time zone ID. See JDK-8254865. > 1607: final List list = new ArrayList(); > 1608: Collections.addAll(list, ids); I think final List list = new ArrayList<>(Arrays.asList(ids)); is going to be faster than final List list = new ArrayList(); Collections.addAll(list, ids); Alternatively you can contruct presized ArrayList. test/jdk/java/util/Formatter/Basic-X.java.template line 1612: > 1610: list.remove("America/WhiteHorse"); > 1611: list.remove("Canada/Yukon"); > 1612: ids = list.toArray(new String[list.size()]); ids = list.toArray(new String[0]); is likely to be faster, see https://shipilev.net/blog/2016/arrays-wisdom-ancients/ - PR: https://git.openjdk.org/jdk/pull/10910
Re: RFR: 8295970: Add jdk_vector tests in GHA [v2]
On Fri, 28 Oct 2022 07:19:31 GMT, Jie Fu wrote: >> Hi all, >> >> As discussed here >> https://github.com/openjdk/jdk/pull/10807#pullrequestreview-1150314487 , it >> would be better to add the vector api tests in GHA. >> >> Thanks. >> Best regards, >> Jie > > Jie Fu 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: > > - Add jdk_vector_sanity test group > - Merge branch 'master' into JDK-8295970 > - Revert changes in test.yml > - 8295970: Add jdk_vector tests in GHA What about possibility to run additional group of the test by passing somehow the name of group to the GA, via label or via /test cmd, or via parameter to the specific task in GA? - PR: https://git.openjdk.org/jdk/pull/10879