Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v6]
On Wed, 25 Nov 2020 23:50:14 GMT, Mark Reinhold wrote:
>> Please review this implementation of JEP 396
>> (https://openjdk.java.net/jeps/396).
>> Alan Bateman is the original author; I’ve credited him in the commit
>> metadata.
>> Passes tiers 1-3 on {linux,macos,windows}-x64 and linux-aarch64.
>
> Mark Reinhold has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix "Add-Exports" case for the
> setAccessibleNonPublicMemberNonExportedPackage test (v2)
Marked as reviewed by alanb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/1324
Re: RFR: 8256862: Several java/foreign tests fail on x86_32 platforms [v2]
On Wed, 25 Nov 2020 22:00:13 GMT, Jorn Vernee wrote: >> This patch fixes several failures on x86_32 of java/foreign tests. >> >> This is mostly done by disabling the failing tests, but the impl of CLinker >> is also adjusted ton properly detect 32 bit platforms as unsupported. >> >> CLinker is specified to fail in the initializer on unsupported platforms, >> and a test is added to verify this as well. >> >> Note that this adds requires clauses to the failing tests that explicitly >> enumerate all available platforms, so this should fix the test failures on >> other platforms as well. >> >> https://bugs.openjdk.java.net/browse/JDK-8256757 is filed for the remaining >> failure in TestStringLatin1IndexOfChar. >> >> CSR link: https://bugs.openjdk.java.net/browse/JDK-8256863 > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Exclude TestNulls as well Marked as reviewed by sundar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1386
Re: RFR: 8159746: (proxy) Support for default methods [v7]
On Wed, 25 Nov 2020 22:24:40 GMT, Mandy Chung wrote: >> This proposes a new static `Proxy::invokeDefaultMethod` method to invoke >> the given default method on the given proxy instance. >> >> The implementation looks up a method handle for `invokespecial` instruction >> as if called from with the proxy class as the caller, equivalent to calling >> `X.super::m` where `X` is a proxy interface of the proxy class and >> `X.super::m` will resolve to the specified default method. >> >> The implementation will call a private static `proxyClassLookup(Lookup >> caller)` >> method of the proxy class to obtain its private Lookup. This private method >> in the proxy class only allows a caller Lookup on java.lang.reflect.Proxy >> class >> with full privilege access to use, or else `IllegalAccessException` will be >> thrown. >> >> This patch also proposes to define a proxy class in an unnamed module to >> a dynamic module to strengthen encapsulation such that they are only >> unconditionally exported from a named module but not open for deep reflective >> access. This only applies to the case if all the proxy interfaces are public >> and in a package that is exported or open. >> >> One dynamic module is created for each class loader that defines proxies. >> The change changes the dynamic module to contain another package (same >> name as the module) that is unconditionally exported and is opened to >> `java.base` only. >> >> There is no change to the package and module of the proxy class for >> the following cases: >> >> - if at least one proxy interface is non-public, then the proxy class is >> defined >> in the package and module of the non-public interfaces >> - if at least one proxy is in a package that is non-exported and non-open, >> if all proxy interfaces are public, then the proxy class is defined in >> a non-exported, non-open package of a dynamic module. >> >> The spec change is that a proxy class used to be defined in an unnamed >> module, i.e. in a exported and open package, is defined in an unconditionally >> exported but non-open package. Programs that assume it to be open >> unconditionally >> will be affected and cannot do deep reflection on such proxy classes. >> >> Peter Levart contributed an initial prototype [1] (thanks Peter). I think >> the exceptions could be simplified as more checking should be done prior to >> the invocation of the method handle like checking the types of the arguments >> with the method type. This approach avoids defining a public API >> `protected Proxy::$$proxyClassLookup$$` method. Instead it defines a >> private static method that is restricted for Proxy class to use (by >> taking a caller parameter to ensure it's a private lookup on Proxy class). >> >> javadoc/specdiff: >> http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/api/ >> http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/specdiff/ >> >> [1] >> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041629.html > > Mandy Chung 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 25 additional > commits since the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into > proxy-default-method > - update copyright header > - clean up DefaultMethodProxy test > - Performance improvement contributed by plevart > - Clean up the patch. Rename to > - Merge branch 'master' of https://github.com/openjdk/jdk into > proxy-default-method > - move invokeDefaultMethod to InvocationHandler and throw IAE if access > check fails > - Merge branch 'master' of https://github.com/openjdk/jdk into > proxy-default-method > - Merge branch 'master' of https://github.com/openjdk/jdk into > proxy-default-method > - minor tweak to the spec wording and impl > - ... and 15 more: > https://git.openjdk.java.net/jdk/compare/0d685bc0...d72d627f Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/313
Re: RFR: 8251989: Hex formatting and parsing utility [v10]
On Wed, 25 Nov 2020 22:51:44 GMT, Roger Riggs wrote:
>> java.util.HexFormat utility:
>>
>> - Format and parse hexadecimal strings, with parameters for delimiter,
>> prefix, suffix and upper/lowercase
>> - Static factories and builder methods to create HexFormat copies with
>> modified parameters.
>> - Consistent naming of methods for conversion of byte arrays to formatted
>> strings and back: formatHex and parseHex
>> - Consistent naming of methods for conversion of primitive types:
>> toHexDigits... and fromHexDigits...
>> - Prefix and suffixes now apply to each formatted value, not the string as
>> a whole
>> - Using java.util.Appendable as a target for buffered conversions so output
>> to Writers and PrintStreams
>>like System.out are supported in addition to StringBuilder. (IOExceptions
>> are converted to unchecked exceptions)
>> - Immutable and thread safe, a "value-based" class
>>
>> See the [HexFormat
>> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>> for details.
>>
>> Review comments and suggestions welcome.
>
> Roger Riggs 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 19 additional
> commits since the last revision:
>
> - Clarified that suffix() and prefix() methods do not return null, instead
> the empty string is returned.
> - Merge branch 'master' into 8251989-hex-formatter
> - Merge branch 'master' into 8251989-hex-formatter
> - Merge branch 'master' into 8251989-hex-formatter
> - The HexFormat API indexing model for array and string ranges is changed
>to describe the range using 'fromIndex (inclusive)' and 'toIndex
> (exclusive)'.
>
>Initially, it was specified as 'index' and 'length'. However, both byte
> arrays
>and strings used in the HexFormat API typically use fromIndex and toIndex
>to describe ranges. Using the same indexing model can prevent mistakes.
>
>The change affects the methods and corresponding tests:
>
>formatHex(byte[] bytes, int fromIndex, int toIndex)
>formatHex(A out, byte[] bytes, int fromIndex, int toIndex)
>parseHex(char[] chars, int fromIndex, int toIndex)
>parseHex(CharSequence string, int fromIndex, int toIndex)
>fromHexDigits(CharSequence string, int fromIndex, int toIndex)
>fromHexDigitsToLong(CharSequence string, int fromIndex, int toIndex)
> - - Added @see and @link references to Integer.toHexString and
> Long.toHexString
>- Clarified parsing is case insensistive in various parse and fromXXX
> methods
>- Source level cleanup based on review comments
>- Expanded some javadoc tag text to make it more descriptive
>- Consistent use of 'hexadecimal' vs 'hex'
> - Review comment updates to class javadoc
> - Review comment updates, in the example code, and to describe the
> characters used to convert to hexadecimal
> - Correct length of StringBuilder in formatHex;
>Correct bug in formatHex(char[], 2, 3) and add test for subranges of char[]
> - Merge branch 'master' into 8251989-hex-formatter
> - ... and 9 more:
> https://git.openjdk.java.net/jdk/compare/d1f1e8b7...b19d2827
Hi Roger,
Other than these few comments, there are some files that need copyright year
updates.
src/java.base/share/classes/java/util/HexFormat.java line 42:
> 40: *
> 41: * There are two factories of {@code HexFormat} with preset parameters
> {@link #of()} and
> 42: * {@link #ofDelimiter(String) of(delimiter)}. For other parameter
> combinations
Is that `ofDelimiter(delimiter)` ?
src/java.base/share/classes/java/util/HexFormat.java line 408:
> 406: * @param fromIndex the initial index of the range, inclusive
> 407: * @param toIndex the final index of the range, exclusive.
> 408: * @return a String formatting or null for non-single byte formatting
`non-single byte delimiter`?
src/java.base/share/classes/java/util/HexFormat.java line 853:
> 851: */
> 852: public int fromHexDigit(int ch) {
> 853: int value = Character.digit(ch, 16);
Do we need to limit parsing the hex digit for only [0-9a-fA-F]? This would
return `0` for other digits, say `fullwidth digit zero` (U+FF10)
-
PR: https://git.openjdk.java.net/jdk/pull/482
Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v6]
On Fri, 20 Nov 2020 13:08:11 GMT, Alan Bateman wrote: >> Looks good. > > testWithAddExportsInManifest create an executable JAR with "Add-Exports: > java.base/sun.security.x509" in the manifest. It runs it twice, once without > any options, the second with --illegal-access=permit, and checks there are no > warnings in both cases. To test attempted deep reflection here would need the > equivalent of setAccessibleNonPublicMemberNonExportedPackage that tries to > access some privates in sun.security.x509. It's okay to use > setAccessibleNonPublicMemberNonExportedPackage to test that privates in > sun.nio.ch aren't accessible but it's not connected to the Add-Exports > attribute in the JAR file. Thanks for pointing that out. I fixed it by arranging for the `Add-Exports` test to export both `sun.security.x509` and `sun.nio.ch`. - PR: https://git.openjdk.java.net/jdk/pull/1324
Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v6]
> Please review this implementation of JEP 396
> (https://openjdk.java.net/jeps/396).
> Alan Bateman is the original author; I’ve credited him in the commit metadata.
> Passes tiers 1-3 on {linux,macos,windows}-x64 and linux-aarch64.
Mark Reinhold has updated the pull request incrementally with one additional
commit since the last revision:
Fix "Add-Exports" case for the setAccessibleNonPublicMemberNonExportedPackage
test (v2)
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/1324/files
- new: https://git.openjdk.java.net/jdk/pull/1324/files/1a7ab4e0..0c9654a0
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1324&range=05
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1324&range=04-05
Stats: 5 lines in 1 file changed: 1 ins; 1 del; 3 mod
Patch: https://git.openjdk.java.net/jdk/pull/1324.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/1324/head:pull/1324
PR: https://git.openjdk.java.net/jdk/pull/1324
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo [v2]
> JDK-8188055 added the function Reference.refersTo. For performance, the > supporting native methods Reference.refersTo0 and PhantomReference.refersTo0 > should be intrinsified by C2. > > Initial patch was prepared by @fisk. > > Tested hs-tier1-4. Added new compiler tests to test intrinsics. > > Ran new test with Shenandoah. Found only one issue. As result I disable > PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination. > Someone from Shenandoah team have to test changes if that is enough. Vladimir Kozlov 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 three additional commits since the last revision: - Merge branch 'master' into JDK-8256999 - Added ZLoadBarrierElided = 0 definition. Removed is_exact argument in load_field_from_object(). Added Shenandoah support for narrow phantom accesses. - 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo - Changes: - all: https://git.openjdk.java.net/jdk/pull/1425/files - new: https://git.openjdk.java.net/jdk/pull/1425/files/7bfec378..08bdd307 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1425&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1425&range=00-01 Stats: 8771 lines in 206 files changed: 2656 ins; 872 del; 5243 mod Patch: https://git.openjdk.java.net/jdk/pull/1425.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1425/head:pull/1425 PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8251989: Hex formatting and parsing utility [v10]
> java.util.HexFormat utility: > > - Format and parse hexadecimal strings, with parameters for delimiter, > prefix, suffix and upper/lowercase > - Static factories and builder methods to create HexFormat copies with > modified parameters. > - Consistent naming of methods for conversion of byte arrays to formatted > strings and back: formatHex and parseHex > - Consistent naming of methods for conversion of primitive types: > toHexDigits... and fromHexDigits... > - Prefix and suffixes now apply to each formatted value, not the string as a > whole > - Using java.util.Appendable as a target for buffered conversions so output > to Writers and PrintStreams >like System.out are supported in addition to StringBuilder. (IOExceptions > are converted to unchecked exceptions) > - Immutable and thread safe, a "value-based" class > > See the [HexFormat > javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html) > for details. > > Review comments and suggestions welcome. Roger Riggs 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 19 additional commits since the last revision: - Clarified that suffix() and prefix() methods do not return null, instead the empty string is returned. - Merge branch 'master' into 8251989-hex-formatter - Merge branch 'master' into 8251989-hex-formatter - Merge branch 'master' into 8251989-hex-formatter - The HexFormat API indexing model for array and string ranges is changed to describe the range using 'fromIndex (inclusive)' and 'toIndex (exclusive)'. Initially, it was specified as 'index' and 'length'. However, both byte arrays and strings used in the HexFormat API typically use fromIndex and toIndex to describe ranges. Using the same indexing model can prevent mistakes. The change affects the methods and corresponding tests: formatHex(byte[] bytes, int fromIndex, int toIndex) formatHex(A out, byte[] bytes, int fromIndex, int toIndex) parseHex(char[] chars, int fromIndex, int toIndex) parseHex(CharSequence string, int fromIndex, int toIndex) fromHexDigits(CharSequence string, int fromIndex, int toIndex) fromHexDigitsToLong(CharSequence string, int fromIndex, int toIndex) - - Added @see and @link references to Integer.toHexString and Long.toHexString - Clarified parsing is case insensistive in various parse and fromXXX methods - Source level cleanup based on review comments - Expanded some javadoc tag text to make it more descriptive - Consistent use of 'hexadecimal' vs 'hex' - Review comment updates to class javadoc - Review comment updates, in the example code, and to describe the characters used to convert to hexadecimal - Correct length of StringBuilder in formatHex; Correct bug in formatHex(char[], 2, 3) and add test for subranges of char[] - Merge branch 'master' into 8251989-hex-formatter - ... and 9 more: https://git.openjdk.java.net/jdk/compare/8444acfa...b19d2827 - Changes: - all: https://git.openjdk.java.net/jdk/pull/482/files - new: https://git.openjdk.java.net/jdk/pull/482/files/2aeab7d7..b19d2827 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=482&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=482&range=08-09 Stats: 384227 lines in 3445 files changed: 245788 ins; 106656 del; 31783 mod Patch: https://git.openjdk.java.net/jdk/pull/482.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/482/head:pull/482 PR: https://git.openjdk.java.net/jdk/pull/482
Re: RFR: 8159746: (proxy) Support for default methods [v7]
> This proposes a new static `Proxy::invokeDefaultMethod` method to invoke > the given default method on the given proxy instance. > > The implementation looks up a method handle for `invokespecial` instruction > as if called from with the proxy class as the caller, equivalent to calling > `X.super::m` where `X` is a proxy interface of the proxy class and > `X.super::m` will resolve to the specified default method. > > The implementation will call a private static `proxyClassLookup(Lookup > caller)` > method of the proxy class to obtain its private Lookup. This private method > in the proxy class only allows a caller Lookup on java.lang.reflect.Proxy > class > with full privilege access to use, or else `IllegalAccessException` will be > thrown. > > This patch also proposes to define a proxy class in an unnamed module to > a dynamic module to strengthen encapsulation such that they are only > unconditionally exported from a named module but not open for deep reflective > access. This only applies to the case if all the proxy interfaces are public > and in a package that is exported or open. > > One dynamic module is created for each class loader that defines proxies. > The change changes the dynamic module to contain another package (same > name as the module) that is unconditionally exported and is opened to > `java.base` only. > > There is no change to the package and module of the proxy class for > the following cases: > > - if at least one proxy interface is non-public, then the proxy class is > defined > in the package and module of the non-public interfaces > - if at least one proxy is in a package that is non-exported and non-open, > if all proxy interfaces are public, then the proxy class is defined in > a non-exported, non-open package of a dynamic module. > > The spec change is that a proxy class used to be defined in an unnamed > module, i.e. in a exported and open package, is defined in an unconditionally > exported but non-open package. Programs that assume it to be open > unconditionally > will be affected and cannot do deep reflection on such proxy classes. > > Peter Levart contributed an initial prototype [1] (thanks Peter). I think > the exceptions could be simplified as more checking should be done prior to > the invocation of the method handle like checking the types of the arguments > with the method type. This approach avoids defining a public API > `protected Proxy::$$proxyClassLookup$$` method. Instead it defines a > private static method that is restricted for Proxy class to use (by > taking a caller parameter to ensure it's a private lookup on Proxy class). > > javadoc/specdiff: > http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/api/ > http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/specdiff/ > > [1] > http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041629.html Mandy Chung 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 25 additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into proxy-default-method - update copyright header - clean up DefaultMethodProxy test - Performance improvement contributed by plevart - Clean up the patch. Rename to - Merge branch 'master' of https://github.com/openjdk/jdk into proxy-default-method - move invokeDefaultMethod to InvocationHandler and throw IAE if access check fails - Merge branch 'master' of https://github.com/openjdk/jdk into proxy-default-method - Merge branch 'master' of https://github.com/openjdk/jdk into proxy-default-method - minor tweak to the spec wording and impl - ... and 15 more: https://git.openjdk.java.net/jdk/compare/1d5cd890...d72d627f - Changes: - all: https://git.openjdk.java.net/jdk/pull/313/files - new: https://git.openjdk.java.net/jdk/pull/313/files/d443f10e..d72d627f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=313&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=313&range=05-06 Stats: 393702 lines in 3591 files changed: 248962 ins; 107506 del; 37234 mod Patch: https://git.openjdk.java.net/jdk/pull/313.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/313/head:pull/313 PR: https://git.openjdk.java.net/jdk/pull/313
Re: RFR: 8253751: Dependencies of automatic modules are not propagated through module layers
On Mon, 23 Nov 2020 15:27:52 GMT, Alan Bateman wrote:
> This is a corner case that arises when creating a Configuration for a child
> module layer. If an explicit module in the child configuration reads an
> automatic module in a parent configuration then it should read all automatic
> modules in the parent configurations. Unfortunately, this read edge wasn't
> created for the case that the child layer does not contain any automatic
> modules.
Looks good.
src/java.base/share/classes/java/lang/module/Resolver.java line 580:
> 578: if (m2.descriptor().isAutomatic()) {
> 579: m2.reads()
> 580: .stream()
Nit: not sure if this formatting is intentional or you meant
`m2.reads().stream()` in one line (which I like better).
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1391
Re: RFR: 8256862: Several java/foreign tests fail on x86_32 platforms [v2]
> This patch fixes several failures on x86_32 of java/foreign tests. > > This is mostly done by disabling the failing tests, but the impl of CLinker > is also adjusted ton properly detect 32 bit platforms as unsupported. > > CLinker is specified to fail in the initializer on unsupported platforms, and > a test is added to verify this as well. > > Note that this adds requires clauses to the failing tests that explicitly > enumerate all available platforms, so this should fix the test failures on > other platforms as well. > > https://bugs.openjdk.java.net/browse/JDK-8256757 is filed for the remaining > failure in TestStringLatin1IndexOfChar. > > CSR link: https://bugs.openjdk.java.net/browse/JDK-8256863 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Exclude TestNulls as well - Changes: - all: https://git.openjdk.java.net/jdk/pull/1386/files - new: https://git.openjdk.java.net/jdk/pull/1386/files/bb3f4497..0ea11786 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1386&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1386&range=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1386.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1386/head:pull/1386 PR: https://git.openjdk.java.net/jdk/pull/1386
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 19:48:32 GMT, Jim Laskey wrote: >> At least, it's more clear that it's reversed, i've initially miss the fact >> that f and g are swapped. >> And :: is able to do inference so, i believe it can be written >> >> `.sorted(Comparator.comparingInt(RandomGeneratorFactory::stateBits).reversed())` > > Unfortunately it couldn't be inferred It's because you have added reverse() as a postfix operation so the inference can not flow backward as it should, using Collections.reverseOrder() should fix that `.sorted(Collections.reverseOrder(Comparator.comparingInt(RandomGeneratorFactory::stateBits)))` using some import statics for reverseOrder and comparintInt make the code readable but given it's in the doc, we are out of luck here. - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8255277: randomDelay in DrainDeadlockT and LoggingDeadlock do not randomly delay
On Wed, 25 Nov 2020 20:13:48 GMT, Daniel Fuchs wrote: > Hi, > > Please find here an almost trivial fix for: > 8255277: randomDelay in DrainDeadlockT and LoggingDeadlock do not randomly > delay > > The two tests are changed from using Math.random() to using the jdk test lib > RandomFactory, which prints its seed in the log file and allows for better > reproducibility in case of failures. > > best regards, > > -- daniel looks good - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1441
RFR: 8256894: define test groups
Defined new test groups as defined in ticket. @fguallini - Commit messages: - 8256894: removing manual tests from corelibs tests - 8256894: define test groups Changes: https://git.openjdk.java.net/jdk/pull/1416/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1416&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256894 Stats: 52 lines in 1 file changed: 51 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1416/head:pull/1416 PR: https://git.openjdk.java.net/jdk/pull/1416
Re: RFR: 8256894: define test groups
On Wed, 25 Nov 2020 11:19:27 GMT, Alan Bateman wrote: >> Defined new test groups as defined in ticket. @fguallini > > test/jdk/TEST.groups line 327: > >> 325: :core_tools \ >> 326: :jdk_other \ >> 327: :jdk_core_manual > > Please don't add manual tests to jdk_core. Removed. - PR: https://git.openjdk.java.net/jdk/pull/1416
Re: RFR: 8256894: define test groups
On Tue, 24 Nov 2020 16:13:59 GMT, Ivan Šipka wrote: > Defined new test groups as defined in ticket. @fguallini test/jdk/TEST.groups line 327: > 325: :core_tools \ > 326: :jdk_other \ > 327: :jdk_core_manual Please don't add manual tests to jdk_core. - PR: https://git.openjdk.java.net/jdk/pull/1416
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Wed, 25 Nov 2020 19:05:19 GMT, Stuart Marks wrote:
>> @johnlinp In any case, you'd also need to update the surrounding prose; we
>> need to remove this:
>> returning the current value or {@code null} if absent:```
>
> @pavelrappo
>
>> What is the required level of fidelity particular (pseudo-) code has to have?
>
> It's potentially a large discussion, one that could be had in the context of
> my JEP draft http://openjdk.java.net/jeps/8068562 . However, speaking
> practically, it's possible to focus the discussion fairly concisely: the main
> responsibility of the `@implSpec` ("Implementation Requirements") section is
> to give implementors of subclasses enough information to decide whether to
> inherit the implementation or to override it, and if they override it, what
> behavior they can expect if they were to call `super.compute`.
>
> In this case, a null-value-tolerating Map implementation needs to know that
> the default implementation calls `remove` in the particular case that you
> mentioned. A concurrent Map implementation will also need to know that the
> default implementation calls `get(key)` and `containsKey(key)` at different
> times, potentially leading to a race condition. Both of these inform the
> override vs. inherit decision.
@stuart-marks
> Both of these inform the override vs. inherit decision.
So in this case - fixing the specification to match the default implementation
seems to be the right call - as existing implementations that do not override
are more probably depending on the current default behavior.
-
PR: https://git.openjdk.java.net/jdk/pull/714
RFR: 8255277: randomDelay in DrainDeadlockT and LoggingDeadlock do not randomly delay
Hi, Please find here an almost trivial fix for: 8255277: randomDelay in DrainDeadlockT and LoggingDeadlock do not randomly delay The two tests are changed from using Math.random() to using the jdk test lib RandomFactory, which prints its seed in the log file and allows for better reproducibility in case of failures. best regards, -- daniel - Commit messages: - 8255277: randomDelay in DrainDeadlockT and LoggingDeadlock do not randomly delay Changes: https://git.openjdk.java.net/jdk/pull/1441/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1441&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255277 Stats: 17 lines in 2 files changed: 13 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/1441.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1441/head:pull/1441 PR: https://git.openjdk.java.net/jdk/pull/1441
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 19:52:21 GMT, Aleksey Shipilev wrote: > Your PR have also been bitten by #1427, merge from master to get it fixed. Thanks! I will apply your patch and merge from master latest changes. - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 19:48:40 GMT, Aleksey Shipilev wrote: >>> I just pulled the fresh master, applied this patch on top, enabled >>> `_PhantomReference_refersTo0` in `c2compiler.cpp`, and ran >>> `CONF=linux-x86_64-server-fastdebug make images run-test TEST=tier1 >>> TEST_VM_OPTS="-XX:+UseShenandoahGC"` without problems. >>> >>> @vnkozlov, what Shenandoah failure did you see? Attention @rkennke. >> >> @shipilev 2 new tests added by JDK-8188055 does not trigger C2 compilation. >> You need to run my new test to trigger problem I see: >> >> java -Xbatch -XX:-TieredCompilation -XX:CICompilerCount=1 >> -XX:+UseShenandoahGC TestReferenceRefersTo.java >> >> # Internal Error >> (/open/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp:999), >> pid=2498681, tid=2498694 >> # assert(!is_narrow) failed: phantom access cannot be narrow >> # >> # JRE version: Java(TM) SE Runtime Environment (16.0) (fastdebug build >> 16-internal+0-2020-11-24) >> # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug >> 16-internal+0-2020-11-24, mixed mode, sharing, compressed oops, shenandoah >> gc, linux-amd64) >> # Problematic frame: >> # V [libjvm.so+0x1862090] >> ShenandoahBarrierC2Support::call_lrb_stub(Node*&, Node*&, Node*, Node*&, >> Node*, unsigned long, PhaseIdealLoop*)+0x7e0 > >> @shipilev 2 new tests added by JDK-8188055 does not trigger C2 compilation. > > That sounds like a testbug to me! Since this PR adds C2 intrinsics, I thought > it is expected that new tests trigger it in default test configs... > >> You need to run my new test to trigger problem I see: >> >> ``` >> java -Xbatch -XX:-TieredCompilation -XX:CICompilerCount=1 >> -XX:+UseShenandoahGC TestReferenceRefersTo.java >> >> # Internal Error >> (/open/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp:999), >> pid=2498681, tid=2498694 >> # assert(!is_narrow) failed: phantom access cannot be narrow >> # > > Right. So new intrinsic introduces the "phantom" access from C2 intrinsic > code, and it can be narrow. Shenandoah did not handle that path, because no > existing code shapes were exercising it, and it was considered dead. Since it > is not dead now, we can simply implement that part like this: > http://cr.openjdk.java.net/~shade/shenandoah/8256999-shenandoah-fix.patch. Your PR have also been bitten by #1427, merge from master to get it fixed. - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 17:48:54 GMT, Vladimir Kozlov wrote: > @shipilev 2 new tests added by JDK-8188055 does not trigger C2 compilation. That sounds like a testbug to me! Since this PR adds C2 intrinsics, I thought it is expected that new tests trigger it in default test configs... > You need to run my new test to trigger problem I see: > > ``` > java -Xbatch -XX:-TieredCompilation -XX:CICompilerCount=1 > -XX:+UseShenandoahGC TestReferenceRefersTo.java > > # Internal Error > (/open/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp:999), > pid=2498681, tid=2498694 > # assert(!is_narrow) failed: phantom access cannot be narrow > # Right. So new intrinsic introduces the "phantom" access from C2 intrinsic code, and it can be narrow. Shenandoah did not handle that path, because no existing code shapes were exercising it, and it was considered dead. Since it is not dead now, we can simply implement that part like this: http://cr.openjdk.java.net/~shade/shenandoah/8256999-shenandoah-fix.patch. - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 18:34:28 GMT, Erik Österlund wrote: >> From this conversation the only change I can do is 'Turn (on_weak || >> on_phantom) into !on_strong'. >> @fisk Is this correct? I am concern that it will include `unknown` decorator >> too. >> I agree with Erik to keep !no_keepalive because he prefer it and this is >> code supported by GC group. > > Well if on_weak || on_phantom then it is provably a weak access. But I think > the absence of the strong decorator does not prove it is weak, as it could > have an unknown strength (via unsafe), in which case we need some extra logic > to see if we can prove that an unknown strength access can't be weak. Okay. I will leave changes as it is. - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 16:26:12 GMT, Rémi Forax wrote: >> Not sure that >> `.sorted(Comparator.comparingInt(RandomGeneratorFactory::stateBits).reversed())` >> is simpler than `.sorted((f, g) -> Integer.compare(g.stateBits(), >> f.stateBits()))`. > > At least, it's more clear that it's reversed, i've initially miss the fact > that f and g are swapped. > And :: is able to do inference so, i believe it can be written > > `.sorted(Comparator.comparingInt(RandomGeneratorFactory::stateBits).reversed())` Unfortunately it couldn't be inferred - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v4]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request incrementally with two additional commits since the last revision: - 8248862: Implement Enhanced Pseudo-Random Number Generators Fix extends - 8248862: Implement Enhanced Pseudo-Random Number Generators Use Map.of instead of Map.ofEntries - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/802fa530..ee8f87c3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1292&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1292&range=02-03 Stats: 169 lines in 15 files changed: 23 ins; 17 del; 129 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8180352: Add Stream.toList() method [v4]
On Tue, 24 Nov 2020 07:10:14 GMT, Stuart Marks wrote: >> This change introduces a new terminal operation on Stream. This looks like a >> convenience method for Stream.collect(Collectors.toList()) or >> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this >> method directly on Stream enables it to do what can't easily by done by a >> Collector. In particular, it allows the stream to deposit results directly >> into a destination array (even in parallel) and have this array be wrapped >> in an unmodifiable List without copying. >> >> In the past we've kept most things from the Collections Framework as >> implementations of Collector, not directly on Stream, whereas only >> fundamental things (like toArray) appear directly on Stream. This is true of >> most Collections, but it does seem that List is special. It can be a thin >> wrapper around an array; it can handle generics better than arrays; and >> unlike an array, it can be made unmodifiable (shallowly immutable); and it >> can be value-based. See John Rose's comments in the bug report: >> >> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065 >> >> This operation is null-tolerant, which matches the rest of Streams. This >> isn't specified, though; a general statement about null handling in Streams >> is probably warranted at some point. >> >> Finally, this method is indeed quite convenient (if the caller can deal with >> what this operation returns), as collecting into a List is the most common >> stream terminal operation. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment and assert regarding array class; use switch expression. Recommend you review the problem list of this source file in IntelliJ, which shows some potential cleanups. This can be done as a follow on PR if appropriate, as can any improvements to the default implementation. src/java.base/share/classes/java/util/ImmutableCollections.java line 163: > 161: * > 162: * @param the List's element type > 163: * @param input the input array Rogue parameter declaration. - Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:54:47 GMT, Rémi Forax
wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>> Changes to RandomGeneratorFactory requested by @PaulSandoz
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 235:
>
>> 233: throws IllegalArgumentException {
>> 234: Map> fm =
>> getFactoryMap();
>> 235: Provider provider =
>> fm.get(name.toUpperCase());
>
> again use of toUpperCase() instead of toUpperCase(Locale)
removed
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 250:
>
>> 248: * @return Stream of matching Providers.
>> 249: */
>> 250: static
>> Stream> all(Class
>> category) {
>
> this signature is weird, T is not used in the parameter, so in case return
> any type of Stream> from a type POV, should it be
> ` Stream> all(Class extends T> category)` instead ?
agree
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 269:
>
>> 267: * @throws IllegalArgumentException when either the name or
>> category is null
>> 268: */
>> 269: static T of(String name, Class
>> category)
>
> Same issue as above, T is not used as a constraint
agree
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Wed, 25 Nov 2020 16:39:20 GMT, Pavel Rappo wrote:
>> @pavelrappo Please see my proposed CSR below. Thank you.
>>
>> # Map::compute should have a clearer implementation requirement.
>>
>> ## Summary
>>
>> java.util.Map::compute should have a clearer implementation requirement in
>> its documentation.
>>
>> ## Problem
>>
>> The documentation of the implementation requirements for Map::compute has
>> the following problems:
>> 1. It lacks of return statements for most of the if-else cases.
>> 1. The indents are 3 spaces, while the convention is 4 spaces.
>> 1. The if-else is overly complicated and can be simplified.
>>
>> ## Solution
>>
>> Rewrite the documentation of Map::compute to match its default
>> implementation.
>>
>> ## Specification
>>
>> diff --git a/src/java.base/share/classes/java/util/Map.java
>> b/src/java.base/share/classes/java/util/Map.java
>> index b1de34b42a5..c3118a90581 100644
>> --- a/src/java.base/share/classes/java/util/Map.java
>> +++ b/src/java.base/share/classes/java/util/Map.java
>> @@ -1113,17 +1113,12 @@ public interface Map {
>> * {@code
>> * V oldValue = map.get(key);
>> * V newValue = remappingFunction.apply(key, oldValue);
>> - * if (oldValue != null) {
>> - *if (newValue != null)
>> - * map.put(key, newValue);
>> - *else
>> - * map.remove(key);
>> - * } else {
>> - *if (newValue != null)
>> - * map.put(key, newValue);
>> - *else
>> - * return null;
>> + * if (newValue != null) {
>> + * map.put(key, newValue);
>> + * } else if (oldValue != null) {
>> + * map.remove(key);
>> * }
>> + * return newValue;
>> * }
>> *
>> * The default implementation makes no guarantees about detecting if
>> the
>
> @johnlinp In any case, you'd also need to update the surrounding prose; we
> need to remove this:
> returning the current value or {@code null} if absent:```
@pavelrappo
> What is the required level of fidelity particular (pseudo-) code has to have?
It's potentially a large discussion, one that could be had in the context of my
JEP draft http://openjdk.java.net/jeps/8068562 . However, speaking practically,
it's possible to focus the discussion fairly concisely: the main responsibility
of the `@implSpec` ("Implementation Requirements") section is to give
implementors of subclasses enough information to decide whether to inherit the
implementation or to override it, and if they override it, what behavior they
can expect if they were to call `super.compute`.
In this case, a null-value-tolerating Map implementation needs to know that the
default implementation calls `remove` in the particular case that you
mentioned. A concurrent Map implementation will also need to know that the
default implementation calls `get(key)` and `containsKey(key)` at different
times, potentially leading to a race condition. Both of these inform the
override vs. inherit decision.
-
PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 18:07:34 GMT, Vladimir Kozlov wrote: >> I don't think we have any !in_heap && on_weak loads today. But if we did, >> they would indeed need read barriers. >> We need read barrier if the the reference isn't provably strong... unless >> it's an AS_NO_KEEPALIVE access. That also reflects why the variable is >> called no_keepalive instead of keepalive; it is to reflect the shared >> decorator name used all over the place. I don't mind inverting it though, >> but personally found it easier to read when the names match our decorators. > > From this conversation the only change I can do is 'Turn (on_weak || > on_phantom) into !on_strong'. > @fisk Is this correct? I am concern that it will include `unknown` decorator > too. > I agree with Erik to keep !no_keepalive because he prefer it and this is code > supported by GC group. Well if on_weak || on_phantom then it is provably a weak access. But I think the absence of the strong decorator does not prove it is weak, as it could have an unknown strength (via unsafe), in which case we need some extra logic to see if we can prove that an unknown strength access can't be weak. - PR: https://git.openjdk.java.net/jdk/pull/1425
RFR: 8256862: Several java/foreign tests fail on x86_32 platforms
This patch fixes several failures on x86_32 of java/foreign tests. This is mostly done by disabling the failing tests, but the impl of CLinker is also adjusted ton properly detect 32 bit platforms as unsupported. CLinker is specified to fail in the initializer on unsupported platforms, and a test is added to verify this as well. Note that this adds requires clauses to the failing tests that explicitly enumerate all available platforms, so this should fix the test failures on other platforms as well. https://bugs.openjdk.java.net/browse/JDK-8256757 is filed for the remaining failure in TestStringLatin1IndexOfChar. CSR link: https://bugs.openjdk.java.net/browse/JDK-8256863 - Commit messages: - Enumerate supported platforms specifically in requires tags - Clarify CLinker statement about unsupported platforms - - Add negative test for 32-bit platform. Changes: https://git.openjdk.java.net/jdk/pull/1386/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1386&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256862 Stats: 94 lines in 20 files changed: 83 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/1386.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1386/head:pull/1386 PR: https://git.openjdk.java.net/jdk/pull/1386
Integrated: 8256486: Linux/Windows-x86 builds broken after JDK-8254231
On Tue, 17 Nov 2020 17:19:13 GMT, Jorn Vernee wrote: > JDK-8254231 breaks the Linux and Windows x86 (32-bit) builds. This contains > the needed changes to get it working again. > > Perhaps the most interesting change is adding the `JNI_ENTRY_CPP_NOENV` > macro. Using just JNI_ENTRY was causing a linkage failure, due to the > declaration of the function in the class not having the same linkage > specifiers. It looks like we can't just specify C linkage for class member > functions. > > However, in this case C linkage is not required, so I've added the new macro > which doesn't have `extern "C"`. I've also dropped the `JNIEnv*` parameter, > since it was not being used, but causing extra work for the caller. > > Other than that, it's just about adding default definitions for missing > functions, and moving around some code in MacroAssembler to be in the correct > `#ifdef` blocks. > > Testing: `make images` on Linux and Windows x86_32 platforms. This pull request has now been integrated. Changeset: 7c73fff3 Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/7c73fff3 Stats: 88 lines in 12 files changed: 45 ins; 27 del; 16 mod 8256486: Linux/Windows-x86 builds broken after JDK-8254231 Reviewed-by: shade - PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 15:13:11 GMT, Erik Österlund wrote: >> src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 623: >> >>> 621: // Also we need to add memory barrier to prevent commoning reads >>> 622: // from this field across safepoint since GC can change its value. >>> 623: bool need_read_barrier = (((on_weak || on_phantom) && !no_keepalive) >>> || >> >> There's a slight change: `in_heap && (on_weak || ...)` turns into `(on_weak >> ...) || (in_heap ...)`. It will introduce a read barrier for `!in_heap && >> on_weak` case. Does it occur in practice? >> >> Another one: `on_weak` turns into ((on_weak ...) && !no_keepalive). >> My interpretation is no read barrier needed when `NO_KEEPALIVE` flag is used >> and currently a redundant barrier is issued. >> >> Maybe replace `!no_keepalive` with just `keep_alive`? The former is harder >> to parse. >> >> The check grows bigger and bigger. Maybe it's time to split it? >> >> Turn `on_weak || on_phantom` into `!is_strong`? > > I don't think we have any !in_heap && on_weak loads today. But if we did, > they would indeed need read barriers. > We need read barrier if the the reference isn't provably strong... unless > it's an AS_NO_KEEPALIVE access. That also reflects why the variable is called > no_keepalive instead of keepalive; it is to reflect the shared decorator name > used all over the place. I don't mind inverting it though, but personally > found it easier to read when the names match our decorators. >From this conversation the only change I can do is 'Turn (on_weak || >on_phantom) into !on_strong'. @fisk Is this correct? I am concern that it will include `unknown` decorator too. I agree with Erik to keep !no_keepalive because he prefer it and this is code supported by GC group. - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 11:09:05 GMT, Per Liden wrote: >> JDK-8188055 added the function Reference.refersTo. For performance, the >> supporting native methods Reference.refersTo0 and PhantomReference.refersTo0 >> should be intrinsified by C2. >> >> Initial patch was prepared by @fisk. >> >> Tested hs-tier1-4. Added new compiler tests to test intrinsics. >> >> Ran new test with Shenandoah. Found only one issue. As result I disable >> PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination. >> Someone from Shenandoah team have to test changes if that is enough. > > ZGC changes look good! > I just pulled the fresh master, applied this patch on top, enabled > `_PhantomReference_refersTo0` in `c2compiler.cpp`, and ran > `CONF=linux-x86_64-server-fastdebug make images run-test TEST=tier1 > TEST_VM_OPTS="-XX:+UseShenandoahGC"` without problems. > > @vnkozlov, what Shenandoah failure did you see? Attention @rkennke. @shipilev 2 new tests added by JDK-8188055 does not trigger C2 compilation. You need to run my new test to trigger problem I see: java -Xbatch -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+UseShenandoahGC TestReferenceRefersTo.java # Internal Error (/open/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp:999), pid=2498681, tid=2498694 # assert(!is_narrow) failed: phantom access cannot be narrow # # JRE version: Java(TM) SE Runtime Environment (16.0) (fastdebug build 16-internal+0-2020-11-24) # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 16-internal+0-2020-11-24, mixed mode, sharing, compressed oops, shenandoah gc, linux-amd64) # Problematic frame: # V [libjvm.so+0x1862090] ShenandoahBarrierC2Support::call_lrb_stub(Node*&, Node*&, Node*, Node*&, Node*, unsigned long, PhaseIdealLoop*)+0x7e0 - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 11:05:39 GMT, Per Liden wrote:
>> JDK-8188055 added the function Reference.refersTo. For performance, the
>> supporting native methods Reference.refersTo0 and PhantomReference.refersTo0
>> should be intrinsified by C2.
>>
>> Initial patch was prepared by @fisk.
>>
>> Tested hs-tier1-4. Added new compiler tests to test intrinsics.
>>
>> Ran new test with Shenandoah. Found only one issue. As result I disable
>> PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination.
>> Someone from Shenandoah team have to test changes if that is enough.
>
> src/hotspot/share/opto/library_call.cpp line 5525:
>
>> 5523: Node* LibraryCallKit::load_field_from_object(Node* fromObj, const
>> char* fieldName, const char* fieldTypeString,
>> 5524: DecoratorSet decorators =
>> IN_HEAP, bool is_exact = false, bool is_static = false,
>> 5525: ciInstanceKlass* fromKls
>> = NULL) {
>
> It looks like the `is_exact` argument here can be removed, as all call-sites
> use the default value, which is `false`, and the only use of it in the
> function is this assert, which will never fail.
> assert(!is_exact || tinst->klass_is_exact(), "klass not exact");
Good suggestion.
-
PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 08:18:23 GMT, Vladimir Ivanov wrote: >> src/hotspot/share/opto/c2compiler.cpp line 476: >> >>> 474: if (UseCompressedOops && UseShenandoahGC) return false; >>> 475: #endif >>> 476: break; >> >> Is this intended to disable the intrinsic on all non-64-bit platforms? Is >> that only for Shenandoah 64-bit? I wonder if it should just be: >> >> case vmIntrinsics::_PhantomReference_refersTo0: >> if (UseCompressedOops && UseShenandoahGC) return false; >> break; > > Considering `UseCompressedOops` doesn't make much sense in 32-bit mode and is > set to `false`, it seems `#ifdef` can be just dropped. You are right. I thought flag UseCompressedOops is defined only in 64-bit VM. @shipilev, #ifdef was placed incorrectly - it should be after `case:`. But as you both pointed, it is not needed. I will remove it. - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 15:07:21 GMT, Erik Österlund wrote: >> Ok, makes sense. What do you think about making `ZLoadBarrierElided = 0` >> then? > > I'm okay with that. I don't have a strong preference. I also prefer to have ZLoadBarrierElided = 0. I will add it. - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v6]
On Wed, 25 Nov 2020 17:05:15 GMT, Aleksey Shipilev wrote: >> I've applied open comments. Please let me know if this is ok to merge. > > Still looks fine to me. Merge from master to get the fixes for GH actions > failures, #1427 Thanks, Jorn. The split looks good. - PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8180352: Add Stream.toList() method [v4]
On Wed, 25 Nov 2020 14:07:07 GMT, Peter Levart wrote: >> This is the default implementation in the Stream interface, which is >> overridden by an implementation in the ReferencePipeline class, so it will >> rarely be used in normal operation. The ReferencePipeline version (part of >> this changeset) is based on toArray() and avoids any copying. I'm thus not >> inclined to add new interfaces in order to support this default >> implementation. >> >> As written it's true that the default implementation does perform apparently >> redundant copies, but we can't be assured that toArray() actually returns a >> freshly created array. Thus, we wrap it using Arrays.asList and then copy it >> using the ArrayList constructor. This is unfortunate but necessary to avoid >> situations where someone could hold a reference to the internal array of a >> List, allowing modification of a List that's supposed to be unmodifiable. > > An alternative with similar performance would be to do a Stream.toArray() and > then copy that array into new Object[] and then wrap that copy with > listFromTrustedArrayNullsAllowed(). The difference would be in the > serialization format of the resulting List and maybe also in the access > performance of resulting List (no indirection via the unmodifiableList > wrapper and different type for JIT to speculate about). So if we want the > resulting List to behave exactly the same in both implementations of > toList(), then this alternative might be preferable. WDYT? Such alternative might also be faster using intrisified array copying method (here measuring just copying overhead): Benchmark (len) Mode Cnt Score Error Units ToListBench.toList1 10 avgt 10 14.213 ±0.061 ns/op ToListBench.toList1 1000 avgt 10 541.883 ±3.845 ns/op ToListBench.toList1 100 avgt 10 753223.523 ± 4656.664 ns/op ToListBench.toList2 10 avgt 10 8.810 ±0.052 ns/op ToListBench.toList2 1000 avgt 10 264.748 ±0.807 ns/op ToListBench.toList2 100 avgt 10 349518.502 ± 3242.061 ns/op https://gist.github.com/plevart/974b67b65210f8dd122773f481c0a603 - PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:45:46 GMT, Rémi Forax wrote: >> Jim Laskey has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Changes to RandomGeneratorFactory requested by @PaulSandoz > > src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line > 157: > >> 155: .stream() >> 156: .filter(p -> !p.type().isInterface()) >> 157: .collect(Collectors.toMap(p -> >> p.type().getSimpleName().toUpperCase(), > > toUpperCase() depends on the Locale ! It was a lame thing to do anyway - removed - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v9]
> JDK-8254231 breaks the Linux and Windows x86 (32-bit) builds. This contains > the needed changes to get it working again. > > Perhaps the most interesting change is adding the `JNI_ENTRY_CPP_NOENV` > macro. Using just JNI_ENTRY was causing a linkage failure, due to the > declaration of the function in the class not having the same linkage > specifiers. It looks like we can't just specify C linkage for class member > functions. > > However, in this case C linkage is not required, so I've added the new macro > which doesn't have `extern "C"`. I've also dropped the `JNIEnv*` parameter, > since it was not being used, but causing extra work for the caller. > > Other than that, it's just about adding default definitions for missing > functions, and moving around some code in MacroAssembler to be in the correct > `#ifdef` blocks. > > Testing: `make images` on Linux and Windows x86_32 platforms. Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits: - Merge branch 'master' into Linker_32bit-fixes_New-Master - Split out 32-bit impl from native invoker and upcall handler - Pass in thread instead of rematerializing it. - Merge branch 'master' into Linker_32bit-fixes_Simpler - Remove JNI_ENTRY_CPP_NOENV - - Move reset_last_Java_frame - Use the Unimplemented() macro instead of hlt() - Merge branch 'master' into Linker_32bit-fixes_Simpler - Remove UnsupportedPlatform test - Remove unneeded cast - ... and 3 more: https://git.openjdk.java.net/jdk/compare/461c5fc6...5e664033 - Changes: https://git.openjdk.java.net/jdk/pull/1266/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1266&range=08 Stats: 88 lines in 12 files changed: 45 ins; 27 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/1266.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1266/head:pull/1266 PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v6]
On Wed, 25 Nov 2020 16:30:16 GMT, Jorn Vernee wrote: >> The VM "entry" changes seem better now. Thanks. >> >> The use of CATCH as a safety-net is also good. > > I've applied open comments. Please let me know if this is ok to merge. Still looks fine to me. Merge from master to get the fixes for GH actions failures, #1427 - PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Sat, 21 Nov 2020 09:21:06 GMT, John Lin
wrote:
>> @johnlinp, you cannot create a CSR by yourself at the moment. Someone else
>> will have to do that for you. Might as well be me. So here's my proposal:
>> come up with the meat, then I'll help you with the paperwork.
>>
>> For starters, have a look at existing CSRs (you don't need a JBS account for
>> that). For example,
>> https://bugs.openjdk.java.net/issues/?jql=issuetype%3DCSR%20and%20Subcomponent%3Djava.util%3Acollections%20
>>
>> Fill in an informal CSR template inline in this thread, and we'll proceed
>> from that point. Is that okay?
>
> @pavelrappo Please see my proposed CSR below. Thank you.
>
> # Map::compute should have a clearer implementation requirement.
>
> ## Summary
>
> java.util.Map::compute should have a clearer implementation requirement in
> its documentation.
>
> ## Problem
>
> The documentation of the implementation requirements for Map::compute has the
> following problems:
> 1. It lacks of return statements for most of the if-else cases.
> 1. The indents are 3 spaces, while the convention is 4 spaces.
> 1. The if-else is overly complicated and can be simplified.
>
> ## Solution
>
> Rewrite the documentation of Map::compute to match its default implementation.
>
> ## Specification
>
> diff --git a/src/java.base/share/classes/java/util/Map.java
> b/src/java.base/share/classes/java/util/Map.java
> index b1de34b42a5..c3118a90581 100644
> --- a/src/java.base/share/classes/java/util/Map.java
> +++ b/src/java.base/share/classes/java/util/Map.java
> @@ -1113,17 +1113,12 @@ public interface Map {
> * {@code
> * V oldValue = map.get(key);
> * V newValue = remappingFunction.apply(key, oldValue);
> - * if (oldValue != null) {
> - *if (newValue != null)
> - * map.put(key, newValue);
> - *else
> - * map.remove(key);
> - * } else {
> - *if (newValue != null)
> - * map.put(key, newValue);
> - *else
> - * return null;
> + * if (newValue != null) {
> + * map.put(key, newValue);
> + * } else if (oldValue != null) {
> + * map.remove(key);
> * }
> + * return newValue;
> * }
> *
> * The default implementation makes no guarantees about detecting if
> the
@johnlinp In any case, you'd also need to update the surrounding prose; we need
to remove this:
returning the current value or {@code null} if absent:```
-
PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Wed, 25 Nov 2020 06:19:04 GMT, Stuart Marks wrote: >>> The proposed CSR has a few problems that we need to resolve. >>> >>> 1. The **Specification** pseudo-code behaves differently from both the old >>> pseudo-code and the actual implementation when `newValue == null && >>> oldValue == null` and `map.containsKey(key) == true`. >>> 2. The content of the **Solution** section seems irrelevant: aside from a >>> couple of missing `return` statements the current pseudo-code is fine. We >>> are after something else, aren't we? The bottom line is we should state the >>> solution more clearly. >>> 3. The **Summary** section differs from that of the JDK-8247402. >> >> I read my previous reply and realized that it is confusing and contains a >> factual error; so let me straighten it out in this new reply rather than >> edit that previous one. >> >> 1. The proposed pseudo-code behaves exactly the same way as the existing >> pseudo-code modulo the missing `return` statements. (For some reason, I >> previously wrote that the proposed pseudo-code behaves differently from the >> existing pseudo-code.) >> 2. Both the proposed pseudo-code and the existing pseudo-code deviate from >> the documented behaviour (written in prose) and the actual implementation. >> The deviation happens when `newValue == null && (oldValue = map.get(key)) == >> null` and `map.containsKey(key) == true`. (That part was correct.) >> >> Now, here's what I should have said in my previous reply. If the CSR intends >> to solve (2) then both the proposed pseudo-code and the **Problem** section >> must be updated; otherwise the **Solution** section must be updated. Put >> differently, either fix the diff and add one more item to that problem list, >> or change the solution. Otherwise the solution does not match the problem >> leaving the CSR in a contradictory state. >> >> I hope this all makes sense now. > >> 2. Both the proposed pseudo-code and the existing pseudo-code deviate from >> the documented behaviour (written in prose) and the actual implementation. > > Let me clarify something. The "documented behavior" is actually the API > specification, the contract that applies to Map.compute and all its > implementations. The pseudo-code is part of the "implementation requirements" > section, whose primary responsibility is to specify what the default > implementation actually does. (Of course, that implementation should also > conform to the API spec.) > > We've established that the pseudo-code -- either the existing or proposed > version -- differs from the default implementation's actual behavior. It's > thus failed its primary responsibility, and therefore that's the problem that > needs to be fixed. I think it's pointless to clean up the pseudo-code if the > result is still incorrect. @stuart-marks I thought about it before reading your reply. And since I hadn't come to a definite conclusion, I worded my response to @johnlinp as a choice. Although in this particular case it's easy to fix the discrepancy, I think it raises a general question which applies to all (pseudo-) code snippets in normative parts of API specifications: **What is the required level of fidelity particular (pseudo-) code has to have?** It's such a huge topic for discussion, that I don't even know where to begin; certainly, not in this PR thread. Perhaps we should start a dedicated (non-RFR/PR) thread on one of the OpenJDK mailing lists to host that discussion. - PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v6]
On Wed, 25 Nov 2020 00:01:47 GMT, David Holmes wrote: >> Jorn Vernee 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 10 additional >> commits since the last revision: >> >> - Merge branch 'master' into Linker_32bit-fixes_Simpler >> - Remove JNI_ENTRY_CPP_NOENV >> - - Move reset_last_Java_frame >> - Use the Unimplemented() macro instead of hlt() >> - Merge branch 'master' into Linker_32bit-fixes_Simpler >> - Remove UnsupportedPlatform test >> - Remove unneeded cast >> - Remove Stuff that makes the jdk_foreign tests pass >> - fix test warnings >> - - Fix 32-bit build errors and tests >>- Add negative test for 32-bit platform. >>- Change CABI to fail more lazily when running on an unsupported platform. >>- Change CLinker layouts to be null on unsupported platforms, instead of >> failing when initializing the class >>- Added note to CLinker about failure to initialize on unsupported >> platforms > > The VM "entry" changes seem better now. Thanks. > > The use of CATCH as a safety-net is also good. I've applied open comments. Please let me know if this is ok to merge. - PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 16:22:34 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java
>> line 151:
>>
>>> 149: if (fm == null) {
>>> 150: synchronized (RandomGeneratorFactory.class) {
>>> 151: if (RandomGeneratorFactory.factoryMap == null) {
>>
>> if `RandomGeneratorFactory.factoryMap` is not null, it returns null because
>> the value is not stored in fm.
>>
>> Please use the class holder idiom (cf Effective Java) instead of using the
>> double-check locking pattern.
>
> ? set in 148 and 152, but class holder idiom +1
If the second `RandomGeneratorFactory.factoryMap` return a non null value ...
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 15:59:01 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java
>> line 88:
>>
>>> 86: * {@code
>>> 87: * RandomGeneratorFactory best =
>>> RandomGenerator.all()
>>> 88: * .sorted((f, g) -> Integer.compare(g.stateBits(),
>>> f.stateBits()))
>>
>> use Comparator.comparingInt(RandomGenerator::stateBits) instead of the lambda
>
> Not sure that
> `.sorted(Comparator.comparingInt(RandomGeneratorFactory::stateBits).reversed())`
> is simpler than `.sorted((f, g) -> Integer.compare(g.stateBits(),
> f.stateBits()))`.
At least, it's more clear that it's reversed, i've initially miss the fact that
f and g are swapped.
And :: is able to do inference so, i believe it can be written
`.sorted(Comparator.comparingInt(RandomGeneratorFactory::stateBits).reversed())`
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:38:59 GMT, Rémi Forax
wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>> Changes to RandomGeneratorFactory requested by @PaulSandoz
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 106:
>
>> 104: * Map of provider classes.
>> 105: */
>> 106: private static volatile Map> RandomGenerator>> factoryMap;
>
> should be FACTORY_MAP given that it's a static field
will fall out of using class holder idiom
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 151:
>
>> 149: if (fm == null) {
>> 150: synchronized (RandomGeneratorFactory.class) {
>> 151: if (RandomGeneratorFactory.factoryMap == null) {
>
> if `RandomGeneratorFactory.factoryMap` is not null, it returns null because
> the value is not stored in fm.
>
> Please use the class holder idiom (cf Effective Java) instead of using the
> double-check locking pattern.
? set in 148 and 152, but class holder idiom +1
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 15:43:39 GMT, Jim Laskey wrote: >> will investigate > > Needed to use ThreadLocalRandomProxy.proxy otherwise a cast would be required. yes, right ! - PR: https://git.openjdk.java.net/jdk/pull/1292
Integrated: JDK-8253936 Replace ... with {@code ...} for java.sql
On Fri, 16 Oct 2020 19:38:45 GMT, Vipin Sharma wrote:
> ... is replaced with {@code ...} in java.sql classes.
> Please review and sponsor this change.
This pull request has now been integrated.
Changeset: dee79d60
Author:Vipin Sharma
Committer: Lance Andersen
URL: https://git.openjdk.java.net/jdk/commit/dee79d60
Stats: 4698 lines in 58 files changed: 0 ins; 0 del; 4698 mod
8253936: Replace ... with {@code ...} for java.sql
Reviewed-by: lancea
-
PR: https://git.openjdk.java.net/jdk/pull/707
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:37:02 GMT, Rémi Forax
wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>> Changes to RandomGeneratorFactory requested by @PaulSandoz
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 88:
>
>> 86: * {@code
>> 87: * RandomGeneratorFactory best =
>> RandomGenerator.all()
>> 88: * .sorted((f, g) -> Integer.compare(g.stateBits(),
>> f.stateBits()))
>
> use Comparator.comparingInt(RandomGenerator::stateBits) instead of the lambda
Not sure that
`.sorted(Comparator.comparingInt(RandomGeneratorFactory::stateBits).reversed())`
is simpler than `.sorted((f, g) -> Integer.compare(g.stateBits(),
f.stateBits()))`.
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 116:
>
>> 114: * Map of properties for provider.
>> 115: */
>> 116: private volatile Map properties =
>> null;
>
> `= null` is useless here
agree
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:55:52 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
>> 453:
>>
>>> 451: * @return a {@code RandomGenerator} object that uses {@code
>>> ThreadLocalRandom}
>>> 452: */
>>> 453: public static RandomGenerator proxy() {
>>
>> proxy is a generic name, is there a better name here ?
>
> will investigate
agree
>> src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
>> 459:
>>
>>> 457: // Methods required by class AbstractSpliteratorGenerator
>>> 458: public Spliterator.OfInt makeIntsSpliterator(long index, long
>>> fence, int origin, int bound) {
>>> 459: return new RandomIntsSpliterator(proxy, index, fence, origin,
>>> bound);
>>
>> should use proxy()
>
> will investigate
Needed to use ThreadLocalRandomProxy.proxy otherwise a cast would be required.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 08:43:04 GMT, Vladimir Ivanov wrote: >> JDK-8188055 added the function Reference.refersTo. For performance, the >> supporting native methods Reference.refersTo0 and PhantomReference.refersTo0 >> should be intrinsified by C2. >> >> Initial patch was prepared by @fisk. >> >> Tested hs-tier1-4. Added new compiler tests to test intrinsics. >> >> Ran new test with Shenandoah. Found only one issue. As result I disable >> PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination. >> Someone from Shenandoah team have to test changes if that is enough. > > src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 623: > >> 621: // Also we need to add memory barrier to prevent commoning reads >> 622: // from this field across safepoint since GC can change its value. >> 623: bool need_read_barrier = (((on_weak || on_phantom) && !no_keepalive) >> || > > There's a slight change: `in_heap && (on_weak || ...)` turns into `(on_weak > ...) || (in_heap ...)`. It will introduce a read barrier for `!in_heap && > on_weak` case. Does it occur in practice? > > Another one: `on_weak` turns into ((on_weak ...) && !no_keepalive). > My interpretation is no read barrier needed when `NO_KEEPALIVE` flag is used > and currently a redundant barrier is issued. > > Maybe replace `!no_keepalive` with just `keep_alive`? The former is harder to > parse. > > The check grows bigger and bigger. Maybe it's time to split it? > > Turn `on_weak || on_phantom` into `!is_strong`? I don't think we have any !in_heap && on_weak loads today. But if we did, they would indeed need read barriers. We need read barrier if the the reference isn't provably strong... unless it's an AS_NO_KEEPALIVE access. That also reflects why the variable is called no_keepalive instead of keepalive; it is to reflect the shared decorator name used all over the place. I don't mind inverting it though, but personally found it easier to read when the names match our decorators. - PR: https://git.openjdk.java.net/jdk/pull/1425
RFR: 8253751: Dependencies of automatic modules are not propagated through module layers
This is a corner case that arises when creating a Configuration for a child module layer. If an explicit module in the child configuration reads an automatic module in a parent configuration then it should read all automatic modules in the parent configurations. Unfortunately, this read edge wasn't created for the case that the child layer does not contain any automatic modules. - Commit messages: - Merge - Merge - Add @bug to test - Fixed typo in comment - Merge - Merge - If module reads automatic module in parent configuration then reads all automatic modules Changes: https://git.openjdk.java.net/jdk/pull/1391/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1391&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253751 Stats: 86 lines in 2 files changed: 76 ins; 5 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/1391.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1391/head:pull/1391 PR: https://git.openjdk.java.net/jdk/pull/1391
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 10:01:15 GMT, Vladimir Ivanov wrote: >> The information that there was a barrier attached, is implicit in the >> ins_encode block due to it being run at all. In other words, since we >> matched the mach node to our ZGC access instead of a normal access, we >> already know that there was barrier data attached, and that we no longer >> have such barrier data. > > Ok, makes sense. What do you think about making `ZLoadBarrierElided = 0` then? I'm okay with that. I don't have a strong preference. - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: JDK-8253936 Replace ... with {@code ...} for java.sql [v2]
On Wed, 4 Nov 2020 19:29:34 GMT, Lance Andersen wrote: >> Vipin Sharma has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated copyright year and other issues reporrted in review > > Marked as reviewed by lancea (Reviewer). I think all steps are completed for this PR, please help sponsoring this. - PR: https://git.openjdk.java.net/jdk/pull/707
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote: >> This PR is to introduce a new random number API for the JDK. The primary API >> is found in RandomGenerator and RandomGeneratorFactory. Further description >> can be found in the JEP https://openjdk.java.net/jeps/356 . >> >> javadoc can be found at >> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html >> >> old PR: https://github.com/openjdk/jdk/pull/1273 > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > 8248862: Implement Enhanced Pseudo-Random Number Generators > > Changes to RandomGeneratorFactory requested by @PaulSandoz src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 46: > 44: import java.util.stream.Stream; > 45: import jdk.internal.util.random.RandomSupport.RandomGeneratorProperty; > 46: Instead of calling a method properties to create a Map, it's usually far easier to use an annotation, annotation values supports less runtime type so BigInteger is not supported but using a String instead should be OK. - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
335:
> 333: ctorBytes = tmpCtorBytes;
> 334: ctorLong = tmpCtorLong;
> 335: ctor = tmpCtor;
This one is a volatile write, so the idea is that ctorBytes and ctorLong does
not need to be volatile too, which i think is not true if there is a code
somewhere that uses ctorBytes or ctorLong without reading ctor.
This code is too smart for me, i'm pretty sure it is wrong too on non TSO cpu.
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
480:
> 478: } catch (Exception ex) {
> 479: // Should never happen.
> 480: throw new IllegalStateException("Random algorithm " + name()
> + " is missing a default constructor");
chain the exception ...
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
497:
> 495: ensureConstructors();
> 496: return ctorLong.newInstance(seed);
> 497: } catch (Exception ex) {
this one is very dubious because the result in an exception is thrown is a
random generator with the wrong seed
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
517:
> 515: ensureConstructors();
> 516: return ctorBytes.newInstance((Object)seed);
> 517: } catch (Exception ex) {
same as above
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8180352: Add Stream.toList() method [v4]
On Tue, 24 Nov 2020 03:46:38 GMT, Stuart Marks wrote:
>> In `ReferencePipeline` class we have:
>>
>> @Override
>> public final Object[] toArray() {
>> return toArray(Object[]::new);
>> }
>>
>> @Override
>> @SuppressWarnings("unchecked")
>> public final A[] toArray(IntFunction generator) {
>> ...
>> @SuppressWarnings("rawtypes")
>> IntFunction rawGenerator = (IntFunction) generator;
>> return (A[]) Nodes.flatten(evaluateToArrayNode(rawGenerator),
>> rawGenerator)
>> .asArray(rawGenerator);
>> }
>>
>>
>> In `Nodes` class we have:
>>
>>
>> public static Node flatten(Node node, IntFunction
>> generator) {
>> if (node.getChildCount() > 0) {
>> ...
>> T[] array = generator.apply((int) size);
>> new ToArrayTask.OfRef<>(node, array, 0).invoke();
>> return node(array);
>> } else {
>> return node;
>> }
>> }
>>
>>
>>
>> It looks like it is required to implement `toList` method in a similar way
>> in order to avoid array copy. i.e. there will be `IntFunction>
>> generator` which will generate 'ArrayList' with specified size and the
>> list's `add` method will be called to add elements to the list.
>
> This is the default implementation in the Stream interface, which is
> overridden by an implementation in the ReferencePipeline class, so it will
> rarely be used in normal operation. The ReferencePipeline version (part of
> this changeset) is based on toArray() and avoids any copying. I'm thus not
> inclined to add new interfaces in order to support this default
> implementation.
>
> As written it's true that the default implementation does perform apparently
> redundant copies, but we can't be assured that toArray() actually returns a
> freshly created array. Thus, we wrap it using Arrays.asList and then copy it
> using the ArrayList constructor. This is unfortunate but necessary to avoid
> situations where someone could hold a reference to the internal array of a
> List, allowing modification of a List that's supposed to be unmodifiable.
An alternative with similar performance would be to do a Stream.toArray() and
then copy that array into new Object[] and then wrap that copy with
listFromTrustedArrayNullsAllowed(). The difference would be in the
serialization format of the resulting List and maybe also in the access
performance of resulting List (no indirection via the unmodifiableList wrapper
and different type for JIT to speculate about). So if we want the resulting
List to behave exactly the same in both implementations of toList(), then this
alternative might be preferable. WDYT?
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
235:
> 233: throws IllegalArgumentException {
> 234: Map> fm =
> getFactoryMap();
> 235: Provider provider =
> fm.get(name.toUpperCase());
again use of toUpperCase() instead of toUpperCase(Locale)
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
250:
> 248: * @return Stream of matching Providers.
> 249: */
> 250: static Stream>
> all(Class category) {
this signature is weird, T is not used in the parameter, so in case return any
type of Stream> from a type POV, should it be
` Stream> all(Class category)` instead ?
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
269:
> 267: * @throws IllegalArgumentException when either the name or category
> is null
> 268: */
> 269: static T of(String name, Class
> category)
Same issue as above, T is not used as a constraint
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
288:
> 286: * @throws IllegalArgumentException when either the name or category
> is null
> 287: */
> 288: static RandomGeneratorFactory
> factoryOf(String name, Class category)
Same as above
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
300:
> 298: */
> 299: @SuppressWarnings("unchecked")
> 300: private void getConstructors(Class
> randomGeneratorClass) {
method name get but do side effects
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:31:52 GMT, Rémi Forax
wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>> Changes to RandomGeneratorFactory requested by @PaulSandoz
>
> src/java.base/share/classes/java/util/random/RandomGenerator.java line 745:
>
>> 743: * if the period is unknown.
>> 744: */
>> 745: BigInteger UNKNOWN_PERIOD = BigInteger.ZERO;
>
> move those 3 values into RandomGeneratorFactory ?
Will ponder on this one. I tend to agree with you since they are most likely to
be used for filtering factories RandomGeneratorFactory querying was a later
development. period() originally was a RandomGenerator query, but got moved
when more queries were added. This will require a CSR and will come later.
> src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
> 444:
>
>> 442: }
>> 443:
>> 444: private static final AbstractSpliteratorGenerator proxy = new
>> ThreadLocalRandomProxy();
>
> should be declared inside ThreadLocalRandomProxy, so the value is only
> initialized when proxy() is called
agree
> src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
> 453:
>
>> 451: * @return a {@code RandomGenerator} object that uses {@code
>> ThreadLocalRandom}
>> 452: */
>> 453: public static RandomGenerator proxy() {
>
> proxy is a generic name, is there a better name here ?
will investigate
> src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
> 459:
>
>> 457: // Methods required by class AbstractSpliteratorGenerator
>> 458: public Spliterator.OfInt makeIntsSpliterator(long index, long
>> fence, int origin, int bound) {
>> 459: return new RandomIntsSpliterator(proxy, index, fence, origin,
>> bound);
>
> should use proxy()
will investigate
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:24:37 GMT, Rémi Forax
wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>> Changes to RandomGeneratorFactory requested by @PaulSandoz
>
> src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
> 433:
>
>> 431: private static final class ThreadLocalRandomProxy extends Random {
>> 432: @java.io.Serial
>> 433: static final long serialVersionUID = 0L;
>
> should be private
(instance?) agree
> src/java.base/share/classes/java/security/SecureRandom.java line 223:
>
>> 221: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
>> 222: );
>> 223: }
>
> Using Map.of() instead of Map.ofEntries() should simplify the code
I had assumed Map.ofEntries() was more efficient but it seems it in turn uses
MapN as well. Will change these cases.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
151:
> 149: if (fm == null) {
> 150: synchronized (RandomGeneratorFactory.class) {
> 151: if (RandomGeneratorFactory.factoryMap == null) {
if `RandomGeneratorFactory.factoryMap` is not null, it returns null because the
value is not stored in fm.
Please use the class holder idiom (cf Effective Java) instead of using the
double-check locking pattern.
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
157:
> 155: .stream()
> 156: .filter(p -> !p.type().isInterface())
> 157: .collect(Collectors.toMap(p ->
> p.type().getSimpleName().toUpperCase(),
toUpperCase() depends on the Locale !
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
175:
> 173: if (props == null) {
> 174: synchronized (provider) {
> 175: if (properties == null) {
same issue with the double check locking
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
177:
> 175: if (properties == null) {
> 176: try {
> 177: Method getProperties =
> provider.type().getDeclaredMethod("getProperties");
Is it not a better way than using reflection here ?
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
180:
> 178:
> PrivilegedExceptionAction> getAction =
> () -> {
> 179: getProperties.setAccessible(true);
> 180: return (Map Object>)getProperties.invoke(null);
Given that we have no control over the fact that the method properties()
doesn't return a Map of something else, this unsafe cast seems dangerous (from
the type system POV).
Having a type representing a reified version of the Map may be a better idea
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8257074 Update the ByteBuffers micro benchmark
On Wed, 25 Nov 2020 13:37:48 GMT, Maurizio Cimadamore wrote: > Looks like a good cleanup. I agree that mixing access and instantiation is > not good from a benchmark perspective - although, given that direct buffers > have a rather convoluted allocation process, I guess it would be useful, in > the long term, to track performances of that too (this can be done by > coupling direct buffer allocation with calls to Unsafe::invokeCleaner, so > that the benchmark is not affected by the GC activity). I'll see if I can write a test for this. > Also, given a carrier e.g. `float` there are at least two ways to get a float > from a byte array: > > * ByteBuffer::getFloat (both direct/heap) > * FloatBuffer::get() (both direct/heap) > > The benchmark here focuses on the first bullet, but I think at some point we > should also cover the remaining axis, so that, for each carrier, we really do > at least 4 tests. Of course if you take into account swappiness for non-byte > carriers, you get an extra variant for direct buffers as well, as you wanna > test native endian vs. non-native endian. Yes, good observation - I had similar thoughts. Let me check if we could easily integrate some of these variants. What I'm trying to end up with is a comprehensive(ish) set of test scenarios that can be used to analyse performance ( of which views and endianness are important). It may be reasonable for this particular benchmark to include views, etc, with appropriately named benchmark methods, so that one can easily run a subset, e.g. "org.openjdk.bench.java.nio.ByteBuffers.test(Direct)?(BulkPut|SinglePutByte)", when trying to "zoom in" on a specific code path. - PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
88:
> 86: * {@code
> 87: * RandomGeneratorFactory best =
> RandomGenerator.all()
> 88: * .sorted((f, g) -> Integer.compare(g.stateBits(),
> f.stateBits()))
use Comparator.comparingInt(RandomGenerator::stateBits) instead of the lambda
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
116:
> 114: * Map of properties for provider.
> 115: */
> 116: private volatile Map properties =
> null;
`= null` is useless here
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
106:
> 104: * Map of provider classes.
> 105: */
> 106: private static volatile Map RandomGenerator>> factoryMap;
should be FACTORY_MAP given that it's a static field
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8257074 Update the ByteBuffers micro benchmark
On Wed, 25 Nov 2020 12:41:40 GMT, Chris Hegarty wrote: > The ByteBuffers micro benchmark seems to be a little dated. > > It should be a useful resource to leverage when analysing the performance > impact of any potential implementation changes in the byte buffer classes. > More specifically, the impact of such changes on the performance of sharp > memory access operations. > > This issue proposes to update the benchmark in the following ways to meet the > aforementioned use-case: > > 1. Remove allocation from the individual benchmarks - it just creates noise. > 2. Consolidate per-thread shared heap and direct buffers. > 3. All scenarios now use absolute memory access operations - so no state of > the shared buffers is considered. > 4. Provide more reasonable default fork, warmup, etc, out-of-the-box. > 5. There seems to have been an existing bug in the test where the size > parameter was not always considered - this is now fixed. Looks like a good cleanup. I agree that mixing access and instantiation is not good from a benchmark perspective - although, given that direct buffers have a rather convoluted allocation process, I guess it would be useful, in the long term, to track performances of that too (this can be done by coupling direct buffer allocation with calls to Unsafe::invokeCleaner, so that the benchmark is not affected by the GC activity). Also, given a carrier e.g. `float` there are at least two ways to get a float from a byte array: * ByteBuffer::getFloat (both direct/heap) * FloatBuffer::get() (both direct/heap) The benchmark here focuses on the first bullet, but I think at some point we should also cover the remaining axis, so that, for each carrier, we really do at least 4 tests. Of course if you take into account swappiness for non-byte carriers, you get an extra variant for direct buffers as well, as you wanna test native endian vs. non-native endian. So many possibilities :-) - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote: >> This PR is to introduce a new random number API for the JDK. The primary API >> is found in RandomGenerator and RandomGeneratorFactory. Further description >> can be found in the JEP https://openjdk.java.net/jeps/356 . >> >> javadoc can be found at >> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html >> >> old PR: https://github.com/openjdk/jdk/pull/1273 > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > 8248862: Implement Enhanced Pseudo-Random Number Generators > > Changes to RandomGeneratorFactory requested by @PaulSandoz src/java.base/share/classes/java/util/random/RandomGenerator.java line 745: > 743: * if the period is unknown. > 744: */ > 745: BigInteger UNKNOWN_PERIOD = BigInteger.ZERO; move those 3 values into RandomGeneratorFactory ? - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8257074 Update the ByteBuffers micro benchmark
On Wed, 25 Nov 2020 13:12:34 GMT, Claes Redestad wrote: > I do wonder if there's some value to at least some of these noisy, allocating > variants, though. Could be interesting to zoom in on code where we allocate > transient, small buffers, e.g. when encoding/decoding strings. But perhaps > that needs to be designed differently and focus on a specific use case. Yes, that is an interest case, but largely orthogonal to the primary use-case for this particular micro benchmark (or maybe; the primary use-case that I want to use this benchmark for ;-) ). It appears to me that the primary purpose is to evaluate the memory access operations, so I want to keep buffer allocation out of the picture. Of course, we could decide to just create a new benchmark, but then I'm not sure what we'd use the old one for. - PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8257074 Update the ByteBuffers micro benchmark
On Wed, 25 Nov 2020 12:41:40 GMT, Chris Hegarty wrote: > The ByteBuffers micro benchmark seems to be a little dated. > > It should be a useful resource to leverage when analysing the performance > impact of any potential implementation changes in the byte buffer classes. > More specifically, the impact of such changes on the performance of sharp > memory access operations. > > This issue proposes to update the benchmark in the following ways to meet the > aforementioned use-case: > > 1. Remove allocation from the individual benchmarks - it just creates noise. > 2. Consolidate per-thread shared heap and direct buffers. > 3. All scenarios now use absolute memory access operations - so no state of > the shared buffers is considered. > 4. Provide more reasonable default fork, warmup, etc, out-of-the-box. > 5. There seems to have been an existing bug in the test where the size > parameter was not always considered - this is now fixed. Marked as reviewed by jvernee (Committer). - PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/security/SecureRandom.java line 223:
> 221: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 222: );
> 223: }
Using Map.of() instead of Map.ofEntries() should simplify the code
src/java.base/share/classes/java/util/Random.java line 129:
> 127: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 128: );
> 129: }
Using Map.of() should simplify the code
src/java.base/share/classes/java/util/SplittableRandom.java line 181:
> 179: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 180: );
> 181: }
Using Map.of() should simplify the code
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
169:
> 167: Map.entry(RandomGeneratorProperty.IS_STOCHASTIC, false),
> 168: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 169: );
Using Map.of() should simplify the code
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
433:
> 431: private static final class ThreadLocalRandomProxy extends Random {
> 432: @java.io.Serial
> 433: static final long serialVersionUID = 0L;
should be private
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
444:
> 442: }
> 443:
> 444: private static final AbstractSpliteratorGenerator proxy = new
> ThreadLocalRandomProxy();
should be declared inside ThreadLocalRandomProxy, so the value is only
initialized when proxy() is called
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
453:
> 451: * @return a {@code RandomGenerator} object that uses {@code
> ThreadLocalRandom}
> 452: */
> 453: public static RandomGenerator proxy() {
proxy is a generic name, is there a better name here ?
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
459:
> 457: // Methods required by class AbstractSpliteratorGenerator
> 458: public Spliterator.OfInt makeIntsSpliterator(long index, long fence,
> int origin, int bound) {
> 459: return new RandomIntsSpliterator(proxy, index, fence, origin,
> bound);
should use proxy()
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8257074 Update the ByteBuffers micro benchmark
On Wed, 25 Nov 2020 12:41:40 GMT, Chris Hegarty wrote: > The ByteBuffers micro benchmark seems to be a little dated. > > It should be a useful resource to leverage when analysing the performance > impact of any potential implementation changes in the byte buffer classes. > More specifically, the impact of such changes on the performance of sharp > memory access operations. > > This issue proposes to update the benchmark in the following ways to meet the > aforementioned use-case: > > 1. Remove allocation from the individual benchmarks - it just creates noise. > 2. Consolidate per-thread shared heap and direct buffers. > 3. All scenarios now use absolute memory access operations - so no state of > the shared buffers is considered. > 4. Provide more reasonable default fork, warmup, etc, out-of-the-box. > 5. There seems to have been an existing bug in the test where the size > parameter was not always considered - this is now fixed. I am not an expert in JMH microbenchmarks but the proposed changes look reasonable to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8257074 Update the ByteBuffers micro benchmark
On Wed, 25 Nov 2020 12:41:40 GMT, Chris Hegarty wrote: > The ByteBuffers micro benchmark seems to be a little dated. > > It should be a useful resource to leverage when analysing the performance > impact of any potential implementation changes in the byte buffer classes. > More specifically, the impact of such changes on the performance of sharp > memory access operations. > > This issue proposes to update the benchmark in the following ways to meet the > aforementioned use-case: > > 1. Remove allocation from the individual benchmarks - it just creates noise. > 2. Consolidate per-thread shared heap and direct buffers. > 3. All scenarios now use absolute memory access operations - so no state of > the shared buffers is considered. > 4. Provide more reasonable default fork, warmup, etc, out-of-the-box. > 5. There seems to have been an existing bug in the test where the size > parameter was not always considered - this is now fixed. Overall a reasonable clean-up. I do wonder if there's some value to at least some of these noisy, allocating variants, though. Could be interesting to zoom in on code where we allocate transient, small buffers, e.g. when encoding/decoding strings. But perhaps that needs to be designed differently and focus on a specific use case. - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8180352: Add Stream.toList() method [v4]
- Mail original - > De: "Stuart Marks" > À: "core-libs-dev" > Envoyé: Mardi 24 Novembre 2020 08:10:14 > Objet: Re: RFR: 8180352: Add Stream.toList() method [v4] >> This change introduces a new terminal operation on Stream. This looks like a >> convenience method for Stream.collect(Collectors.toList()) or >> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this >> method directly on Stream enables it to do what can't easily by done by a >> Collector. In particular, it allows the stream to deposit results directly >> into >> a destination array (even in parallel) and have this array be wrapped in an >> unmodifiable List without copying. >> >> In the past we've kept most things from the Collections Framework as >> implementations of Collector, not directly on Stream, whereas only >> fundamental >> things (like toArray) appear directly on Stream. This is true of most >> Collections, but it does seem that List is special. It can be a thin wrapper >> around an array; it can handle generics better than arrays; and unlike an >> array, it can be made unmodifiable (shallowly immutable); and it can be >> value-based. See John Rose's comments in the bug report: >> >> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065 >> >> This operation is null-tolerant, which matches the rest of Streams. This >> isn't >> specified, though; a general statement about null handling in Streams is >> probably warranted at some point. >> >> Finally, this method is indeed quite convenient (if the caller can deal with >> what this operation returns), as collecting into a List is the most common >> stream terminal operation. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment and assert regarding array class; use switch expression. > > - Hi Stuart, This version is Ok for me. I still think that using a null friendly List is a mistake, but you, Brian and John all think it's not an issue, so let's go with it. > > Changes: > - all: https://git.openjdk.java.net/jdk/pull/1026/files > - new: https://git.openjdk.java.net/jdk/pull/1026/files/15beacd2..bd890ae5 > > Webrevs: > - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1026&range=03 > - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1026&range=02-03 > > Stats: 20 lines in 1 file changed: 4 ins; 4 del; 12 mod > Patch: https://git.openjdk.java.net/jdk/pull/1026.diff > Fetch: git fetch https://git.openjdk.java.net/jdk pull/1026/head:pull/1026 > > PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: 8248862: Implement Enhanced Pseudo-Random Number Generators Changes to RandomGeneratorFactory requested by @PaulSandoz - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/9d6d1a94..802fa530 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1292&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1292&range=01-02 Stats: 54 lines in 1 file changed: 23 ins; 7 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v8]
> JDK-8254231 breaks the Linux and Windows x86 (32-bit) builds. This contains > the needed changes to get it working again. > > Perhaps the most interesting change is adding the `JNI_ENTRY_CPP_NOENV` > macro. Using just JNI_ENTRY was causing a linkage failure, due to the > declaration of the function in the class not having the same linkage > specifiers. It looks like we can't just specify C linkage for class member > functions. > > However, in this case C linkage is not required, so I've added the new macro > which doesn't have `extern "C"`. I've also dropped the `JNIEnv*` parameter, > since it was not being used, but causing extra work for the caller. > > Other than that, it's just about adding default definitions for missing > functions, and moving around some code in MacroAssembler to be in the correct > `#ifdef` blocks. > > Testing: `make images` on Linux and Windows x86_32 platforms. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Split out 32-bit impl from native invoker and upcall handler - Changes: - all: https://git.openjdk.java.net/jdk/pull/1266/files - new: https://git.openjdk.java.net/jdk/pull/1266/files/c76286cd..b0edea8d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1266&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1266&range=06-07 Stats: 72 lines in 4 files changed: 64 ins; 8 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1266.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1266/head:pull/1266 PR: https://git.openjdk.java.net/jdk/pull/1266
RFR: 8257074 Update the ByteBuffers micro benchmark
The ByteBuffers micro benchmark seems to be a little dated. It should be a useful resource to leverage when analysing the performance impact of any potential implementation changes in the byte buffer classes. More specifically, the impact of such changes on the performance of sharp memory access operations. This issue proposes to update the benchmark in the following ways to meet the aforementioned use-case: 1. Remove allocation from the individual benchmarks - it just creates noise. 2. Consolidate per-thread shared heap and direct buffers. 3. All scenarios now use absolute memory access operations - so no state of the shared buffers is considered. 4. Provide more reasonable default fork, warmup, etc, out-of-the-box. 5. There seems to have been an existing bug in the test where the size parameter was not always considered - this is now fixed. - Commit messages: - Initial changes Changes: https://git.openjdk.java.net/jdk/pull/1430/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1430&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257074 Stats: 69 lines in 1 file changed: 22 ins; 14 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/1430.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1430/head:pull/1430 PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview)
On Tue, 24 Nov 2020 23:00:05 GMT, Mandy Chung wrote:
>> I agree. This @apiNote needs more clarification to help the readers to
>> understand the context here.One thing we could do in the Package class
>> description to add a "Package Sealing" section that can also explain that it
>> has no relationship to "sealed classes".
>
> I added an API note in `Package::isSealed` [1] to clarify sealed package vs
> sealed class or interface.
>
> The API note you added in `Class::isSealed` can be clarified in a similar
> fashion like: "Sealed class or interface has no relationship with {@linkplain
> Package#isSealed package sealing}".
>
> [1] https://github.com/openjdk/jdk/commit/3c230b8a
Thanks for that update, Mandy! I've tweaked the API note as per your
recommendation. I'll publish a fixed PR later, reflecting the other review
comments as well.
-
PR: https://git.openjdk.java.net/jdk/pull/1227
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v7]
On Wed, 25 Nov 2020 12:16:01 GMT, Vladimir Ivanov wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Pass in thread instead of rematerializing it.
>
> src/hotspot/cpu/x86/universalNativeInvoker_x86.cpp line 33:
>
>> 31:
>> 32: void ProgrammableInvoker::Generator::generate() {
>> 33: #ifdef _LP64
>
> Instead of introducing ifdefs, I'd prefer to see
> `universalNativeInvoker_x86.cpp` split into
> `universalNativeInvoker_x86_64.cpp` (current code) and
> `universalNativeInvoker_x86_32.cpp` (populated with dummy impelmentations).
Yeah, makes sense. The old code had ifdefs, but splitting seems cleaner to me
as well.
-
PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v7]
On Wed, 25 Nov 2020 12:09:07 GMT, Jorn Vernee wrote:
>> JDK-8254231 breaks the Linux and Windows x86 (32-bit) builds. This contains
>> the needed changes to get it working again.
>>
>> Perhaps the most interesting change is adding the `JNI_ENTRY_CPP_NOENV`
>> macro. Using just JNI_ENTRY was causing a linkage failure, due to the
>> declaration of the function in the class not having the same linkage
>> specifiers. It looks like we can't just specify C linkage for class member
>> functions.
>>
>> However, in this case C linkage is not required, so I've added the new macro
>> which doesn't have `extern "C"`. I've also dropped the `JNIEnv*` parameter,
>> since it was not being used, but causing extra work for the caller.
>>
>> Other than that, it's just about adding default definitions for missing
>> functions, and moving around some code in MacroAssembler to be in the
>> correct `#ifdef` blocks.
>>
>> Testing: `make images` on Linux and Windows x86_32 platforms.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Pass in thread instead of rematerializing it.
src/hotspot/cpu/x86/universalNativeInvoker_x86.cpp line 33:
> 31:
> 32: void ProgrammableInvoker::Generator::generate() {
> 33: #ifdef _LP64
Instead of introducing ifdefs, I'd prefer to see
`universalNativeInvoker_x86.cpp` split into `universalNativeInvoker_x86_64.cpp`
(current code) and `universalNativeInvoker_x86_32.cpp` (populated with dummy
impelmentations).
-
PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v7]
> JDK-8254231 breaks the Linux and Windows x86 (32-bit) builds. This contains > the needed changes to get it working again. > > Perhaps the most interesting change is adding the `JNI_ENTRY_CPP_NOENV` > macro. Using just JNI_ENTRY was causing a linkage failure, due to the > declaration of the function in the class not having the same linkage > specifiers. It looks like we can't just specify C linkage for class member > functions. > > However, in this case C linkage is not required, so I've added the new macro > which doesn't have `extern "C"`. I've also dropped the `JNIEnv*` parameter, > since it was not being used, but causing extra work for the caller. > > Other than that, it's just about adding default definitions for missing > functions, and moving around some code in MacroAssembler to be in the correct > `#ifdef` blocks. > > Testing: `make images` on Linux and Windows x86_32 platforms. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Pass in thread instead of rematerializing it. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1266/files - new: https://git.openjdk.java.net/jdk/pull/1266/files/960021ba..c76286cd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1266&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1266&range=05-06 Stats: 6 lines in 2 files changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/1266.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1266/head:pull/1266 PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 03:31:36 GMT, Vladimir Kozlov wrote: > JDK-8188055 added the function Reference.refersTo. For performance, the > supporting native methods Reference.refersTo0 and PhantomReference.refersTo0 > should be intrinsified by C2. > > Initial patch was prepared by @fisk. > > Tested hs-tier1-4. Added new compiler tests to test intrinsics. > > Ran new test with Shenandoah. Found only one issue. As result I disable > PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination. > Someone from Shenandoah team have to test changes if that is enough. ZGC changes look good! - Marked as reviewed by pliden (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 03:31:36 GMT, Vladimir Kozlov wrote:
> JDK-8188055 added the function Reference.refersTo. For performance, the
> supporting native methods Reference.refersTo0 and PhantomReference.refersTo0
> should be intrinsified by C2.
>
> Initial patch was prepared by @fisk.
>
> Tested hs-tier1-4. Added new compiler tests to test intrinsics.
>
> Ran new test with Shenandoah. Found only one issue. As result I disable
> PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination.
> Someone from Shenandoah team have to test changes if that is enough.
src/hotspot/share/opto/library_call.cpp line 5525:
> 5523: Node* LibraryCallKit::load_field_from_object(Node* fromObj, const char*
> fieldName, const char* fieldTypeString,
> 5524: DecoratorSet decorators =
> IN_HEAP, bool is_exact = false, bool is_static = false,
> 5525: ciInstanceKlass* fromKls =
> NULL) {
It looks like the `is_exact` argument here can be removed, as all call-sites
use the default value, which is `false`, and the only use of it in the function
is this assert, which will never fail.
assert(!is_exact || tinst->klass_is_exact(), "klass not exact");
-
PR: https://git.openjdk.java.net/jdk/pull/1425
Integrated: 8256865: Foreign Memory Access and Linker API are missing NPE checks
On Mon, 23 Nov 2020 15:11:49 GMT, Maurizio Cimadamore wrote: > Both the Foreign Memory Access and the Foreign Linker APIs leave something to > be desired when it comes to handling NPEs - first, most of the API javadoc is > oblivious to NPEs being thrown. Secondly, not all API method implementations > add expicit NPE checks - with the result of NPE often being thrown very deep > in the call chain - if at all. Third, test for API coverage of nulls is > ad-hoc. > > This patch rectifies all these issues. To increase coverage for null injected > into APIs, this patch introduces a new framework for testing an API in bulk, > so that all methods are reflectively called with some values replaced with > nulls, so that all combinations are tried. > > I've also added, as part of this patch, a test to cover the statics in > MemoryAccess which were not covered throughly. This pull request has now been integrated. Changeset: 9aeadbb0 Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/9aeadbb0 Stats: 1021 lines in 36 files changed: 826 ins; 164 del; 31 mod 8256865: Foreign Memory Access and Linker API are missing NPE checks Reviewed-by: jvernee, sundar, chegar - PR: https://git.openjdk.java.net/jdk/pull/1388
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v6]
On Tue, 24 Nov 2020 23:59:11 GMT, David Holmes wrote:
>> Jorn Vernee 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 10 additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into Linker_32bit-fixes_Simpler
>> - Remove JNI_ENTRY_CPP_NOENV
>> - - Move reset_last_Java_frame
>> - Use the Unimplemented() macro instead of hlt()
>> - Merge branch 'master' into Linker_32bit-fixes_Simpler
>> - Remove UnsupportedPlatform test
>> - Remove unneeded cast
>> - Remove Stuff that makes the jdk_foreign tests pass
>> - fix test warnings
>> - - Fix 32-bit build errors and tests
>>- Add negative test for 32-bit platform.
>>- Change CABI to fail more lazily when running on an unsupported platform.
>>- Change CLinker layouts to be null on unsupported platforms, instead of
>> failing when initializing the class
>>- Added note to CLinker about failure to initialize on unsupported
>> platforms
>
> src/hotspot/share/prims/universalUpcallHandler.cpp line 37:
>
>> 35:
>> 36: void ProgrammableUpcallHandler::upcall_helper(jobject rec, address buff)
>> {
>> 37: JavaThread* THREAD = JavaThread::current();
>
> You could pass the current thread in rather than re-manifesting it.
Ah, of course. Will fix
-
PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8254082: AbstractStringBuilder.insert(int dstOffset, CharSequence s, int start, int end) is missing fast-path for String [v2]
On Wed, 25 Nov 2020 08:58:21 GMT, Сергей Цыпанов
wrote:
>> Original mail:
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/069197.html
>>
>> Hello,
>>
>> while working with `StringBuilder.insert()` I've spotted that its delegate
>> `AbstractStringBuilder.insert()` is missing
>> a fast-path for the most frequent case when its argument is `String`.
>>
>> Previously they did similart optimization for
>> `StirngBuilder.append(CharSequence, int, int)`,
>> see https://bugs.openjdk.java.net/browse/JDK-8224986
>>
>> I'd like to contribute a trivial patch that brings improvement for the case
>> when SB's content is Latin1
>> and inserted String is Latin1 as well.
>>
>> To measure improvement I've used simple benchmark:
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class StringBuilderInsertBenchmark {
>>
>> @Benchmark
>> public StringBuilder insert(Data data) {
>> String string = data.string;
>> return new StringBuilder().append("ABC").insert(1, string, 1,
>> data.length + 1);
>> }
>>
>> @State(Scope.Thread)
>> public static class Data {
>> String string;
>>
>> @Param({"true", "false"})
>> private boolean latin;
>>
>> @Param({"8", "64", "128", "1024"})
>> private int length;
>>
>> @Setup
>> public void setup() {
>> String alphabet = latin
>> ? "abcdefghijklmnopqrstuvwxyz"// English
>> : "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>>
>> string = new RandomStringGenerator().randomString(alphabet, length +
>> 2);
>> }
>> }
>> }
>>
>> public final class RandomStringGenerator {
>>
>> public String randomString(String alphabet, int length) {
>> char[] chars = alphabet.toCharArray();
>>
>> ThreadLocalRandom random = ThreadLocalRandom.current();
>>
>> char[] array = new char[length];
>> for (int i = 0; i < length; i++) {
>> array[i] = chars[random.nextInt(chars.length)];
>> }
>>
>> return new String(array);
>> }
>> }
>> Which gives
>>
>> (latin) (length) original patchedUnits
>> insert true 824.2 ± 0.1 22.2 ± 0.0ns/op
>> insert true6453.8 ± 0.2 36.1 ± 0.1ns/op
>> insert true 12880.9 ± 0.2 44.6 ± 0.0ns/op
>> insert true 1024 365.4 ± 0.5 109.8 ± 3.9ns/op
>>
>> insertfalse 833.5 ± 0.5 32.3 ± 0.2ns/op
>> insertfalse6473.2 ± 0.3 73.2 ± 0.2ns/op
>> insertfalse 128 103.9 ± 0.6 103.3 ± 0.1ns/op
>> insertfalse 1024 576.5 ± 4.8 569.5 ± 2.0ns/op
>> Patch is attached. As of tests tier1 and tier2 are ok.
>>
>> With best regards,
>> Sergey Tsypanov
>
> Сергей Цыпанов 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 two additional
> commits since the last revision:
>
> - Merge branch 'master' into asb
> - 8254082: Add fast-path for String into AbstractStringBuilder.insert(int,
> CharSequence, int, int)
Hi Sergey,
this is interesting! I think `StringBuilder.insert` wasn't given much love in
JEP 254 since it's likely (much?) less commonly used than `append`. It'd of
course be nice to get it up to speed.
Looking at your patch and existing code I think there might be ways to improve
this further and get an improvement on most non-latin1 cases too. Possibly
cleaning things up a bit, too.
(`putCharsAt(int, String, int, int)` should probably be called `putStringAt`?)
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1716:
> 1714: }
> 1715:
> 1716: private void putCharsAt(int index, String s, int off, int end) {
Comparing this with `putStringAt(int index, String str)` below I think begs for
some consolidation here. I think we either should add a `getBytes(value, index,
coder, length)`, or - perhaps preferably? - factor out the package private
`String.getBytes` and implement it here in ASB using `s.value()`
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1721:
> 1719: return;
> 1720: }
> 1721: inflate();
Like in `String.getBytes(byte[], int, byte)` I think we could do an `arraycopy`
if both are `false` too, just need to carefully adjust the `index` et.c. In
fact the only case that can't use an `arraycopy` in the end is when
`s.isLatin1()` and the current sb is already inflated (that's what the
`StringLatin1.inflate` branch does in `getBytes`).
I think if you consolidate/merge this with the logic in
`String.getBytes(byte[], int, byte)` as suggested you'll end up with a sizeable
improvement on non-latin1 cases too.
-
Changes requested by redestad (Reviewer).
PR: https://git.openjdk.java.net/jd
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 09:47:19 GMT, Erik Österlund wrote:
>> src/hotspot/cpu/x86/gc/z/z_x86_64.ad line 123:
>>
>>> 121:
>>> 122: ins_encode %{
>>> 123: if (barrier_data() != 0) { // barrier could be elided by
>>> ZBarrierSetC2::analyze_dominating_barriers()
>>
>> Maybe keep a bit reserved for `ZLoadBarrierElided` to just map it to `0`?
>> The former is preferred because it keeps the info that there was a barrier
>> data attached in the first place.
>
> The information that there was a barrier attached, is implicit in the
> ins_encode block due to it being run at all. In other words, since we matched
> the mach node to our ZGC access instead of a normal access, we already know
> that there was barrier data attached, and that we no longer have such barrier
> data.
Ok, makes sense. What do you think about making `ZLoadBarrierElided = 0` then?
-
PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 08:30:46 GMT, Vladimir Ivanov wrote:
>> JDK-8188055 added the function Reference.refersTo. For performance, the
>> supporting native methods Reference.refersTo0 and PhantomReference.refersTo0
>> should be intrinsified by C2.
>>
>> Initial patch was prepared by @fisk.
>>
>> Tested hs-tier1-4. Added new compiler tests to test intrinsics.
>>
>> Ran new test with Shenandoah. Found only one issue. As result I disable
>> PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination.
>> Someone from Shenandoah team have to test changes if that is enough.
>
> src/hotspot/cpu/x86/gc/z/z_x86_64.ad line 123:
>
>> 121:
>> 122: ins_encode %{
>> 123: if (barrier_data() != 0) { // barrier could be elided by
>> ZBarrierSetC2::analyze_dominating_barriers()
>
> Maybe keep a bit reserved for `ZLoadBarrierElided` to just map it to `0`? The
> former is preferred because it keeps the info that there was a barrier data
> attached in the first place.
The information that there was a barrier attached, is implicit in the
ins_encode block due to it being run at all. In other words, since we matched
the mach node to our ZGC access instead of a normal access, we already know
that there was barrier data attached, and that we no longer have such barrier
data.
-
PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256865: Foreign Memory Access and Linker API are missing NPE checks [v7]
On Tue, 24 Nov 2020 16:01:15 GMT, Maurizio Cimadamore wrote: >> Both the Foreign Memory Access and the Foreign Linker APIs leave something >> to be desired when it comes to handling NPEs - first, most of the API >> javadoc is oblivious to NPEs being thrown. Secondly, not all API method >> implementations add expicit NPE checks - with the result of NPE often being >> thrown very deep in the call chain - if at all. Third, test for API coverage >> of nulls is ad-hoc. >> >> This patch rectifies all these issues. To increase coverage for null >> injected into APIs, this patch introduces a new framework for testing an API >> in bulk, so that all methods are reflectively called with some values >> replaced with nulls, so that all combinations are tried. >> >> I've also added, as part of this patch, a test to cover the statics in >> MemoryAccess which were not covered throughly. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix another typo caused by bulk replace Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1388
Integrated: 8255904: Remove superfluous use of reflection in Class::isRecord
On Tue, 24 Nov 2020 10:12:23 GMT, Chris Hegarty wrote: > Minor cleanup - Reflective access to j.l.Record is no longer required since > it became standard. This pull request has now been integrated. Changeset: cdb41ba1 Author:Chris Hegarty URL: https://git.openjdk.java.net/jdk/commit/cdb41ba1 Stats: 11 lines in 1 file changed: 0 ins; 10 del; 1 mod 8255904: Remove superfluous use of reflection in Class::isRecord Reviewed-by: redestad, alanb - PR: https://git.openjdk.java.net/jdk/pull/1406
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 03:31:36 GMT, Vladimir Kozlov wrote:
> JDK-8188055 added the function Reference.refersTo. For performance, the
> supporting native methods Reference.refersTo0 and PhantomReference.refersTo0
> should be intrinsified by C2.
>
> Initial patch was prepared by @fisk.
>
> Tested hs-tier1-4. Added new compiler tests to test intrinsics.
>
> Ran new test with Shenandoah. Found only one issue. As result I disable
> PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination.
> Someone from Shenandoah team have to test changes if that is enough.
src/hotspot/cpu/x86/gc/z/z_x86_64.ad line 123:
> 121:
> 122: ins_encode %{
> 123: if (barrier_data() != 0) { // barrier could be elided by
> ZBarrierSetC2::analyze_dominating_barriers()
Maybe keep a bit reserved for `ZLoadBarrierElided` to just map it to `0`? The
former is preferred because it keeps the info that there was a barrier data
attached in the first place.
src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 623:
> 621: // Also we need to add memory barrier to prevent commoning reads
> 622: // from this field across safepoint since GC can change its value.
> 623: bool need_read_barrier = (((on_weak || on_phantom) && !no_keepalive) ||
There's a slight change: `in_heap && (on_weak || ...)` turns into `(on_weak
...) || (in_heap ...)`. It will introduce a read barrier for `!in_heap &&
on_weak` case. Does it occur in practice?
Another one: `on_weak` turns into ((on_weak ...) && !no_keepalive).
My interpretation is no read barrier needed when `NO_KEEPALIVE` flag is used
and currently a redundant barrier is issued.
Maybe replace `!no_keepalive` with just `keep_alive`? The former is harder to
parse.
The check grows bigger and bigger. Maybe it's time to split it?
Turn `on_weak || on_phantom` into `!is_strong`?
-
PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8254082: AbstractStringBuilder.insert(int dstOffset, CharSequence s, int start, int end) is missing fast-path for String [v2]
> Original mail:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/069197.html
>
> Hello,
>
> while working with `StringBuilder.insert()` I've spotted that its delegate
> `AbstractStringBuilder.insert()` is missing
> a fast-path for the most frequent case when its argument is `String`.
>
> Previously they did similart optimization for
> `StirngBuilder.append(CharSequence, int, int)`,
> see https://bugs.openjdk.java.net/browse/JDK-8224986
>
> I'd like to contribute a trivial patch that brings improvement for the case
> when SB's content is Latin1
> and inserted String is Latin1 as well.
>
> To measure improvement I've used simple benchmark:
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class StringBuilderInsertBenchmark {
>
> @Benchmark
> public StringBuilder insert(Data data) {
> String string = data.string;
> return new StringBuilder().append("ABC").insert(1, string, 1, data.length
> + 1);
> }
>
> @State(Scope.Thread)
> public static class Data {
> String string;
>
> @Param({"true", "false"})
> private boolean latin;
>
> @Param({"8", "64", "128", "1024"})
> private int length;
>
> @Setup
> public void setup() {
> String alphabet = latin
> ? "abcdefghijklmnopqrstuvwxyz"// English
> : "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>
> string = new RandomStringGenerator().randomString(alphabet, length + 2);
> }
> }
> }
>
> public final class RandomStringGenerator {
>
> public String randomString(String alphabet, int length) {
> char[] chars = alphabet.toCharArray();
>
> ThreadLocalRandom random = ThreadLocalRandom.current();
>
> char[] array = new char[length];
> for (int i = 0; i < length; i++) {
> array[i] = chars[random.nextInt(chars.length)];
> }
>
> return new String(array);
> }
> }
> Which gives
>
> (latin) (length) original patchedUnits
> insert true 824.2 ± 0.1 22.2 ± 0.0ns/op
> insert true6453.8 ± 0.2 36.1 ± 0.1ns/op
> insert true 12880.9 ± 0.2 44.6 ± 0.0ns/op
> insert true 1024 365.4 ± 0.5 109.8 ± 3.9ns/op
>
> insertfalse 833.5 ± 0.5 32.3 ± 0.2ns/op
> insertfalse6473.2 ± 0.3 73.2 ± 0.2ns/op
> insertfalse 128 103.9 ± 0.6 103.3 ± 0.1ns/op
> insertfalse 1024 576.5 ± 4.8 569.5 ± 2.0ns/op
> Patch is attached. As of tests tier1 and tier2 are ok.
>
> With best regards,
> Sergey Tsypanov
Сергей Цыпанов 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 two additional
commits since the last revision:
- Merge branch 'master' into asb
- 8254082: Add fast-path for String into AbstractStringBuilder.insert(int,
CharSequence, int, int)
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/402/files
- new: https://git.openjdk.java.net/jdk/pull/402/files/128cce75..f7e7b4fe
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=402&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=402&range=00-01
Stats: 556828 lines in 5364 files changed: 448654 ins; 76031 del; 32143 mod
Patch: https://git.openjdk.java.net/jdk/pull/402.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/402/head:pull/402
PR: https://git.openjdk.java.net/jdk/pull/402
Integrated: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, setDaemon and isDaemon
On Thu, 19 Nov 2020 14:24:18 GMT, Alan Bateman wrote: > This change terminally deprecates the following methods defined by > java.lang.ThreadGroup > > - stop > - destroy > - isDestroyed > - setDaemon > - isDaemon > > The stop method has been deprecated since=1.2 because it is inherently > unsafe. It is time to terminally deprecate this method so it can be removed > in a future release. Thread.stop will be examined in a separate issue. > > The destroy, isDestroyed, setDaemon, isDaemon methods support the mechanism > to explicitly or automatically destroy a thread group. As detailed in > JDK-8252885, the mechanism to destroy thread groups is flawed and racy. > Furthermore, this mechanism inhibits efforts to drop the reference from a > thread group to its threads (so that thread creation, starting and > termination do not need to coordinate with their thread group). These methods > should be terminally deprecated so they can be degraded in a future release > and eventually removed. > > CSR with more information: https://bugs.openjdk.java.net/browse/JDK-8256644 This pull request has now been integrated. Changeset: 79e57ace Author:Alan Bateman URL: https://git.openjdk.java.net/jdk/commit/79e57ace Stats: 26 lines in 4 files changed: 23 ins; 0 del; 3 mod 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, setDaemon and isDaemon Reviewed-by: serb, rriggs, iris, mchung, smarks - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 03:31:36 GMT, Vladimir Kozlov wrote: > JDK-8188055 added the function Reference.refersTo. For performance, the > supporting native methods Reference.refersTo0 and PhantomReference.refersTo0 > should be intrinsified by C2. > > Initial patch was prepared by @fisk. > > Tested hs-tier1-4. Added new compiler tests to test intrinsics. > > Ran new test with Shenandoah. Found only one issue. As result I disable > PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination. > Someone from Shenandoah team have to test changes if that is enough. I just pulled the fresh master, applied this patch on top, enabled `_PhantomReference_refersTo0` in `c2compiler.cpp`, and ran `CONF=linux-x86_64-server-fastdebug make images run-test TEST=tier1 TEST_VM_OPTS="-XX:+UseShenandoahGC"` without problems. @vnkozlov, what Shenandoah failure did you see? Attention @rkennke. - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 07:58:42 GMT, Aleksey Shipilev wrote: >> JDK-8188055 added the function Reference.refersTo. For performance, the >> supporting native methods Reference.refersTo0 and PhantomReference.refersTo0 >> should be intrinsified by C2. >> >> Initial patch was prepared by @fisk. >> >> Tested hs-tier1-4. Added new compiler tests to test intrinsics. >> >> Ran new test with Shenandoah. Found only one issue. As result I disable >> PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination. >> Someone from Shenandoah team have to test changes if that is enough. > > src/hotspot/share/opto/c2compiler.cpp line 476: > >> 474: if (UseCompressedOops && UseShenandoahGC) return false; >> 475: #endif >> 476: break; > > Is this intended to disable the intrinsic on all non-64-bit platforms? Is > that only for Shenandoah 64-bit? I wonder if it should just be: > > case vmIntrinsics::_PhantomReference_refersTo0: > if (UseCompressedOops && UseShenandoahGC) return false; > break; Considering `UseCompressedOops` doesn't make much sense in 32-bit mode and is set to `false`, it seems `#ifdef` can be just dropped. - PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
On Wed, 25 Nov 2020 03:31:36 GMT, Vladimir Kozlov wrote: > JDK-8188055 added the function Reference.refersTo. For performance, the > supporting native methods Reference.refersTo0 and PhantomReference.refersTo0 > should be intrinsified by C2. > > Initial patch was prepared by @fisk. > > Tested hs-tier1-4. Added new compiler tests to test intrinsics. > > Ran new test with Shenandoah. Found only one issue. As result I disable > PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination. > Someone from Shenandoah team have to test changes if that is enough. src/hotspot/share/opto/c2compiler.cpp line 476: > 474: if (UseCompressedOops && UseShenandoahGC) return false; > 475: #endif > 476: break; Is this intended to disable the intrinsic on all non-64-bit platforms? Is that only for Shenandoah 64-bit? I wonder if it should just be: case vmIntrinsics::_PhantomReference_refersTo0: if (UseCompressedOops && UseShenandoahGC) return false; break; - PR: https://git.openjdk.java.net/jdk/pull/1425
