Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification [v2]
On Tue, 24 Nov 2020 03:05:19 GMT, Kim Barrett wrote: >> Please review this change to Reference.clear() to address several issues. >> >> (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent >> field to null may extend the lifetime of the referent value. >> >> (JDK-8240696) For GCs with concurrent reference processing, clearing the >> referent field during reference processing may discard the expected >> notification. >> >> Both of these are addressed by introducing a private native helper function >> for clearing the referent, rather than using an ordinary in-Java field >> assignment. Tests have been added for both of these issues. This required >> adding a new breakpoint in reference processing for ZGC. >> >> Of course, finalization adds some complexity to the problem. We deal with >> that by having FinalReference override clear. The implementation is >> provided by a new package-private method in Reference. (There are a number >> of alternatives, all of them clumsy; finalization is annoying that way.) >> >> While dealing with FinalReference clearing it was noted that the recent >> JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not >> updated to call the new Reference.getInactive(), instead still calling get() >> on FinalReferences, with the JDK-8256106 problems. Fixing that showed the >> assertion for inactive FinalReference added by JDK-8256370 used the wrong >> test. >> >> Rather than tracking down and changing all get() and clear() calls on final >> references and changing them to use getInactive and a new similar clear >> function, I've changed FinalReference to override get and clear, which call >> the helper functions in Reference. I've also renamed getInactive to be more >> explanatory and less convenient to call directly, and similarly named the >> helper for clear. This means that get/clear should never be called on an >> active FinalReference. That's already never done, and would have problems >> if it were. >> >> Testing: >> mach5 tier1-6 >> Local (linux-x64) tier1 using Shenandoah. >> New TestReferenceClearDuringMarking fails for G1 without these changes. >> New TestReferenceClearDuringReferenceProcessing fails for ZGC without these >> changes. > > Kim Barrett has updated the pull request incrementally with two additional > commits since the last revision: > > - new tests need ref in oldgen too > - remove obsolete comment about races with clear and enqueue Looks good! - Marked as reviewed by pliden (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1376
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=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. - 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=1026=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1026=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: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v4]
On Tue, 24 Nov 2020 01:14:20 GMT, David Holmes wrote: >> Jorn Vernee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Use the Unimplemented() macro instead of hlt() > > src/hotspot/share/prims/universalUpcallHandler.cpp line 36: > >> 34: extern struct JavaVM_ main_vm; >> 35: >> 36: JNI_ENTRY_CPP_NOENV(void, >> ProgrammableUpcallHandler::upcall_helper(jobject rec, address buff)) > > I do not like this. I think you have a design flaw here. We should not be > directly calling member functions like this from other native code! The entry > point should be a simple C-linkage function that is a normal JNI_ENTRY > (assuming it actually should be a JNI_ENTRY and not JVM_ENTRY?) and that can > then call the true target. Looking closer at this and the potential call-chain I don't think this is really either a JNI_ENTRY nor a JVM_ENTRY as we would normally define them. The upcall_stub seems to allow a thread to "tunnel" its way into the VM for a Java call, rather than going through the normal exported interfaces (i.e. JNI). I suspect that earlier in the review process it was requested that this be made some kind of ENTRY but I'm really not sure that fits at all. It may be cleaner, simpler and clearer to just declare a non-exported method that use `ThreadInVMFromNative` directly. - PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8256865: Foreign Memory Access and Linker API are missing NPE checks [v2]
On Mon, 23 Nov 2020 18:22:14 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 cut/paste error in FunctionDescriptor Unused imports in TestLayouts.java? - Marked as reviewed by sundar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1388
Re: RFR: 8180352: Add Stream.toList() method [v3]
On Sat, 21 Nov 2020 11:02:27 GMT, Rémi Forax wrote: >> Stuart Marks has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adjust List.copyOf to null-check and copy allowNulls lists. >> Fix equals, hashCode, indexOf, lastIndexOf to handle nulls properly. >> Add MOAT tests for new lists; add equals and hashCode tests. > > src/java.base/share/classes/java/util/ImmutableCollections.java line 222: > >> 220: default: >> 221: return (List) new ListN<>(input, false); >> 222: } > > Using a switch expression + arrow (->) here will allow you too factor the > cast, even if it disappear in the generated bytecode, I think it makes the > code more readable Heh yes, I haven't gotten used to using those yet! - PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v3]
On Sat, 21 Nov 2020 10:58:49 GMT, Rémi Forax
wrote:
>> It's an implementation invariant that the internal array be Object[]. Having
>> it be something other than Object[] can lead to subtle bugs. See
>> [JDK-6260652](https://bugs.openjdk.java.net/browse/JDK-6260652) for example.
>
> you can still calls the varargs with an already created array
> listFromTrustedArray(new String[] { "foo" });
>
> I think at least an assert is missing
> assert input.getClass() == Object.class;
If the parameter were of type `E...`, then a call such as
`listFromTrustedArray("foo", "bar")` would result in a `String[]`. The idea is
to avoid accidents such as the one that caused JDK-6260652, and not prevent
deliberate passing of an array of the wrong type, though this is an internal
interface so it's unlikely to occur. Still, a stronger comment and an assert
might be worthwhile.
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v3]
On Mon, 23 Nov 2020 14:06:02 GMT, sergus13
wrote:
>> This implementation duplicates the array twice, once in this.toArray() and
>> once in the constructor of ArrayList that calls toArray() on
>> Collections.ArrayList.
>>
>> I'm sure there is a better way
>
> 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.
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: JDK-8256475: Fix Behavior when Installer name differs from applicatio… [v4]
On Thu, 19 Nov 2020 20:45:23 GMT, Andy Herrick wrote:
>> …n name.
>
> Andy Herrick has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JDK-8256475: Fix Behavior when Installer name differs from application name.
Looks good.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/MainResources_zh_CN.properties
line 75:
> 73: error.jlink.failed=jlink \u5931\u8D25\uFF0C\u51FA\u73B0 {0}
> 74: error.blocked.option=\u4E0D\u5141\u8BB8\u5728 --jlink-options
> \u4E2D\u4F7F\u7528 jlink \u9009\u9879 [{0}]
> 75: error.no.name=Name not specified with --name and cannot infer one from
> app-image
Do we need to submit a follow-up bug for localization team that we have new
non-localized messages in the region specific properties file? I see that
foreign app image warning is still not localized.
-
Marked as reviewed by kizune (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1300
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification [v2]
On Tue, 24 Nov 2020 02:59:50 GMT, Kim Barrett wrote:
>> Looks good. Just want to request that you also remove the following comment
>> in zReferenceProcessor.cpp, as it's no longer true.
>>
>> --- a/src/hotspot/share/gc/z/zReferenceProcessor.cpp
>> +++ b/src/hotspot/share/gc/z/zReferenceProcessor.cpp
>> @@ -184,12 +184,6 @@ bool ZReferenceProcessor::should_discover(oop
>> reference, ReferenceType type) con
>> }
>>
>> bool ZReferenceProcessor::should_drop(oop reference, ReferenceType type)
>> const {
>> - // This check is racing with a call to Reference.clear() from the
>> application.
>> - // If the application clears the reference after this check it will still
>> end
>> - // up on the pending list, and there's nothing we can do about that
>> without
>> - // changing the Reference.clear() API. This check is also racing with a
>> call
>> - // to Reference.enqueue() from the application, which is unproblematic,
>> since
>> - // the application wants the reference to be enqueued anyway.
>>const oop referent = reference_referent(reference);
>>if (referent == NULL) {
>> // Reference has been cleared, by a call to Reference.enqueue()
>
>> Looks good. Just want to request that you also remove the following comment
>> in zReferenceProcessor.cpp, as it's no longer true.
>>
>> ```
>> --- a/src/hotspot/share/gc/z/zReferenceProcessor.cpp
>> +++ b/src/hotspot/share/gc/z/zReferenceProcessor.cpp
>> @@ -184,12 +184,6 @@ bool ZReferenceProcessor::should_discover(oop
>> reference, ReferenceType type) con
>> }
>>
>> bool ZReferenceProcessor::should_drop(oop reference, ReferenceType type)
>> const {
>> - // This check is racing with a call to Reference.clear() from the
>> application.
>> - // If the application clears the reference after this check it will still
>> end
>> - // up on the pending list, and there's nothing we can do about that
>> without
>> - // changing the Reference.clear() API. This check is also racing with a
>> call
>> - // to Reference.enqueue() from the application, which is unproblematic,
>> since
>> - // the application wants the reference to be enqueued anyway.
>>const oop referent = reference_referent(reference);
>>if (referent == NULL) {
>> // Reference has been cleared, by a call to Reference.enqueue()
>> ```
>
> Done.
I realized there was a theoretical problem with the new tests; they
should also be ensuring the Reference objects are in oldgen. That's
fixed in the latest push.
-
PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification [v2]
> Please review this change to Reference.clear() to address several issues. > > (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent > field to null may extend the lifetime of the referent value. > > (JDK-8240696) For GCs with concurrent reference processing, clearing the > referent field during reference processing may discard the expected > notification. > > Both of these are addressed by introducing a private native helper function > for clearing the referent, rather than using an ordinary in-Java field > assignment. Tests have been added for both of these issues. This required > adding a new breakpoint in reference processing for ZGC. > > Of course, finalization adds some complexity to the problem. We deal with > that by having FinalReference override clear. The implementation is > provided by a new package-private method in Reference. (There are a number > of alternatives, all of them clumsy; finalization is annoying that way.) > > While dealing with FinalReference clearing it was noted that the recent > JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not > updated to call the new Reference.getInactive(), instead still calling get() > on FinalReferences, with the JDK-8256106 problems. Fixing that showed the > assertion for inactive FinalReference added by JDK-8256370 used the wrong > test. > > Rather than tracking down and changing all get() and clear() calls on final > references and changing them to use getInactive and a new similar clear > function, I've changed FinalReference to override get and clear, which call > the helper functions in Reference. I've also renamed getInactive to be more > explanatory and less convenient to call directly, and similarly named the > helper for clear. This means that get/clear should never be called on an > active FinalReference. That's already never done, and would have problems > if it were. > > Testing: > mach5 tier1-6 > Local (linux-x64) tier1 using Shenandoah. > New TestReferenceClearDuringMarking fails for G1 without these changes. > New TestReferenceClearDuringReferenceProcessing fails for ZGC without these > changes. Kim Barrett has updated the pull request incrementally with two additional commits since the last revision: - new tests need ref in oldgen too - remove obsolete comment about races with clear and enqueue - Changes: - all: https://git.openjdk.java.net/jdk/pull/1376/files - new: https://git.openjdk.java.net/jdk/pull/1376/files/46e5b1f6..c19efd70 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1376=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1376=00-01 Stats: 17 lines in 3 files changed: 7 ins; 6 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/1376.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1376/head:pull/1376 PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification [v2]
On Mon, 23 Nov 2020 12:50:31 GMT, Per Liden wrote:
> Looks good. Just want to request that you also remove the following comment
> in zReferenceProcessor.cpp, as it's no longer true.
>
> ```
> --- a/src/hotspot/share/gc/z/zReferenceProcessor.cpp
> +++ b/src/hotspot/share/gc/z/zReferenceProcessor.cpp
> @@ -184,12 +184,6 @@ bool ZReferenceProcessor::should_discover(oop reference,
> ReferenceType type) con
> }
>
> bool ZReferenceProcessor::should_drop(oop reference, ReferenceType type)
> const {
> - // This check is racing with a call to Reference.clear() from the
> application.
> - // If the application clears the reference after this check it will still
> end
> - // up on the pending list, and there's nothing we can do about that without
> - // changing the Reference.clear() API. This check is also racing with a
> call
> - // to Reference.enqueue() from the application, which is unproblematic,
> since
> - // the application wants the reference to be enqueued anyway.
>const oop referent = reference_referent(reference);
>if (referent == NULL) {
> // Reference has been cleared, by a call to Reference.enqueue()
> ```
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256865: Foreign Memory Access and Linker API are missing NPE checks [v2]
On Mon, 23 Nov 2020 18:10:47 GMT, Jorn Vernee wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix cut/paste error in FunctionDescriptor > > Already looked at this in panama-foreign, but found one minor issue. Just an observation but there are precedents for just placing a generic statement in the package and/or class javadoc: "Unless specified otherwise any method that is passed a null reference will throw NullPointerException" rather than adding @throws NPE all over the place. - PR: https://git.openjdk.java.net/jdk/pull/1388
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Mon, 23 Nov 2020 14:35:37 GMT, Pavel Rappo 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. - PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: JDK-8256801: tools/jpackage/share/FileAssociationsTest.java#id0 failed unpack.bat with "Exit code: 1603"
On Mon, 23 Nov 2020 20:16:58 GMT, Andy Herrick wrote: > When executing msiexec (possibly from batch script) retry on exitCode 1603 as > well as 1618 Looks fine. - Marked as reviewed by kizune (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1400
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v4]
On Mon, 23 Nov 2020 20:36:11 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: > > Use the Unimplemented() macro instead of hlt() Hi Jorn, Sorry but I don't agree with the addition of JNI_ENTRY_CPP_NOENV. I think there is a flaw in the way these methods are exposed/structured. See comment below. Thanks, David src/hotspot/share/prims/universalUpcallHandler.cpp line 36: > 34: extern struct JavaVM_ main_vm; > 35: > 36: JNI_ENTRY_CPP_NOENV(void, > ProgrammableUpcallHandler::upcall_helper(jobject rec, address buff)) I do not like this. I think you have a design flaw here. We should not be directly calling member functions like this from other native code! The entry point should be a simple C-linkage function that is a normal JNI_ENTRY (assuming it actually should be a JNI_ENTRY and not JVM_ENTRY?) and that can then call the true target. - Changes requested by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview)
Hi Harold, On 24/11/2020 6:27 am, Harold Seigel wrote: Hi David, Thanks for looking at this. The intent was for method Class.permittedSubclasses() to be implemented similarly to Class.getNestMembers(). Are you suggesting that a security manager check be added to permittedSubclasses() similar to the security manager check in getNestMembers()? No I'm suggesting the change to the API is plain wrong. :) Please see discussion in the CSR. Cheers, David Thanks, Harold On 11/18/2020 12:31 AM, David Holmes wrote: Hi Vincente, On 16/11/2020 11:36 pm, Vicente Romero wrote: Please review the code for the second iteration of sealed classes. In this iteration we are: - Enhancing narrowing reference conversion to allow for stricter checking of cast conversions with respect to sealed type hierarchies. - Also local classes are not considered when determining implicitly declared permitted direct subclasses of a sealed class or sealed interface The major change here seems to be that getPermittedSubclasses() now returns actual Class objects instead of ClassDesc. My recollection from earlier discussions here was that the use of ClassDesc was very deliberate as the permitted subclasses may not actually exist and there may be security concerns with returning them! Cheers, David - - Commit messages: - 8246778: Compiler implementation for Sealed Classes (Second Preview) Changes: https://git.openjdk.java.net/jdk/pull/1227/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1227=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8246778 Stats: 589 lines in 12 files changed: 508 ins; 18 del; 63 mod Patch: https://git.openjdk.java.net/jdk/pull/1227.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1227/head:pull/1227 PR: https://git.openjdk.java.net/jdk/pull/1227
Re: RFR: JDK-8256801: tools/jpackage/share/FileAssociationsTest.java#id0 failed unpack.bat with "Exit code: 1603"
On Mon, 23 Nov 2020 20:16:58 GMT, Andy Herrick wrote: > When executing msiexec (possibly from batch script) retry on exitCode 1603 as > well as 1618 Marked as reviewed by almatvee (Committer). - PR: https://git.openjdk.java.net/jdk/pull/1400
Re: RFR: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v5]
> 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:
Revert: Fix "Add-Exports" case for the
setAccessibleNonPublicMemberNonExportedPackage test
This reverts commit 6a4d4792261f54b014336d1907b0040b15fdb467.
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/1324/files
- new: https://git.openjdk.java.net/jdk/pull/1324/files/6a4d4792..1a7ab4e0
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=1324=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk=1324=03-04
Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 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: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default [v4]
> 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
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/1324/files
- new: https://git.openjdk.java.net/jdk/pull/1324/files/a57e6065..6a4d4792
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=1324=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=1324=02-03
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 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
Integrated: 8256839: JavaDoc for java.time.Period.negated() method
On Mon, 23 Nov 2020 19:24:51 GMT, Naoto Sato wrote: > Hi, > > Please review this simple doc fix. Thanks. This pull request has now been integrated. Changeset: b3497f9b Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/b3497f9b Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8256839: JavaDoc for java.time.Period.negated() method Reviewed-by: rriggs, lancea, joehw, scolebourne - PR: https://git.openjdk.java.net/jdk/pull/1399
Re: RFR: JDK-8256801: tools/jpackage/share/FileAssociationsTest.java#id0 failed unpack.bat with "Exit code: 1603"
On Mon, 23 Nov 2020 20:16:58 GMT, Andy Herrick wrote: > When executing msiexec (possibly from batch script) retry on exitCode 1603 as > well as 1618 Marked as reviewed by asemenyuk (Committer). - PR: https://git.openjdk.java.net/jdk/pull/1400
Re: RFR: 8256370: Add asserts to Reference.getInactive()
On Sun, 22 Nov 2020 22:15:20 GMT, Kim Barrett wrote: >> Thanks for adding this asserts. If there is any reason to use `getInactive` >> by other references in the future, we could remove these asserts at that >> time. > > I didn't notice this before it was integrated. > > The test for inactive isn't right; rather than `next == this` it > should be `next != null`. This becomes apparent once > FinalizerHistogram is fixed to call getInactive() rather than get(). > > I noticed this while working on JDK-8256517, where I ran into some > similar issues. I will address these problems as part of that change. @kimbarrett thanks for correcting this inactive FinalReference assert. - PR: https://git.openjdk.java.net/jdk/pull/1231
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification
On Mon, 23 Nov 2020 01:43:39 GMT, Kim Barrett wrote: > Please review this change to Reference.clear() to address several issues. > > (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent > field to null may extend the lifetime of the referent value. > > (JDK-8240696) For GCs with concurrent reference processing, clearing the > referent field during reference processing may discard the expected > notification. > > Both of these are addressed by introducing a private native helper function > for clearing the referent, rather than using an ordinary in-Java field > assignment. Tests have been added for both of these issues. This required > adding a new breakpoint in reference processing for ZGC. > > Of course, finalization adds some complexity to the problem. We deal with > that by having FinalReference override clear. The implementation is > provided by a new package-private method in Reference. (There are a number > of alternatives, all of them clumsy; finalization is annoying that way.) > > While dealing with FinalReference clearing it was noted that the recent > JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not > updated to call the new Reference.getInactive(), instead still calling get() > on FinalReferences, with the JDK-8256106 problems. Fixing that showed the > assertion for inactive FinalReference added by JDK-8256370 used the wrong > test. > > Rather than tracking down and changing all get() and clear() calls on final > references and changing them to use getInactive and a new similar clear > function, I've changed FinalReference to override get and clear, which call > the helper functions in Reference. I've also renamed getInactive to be more > explanatory and less convenient to call directly, and similarly named the > helper for clear. This means that get/clear should never be called on an > active FinalReference. That's already never done, and would have problems > if it were. > > Testing: > mach5 tier1-6 > Local (linux-x64) tier1 using Shenandoah. > New TestReferenceClearDuringMarking fails for G1 without these changes. > New TestReferenceClearDuringReferenceProcessing fails for ZGC without these > changes. Looks good. Thanks for correcting `assert next != null` inactive FinalReference check. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256839: JavaDoc for java.time.Period.negated() method
On Mon, 23 Nov 2020 19:24:51 GMT, Naoto Sato wrote: > Hi, > > Please review this simple doc fix. Thanks. Marked as reviewed by scolebourne (Author). - PR: https://git.openjdk.java.net/jdk/pull/1399
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification
On Mon, 23 Nov 2020 19:18:05 GMT, Per Liden wrote: >> test/hotspot/jtreg/gc/TestReferenceClearDuringReferenceProcessing.java line >> 28: >> >>> 26: /* @test >>> 27: * @bug 8256517 >>> 28: * @requires vm.gc.Z >> >> Please add | vm.gc.Shenandoah here. > > Note that for this test to be useful, the GC needs to support concurrent GC > breakpoints, which Shenandoah doesn't do. Ok, right. Nevermind then! - PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification
On Mon, 23 Nov 2020 01:43:39 GMT, Kim Barrett wrote: > Please review this change to Reference.clear() to address several issues. > > (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent > field to null may extend the lifetime of the referent value. > > (JDK-8240696) For GCs with concurrent reference processing, clearing the > referent field during reference processing may discard the expected > notification. > > Both of these are addressed by introducing a private native helper function > for clearing the referent, rather than using an ordinary in-Java field > assignment. Tests have been added for both of these issues. This required > adding a new breakpoint in reference processing for ZGC. > > Of course, finalization adds some complexity to the problem. We deal with > that by having FinalReference override clear. The implementation is > provided by a new package-private method in Reference. (There are a number > of alternatives, all of them clumsy; finalization is annoying that way.) > > While dealing with FinalReference clearing it was noted that the recent > JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not > updated to call the new Reference.getInactive(), instead still calling get() > on FinalReferences, with the JDK-8256106 problems. Fixing that showed the > assertion for inactive FinalReference added by JDK-8256370 used the wrong > test. > > Rather than tracking down and changing all get() and clear() calls on final > references and changing them to use getInactive and a new similar clear > function, I've changed FinalReference to override get and clear, which call > the helper functions in Reference. I've also renamed getInactive to be more > explanatory and less convenient to call directly, and similarly named the > helper for clear. This means that get/clear should never be called on an > active FinalReference. That's already never done, and would have problems > if it were. > > Testing: > mach5 tier1-6 > Local (linux-x64) tier1 using Shenandoah. > New TestReferenceClearDuringMarking fails for G1 without these changes. > New TestReferenceClearDuringReferenceProcessing fails for ZGC without these > changes. Looks good! Thanks! - Marked as reviewed by rkennke (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v4]
> 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: Use the Unimplemented() macro instead of hlt() - Changes: - all: https://git.openjdk.java.net/jdk/pull/1266/files - new: https://git.openjdk.java.net/jdk/pull/1266/files/9dcbb58c..4f0c6ef9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1266=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1266=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 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: 8246778: Compiler implementation for Sealed Classes (Second Preview)
Hi David, Thanks for looking at this. The intent was for method Class.permittedSubclasses() to be implemented similarly to Class.getNestMembers(). Are you suggesting that a security manager check be added to permittedSubclasses() similar to the security manager check in getNestMembers()? Thanks, Harold On 11/18/2020 12:31 AM, David Holmes wrote: Hi Vincente, On 16/11/2020 11:36 pm, Vicente Romero wrote: Please review the code for the second iteration of sealed classes. In this iteration we are: - Enhancing narrowing reference conversion to allow for stricter checking of cast conversions with respect to sealed type hierarchies. - Also local classes are not considered when determining implicitly declared permitted direct subclasses of a sealed class or sealed interface The major change here seems to be that getPermittedSubclasses() now returns actual Class objects instead of ClassDesc. My recollection from earlier discussions here was that the use of ClassDesc was very deliberate as the permitted subclasses may not actually exist and there may be security concerns with returning them! Cheers, David - - Commit messages: - 8246778: Compiler implementation for Sealed Classes (Second Preview) Changes: https://git.openjdk.java.net/jdk/pull/1227/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1227=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8246778 Stats: 589 lines in 12 files changed: 508 ins; 18 del; 63 mod Patch: https://git.openjdk.java.net/jdk/pull/1227.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1227/head:pull/1227 PR: https://git.openjdk.java.net/jdk/pull/1227
RFR: JDK-8256801: tools/jpackage/share/FileAssociationsTest.java#id0 failed unpack.bat with "Exit code: 1603"
When executing msiexec (possibly from batch script) retry on exitCode 1603 as well as 1618 - Commit messages: - JDK-8256801: tools/jpackage/share/FileAssociationsTest.java#id0 failed unpack.bat with "Exit code: 1603" Changes: https://git.openjdk.java.net/jdk/pull/1400/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1400=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256801 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1400.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1400/head:pull/1400 PR: https://git.openjdk.java.net/jdk/pull/1400
Re: RFR: 8256839: JavaDoc for java.time.Period.negated() method
On Mon, 23 Nov 2020 19:24:51 GMT, Naoto Sato wrote: > Hi, > > Please review this simple doc fix. Thanks. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1399
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v3]
On Mon, 23 Nov 2020 13:19:23 GMT, Aleksey Shipilev wrote: >> Jorn Vernee has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. > > src/hotspot/cpu/x86/universalNativeInvoker_x86.cpp line 132: > >> 130: __ ret(0); >> 131: #else >> 132: __ hlt(); // NYI > > "Not Implemented Yet" is "NIY". But then there is a macro you can use here: > `Unimplemented()`. Was going for "Not Yet Implemented" (this seems to be more common in existing code) I'll change it to use `Unimplemented();` - PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v2]
On Mon, 23 Nov 2020 19:25:31 GMT, Aleksey Shipilev 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 seven additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into Linker_32bit-fixes_New-Master
>> - - Add negative test for 32-bit platform.
>>- Added note to CLinker about failure to initialize on unsupported
>> platforms
>> - 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/jdk.incubator.foreign/share/classes/jdk/internal/foreign/PlatformLayouts.java
> line 144:
>
>> 142: * The {@code T*} native type.
>> 143: */
>> 144: public static final ValueLayout C_POINTER =
>> ofPointer(LITTLE_ENDIAN, 64);
>
> I somewhat get the change in `Win64()` and `AArch64()`, but why here at
> `SysV()`? Surely x86_32 is the platform with 32-bit pointers?
SysV here is the 64-bit SysV ABI, not 32. Perhaps this needs to be
disambiguated yet, but we can cross that bridge when adding 32-bit support.
Any way, I pushed the wrong thing here. This is supposed to go in another PR.
Will fix.
-
PR: https://git.openjdk.java.net/jdk/pull/1266
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v3]
> 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: Merge branch 'master' into Linker_32bit-fixes_Simpler - Changes: - all: https://git.openjdk.java.net/jdk/pull/1266/files - new: https://git.openjdk.java.net/jdk/pull/1266/files/df5e65c1..9dcbb58c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1266=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1266=01-02 Stats: 94 lines in 20 files changed: 0 ins; 83 del; 11 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: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 19:39:18 GMT, Poonam Bajaj wrote: > With respect to JDK-8255908, the changes look good to me. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 19:56:56 GMT, Severin Gehwolf wrote: >> With respect to JDK-8255908, the changes look good to me. > >> With respect to JDK-8255908, the changes look good to me. > > Thanks! @bobvandette Please review when you've got some cycles to spare. Much appreciated! - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8256839: JavaDoc for java.time.Period.negated() method
On Mon, 23 Nov 2020 19:24:51 GMT, Naoto Sato wrote: > Hi, > > Please review this simple doc fix. Thanks. Looks fine and this is validated via test/jdk/java/time/tck/java/time/TCKPeriod.java, - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1399
Re: RFR: 8256839: JavaDoc for java.time.Period.negated() method
On Mon, 23 Nov 2020 19:24:51 GMT, Naoto Sato wrote: > Hi, > > Please review this simple doc fix. Thanks. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1399
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 15:50:18 GMT, Severin Gehwolf wrote: >> This is an enhancement which solves two issues: >> >> 1. Multiple reads of relevant cgroup interface files. Now interface files >> are only read once per file (just like Hotspot). >> 2. Proxies creation of the impl specific subsystem via `determineType()` as >> before, but now reads all relevant interface files: `/proc/cgroups`, >> `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the >> parsed information to the impl specific subsystem classes for instantiation. >> This allows for more flexibility of testing as interface files can be mocked >> and, thus, more cases can be tested that way without having access to these >> specific systems. For example, proper regression tests for JDK-8217766 and >> JDK-8253435 have been added now with this in place. >> >> * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests >> pass. > > @poonamparhar Perhaps you want to take a look at this in the context of not > regressing JDK-8255908? Thanks! With respect to JDK-8255908, the changes look good to me. - PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v2]
On Mon, 23 Nov 2020 18:58:15 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 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 seven additional
> commits since the last revision:
>
> - Merge branch 'master' into Linker_32bit-fixes_New-Master
> - - Add negative test for 32-bit platform.
>- Added note to CLinker about failure to initialize on unsupported
> platforms
> - 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
Some minor comments before looking more closely.
src/hotspot/cpu/x86/universalNativeInvoker_x86.cpp line 132:
> 130: __ ret(0);
> 131: #else
> 132: __ hlt(); // NYI
"Not Implemented Yet" is "NIY". But then there is a macro you can use here:
`Unimplemented()`.
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/PlatformLayouts.java
line 144:
> 142: * The {@code T*} native type.
> 143: */
> 144: public static final ValueLayout C_POINTER =
> ofPointer(LITTLE_ENDIAN, 64);
I somewhat get the change in `Win64()` and `AArch64()`, but why here at
`SysV()`? Surely x86_32 is the platform with 32-bit pointers?
-
PR: https://git.openjdk.java.net/jdk/pull/1266
RFR: 8256839: JavaDoc for java.time.Period.negated() method
Hi, Please review this simple doc fix. Thanks. - Commit messages: - 8256839: JavaDoc for java.time.Period.negated() method Changes: https://git.openjdk.java.net/jdk/pull/1399/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1399=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256839 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1399.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1399/head:pull/1399 PR: https://git.openjdk.java.net/jdk/pull/1399
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification
On Mon, 23 Nov 2020 15:00:08 GMT, Roman Kennke wrote: >> Please review this change to Reference.clear() to address several issues. >> >> (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent >> field to null may extend the lifetime of the referent value. >> >> (JDK-8240696) For GCs with concurrent reference processing, clearing the >> referent field during reference processing may discard the expected >> notification. >> >> Both of these are addressed by introducing a private native helper function >> for clearing the referent, rather than using an ordinary in-Java field >> assignment. Tests have been added for both of these issues. This required >> adding a new breakpoint in reference processing for ZGC. >> >> Of course, finalization adds some complexity to the problem. We deal with >> that by having FinalReference override clear. The implementation is >> provided by a new package-private method in Reference. (There are a number >> of alternatives, all of them clumsy; finalization is annoying that way.) >> >> While dealing with FinalReference clearing it was noted that the recent >> JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not >> updated to call the new Reference.getInactive(), instead still calling get() >> on FinalReferences, with the JDK-8256106 problems. Fixing that showed the >> assertion for inactive FinalReference added by JDK-8256370 used the wrong >> test. >> >> Rather than tracking down and changing all get() and clear() calls on final >> references and changing them to use getInactive and a new similar clear >> function, I've changed FinalReference to override get and clear, which call >> the helper functions in Reference. I've also renamed getInactive to be more >> explanatory and less convenient to call directly, and similarly named the >> helper for clear. This means that get/clear should never be called on an >> active FinalReference. That's already never done, and would have problems >> if it were. >> >> Testing: >> mach5 tier1-6 >> Local (linux-x64) tier1 using Shenandoah. >> New TestReferenceClearDuringMarking fails for G1 without these changes. >> New TestReferenceClearDuringReferenceProcessing fails for ZGC without these >> changes. > > test/hotspot/jtreg/gc/TestReferenceClearDuringReferenceProcessing.java line > 28: > >> 26: /* @test >> 27: * @bug 8256517 >> 28: * @requires vm.gc.Z > > Please add | vm.gc.Shenandoah here. Note that for this test to be useful, the GC needs to support concurrent GC breakpoints, which Shenandoah doesn't do. - PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231 [v2]
> 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 incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge branch 'master' into Linker_32bit-fixes_New-Master - - Add negative test for 32-bit platform. - Added note to CLinker about failure to initialize on unsupported platforms - 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1266/files - new: https://git.openjdk.java.net/jdk/pull/1266/files/f0fed210..df5e65c1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1266=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1266=00-01 Stats: 1085 lines in 51 files changed: 903 ins; 125 del; 57 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
Integrated: 8256585: Remove in-place conversion vector operators from Vector API
On Thu, 19 Nov 2020 03:26:20 GMT, Sandhya Viswanathan wrote: > Remove partially implemented in-place conversion vector operators from Vector > API: >ofNarrowing, ofWidening, INPLACE_XXX This pull request has now been integrated. Changeset: 9de5d091 Author:Sandhya Viswanathan URL: https://git.openjdk.java.net/jdk/commit/9de5d091 Stats: 133 lines in 2 files changed: 0 ins; 130 del; 3 mod 8256585: Remove in-place conversion vector operators from Vector API Reviewed-by: psandoz - PR: https://git.openjdk.java.net/jdk/pull/1305
Re: RFR: 8256865: Foreign Memory Access and Linker API are missing NPE checks [v2]
> 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 cut/paste error in FunctionDescriptor - Changes: - all: https://git.openjdk.java.net/jdk/pull/1388/files - new: https://git.openjdk.java.net/jdk/pull/1388/files/80ffde44..4565f777 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1388=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1388=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1388.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1388/head:pull/1388 PR: https://git.openjdk.java.net/jdk/pull/1388
Re: RFR: 8256865: Foreign Memory Access and Linker API are missing NPE checks [v2]
On Mon, 23 Nov 2020 17:58:22 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix cut/paste error in FunctionDescriptor
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/FunctionDescriptor.java
> line 152:
>
>> 150: * @return the new function descriptor.
>> 151: * @throws NullPointerException if either {@code addedLayouts ==
>> null}, or any of the
>> 152: * layouts in {@code addedLayouts} is null.@throws
>> NullPointerException if any of the new argument layouts is null.
>
> Spurious `@throws` tag here
Good catch
-
PR: https://git.openjdk.java.net/jdk/pull/1388
Re: RFR: 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.
Already looked at this in panama-foreign, but found one minor issue.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/FunctionDescriptor.java
line 152:
> 150: * @return the new function descriptor.
> 151: * @throws NullPointerException if either {@code addedLayouts ==
> null}, or any of the
> 152: * layouts in {@code addedLayouts} is null.@throws
> NullPointerException if any of the new argument layouts is null.
Spurious `@throws` tag here
-
Marked as reviewed by jvernee (Committer).
PR: https://git.openjdk.java.net/jdk/pull/1388
Re: RFR: 8230501: Class data support for hidden classes [v4]
On Mon, 23 Nov 2020 10:23:25 GMT, Chris Hegarty wrote:
> It is my understanding that `Lookup` object returned from
> defineHiddenClass[WithClassData] features the ORIGINAL access lookup mode,
> right? I cannot find a normative statement to confirm this. If it is the
> case, then it would be good to clarify this.
Yes with ORIGINAL access. I updated `@return` for both `defineHiddenClass`
and `defineHiddenClassWithClassData` as follows:
- * @return the {@code Lookup} object on the hidden class
+ * @return the {@code Lookup} object on the hidden class with
+ * {@linkplain #ORIGINAL original} and
+ * {@linkplain Lookup#hasFullPrivilegeAccess() full privilege} access
-
PR: https://git.openjdk.java.net/jdk/pull/1171
Integrated: 8247432: Update IANA Language Subtag Registry to Version 2020-09-29
On Fri, 20 Nov 2020 17:55:55 GMT, Naoto Sato wrote: > Hi, > > Please review the changes to the subject issue. This is to incorporate the > latest language subtag registry definition into the JDK. This pull request has now been integrated. Changeset: ae0ca743 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/ae0ca743 Stats: 52 lines in 2 files changed: 44 ins; 5 del; 3 mod 8247432: Update IANA Language Subtag Registry to Version 2020-09-29 Reviewed-by: joehw - PR: https://git.openjdk.java.net/jdk/pull/1357
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 15:46:56 GMT, Severin Gehwolf wrote:
> This is an enhancement which solves two issues:
>
> 1. Multiple reads of relevant cgroup interface files. Now interface files are
> only read once per file (just like Hotspot).
> 2. Proxies creation of the impl specific subsystem via `determineType()` as
> before, but now reads all relevant interface files: `/proc/cgroups`,
> `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the
> parsed information to the impl specific subsystem classes for instantiation.
> This allows for more flexibility of testing as interface files can be mocked
> and, thus, more cases can be tested that way without having access to these
> specific systems. For example, proper regression tests for JDK-8217766 and
> JDK-8253435 have been added now with this in place.
>
> * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests
> pass.
test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java line 238:
> 236: assertFalse("Join controller combination expected as cgroups
> v1", res.isCgroupV2());
> 237: CgroupInfo memoryInfo = res.getInfos().get("memory");
> 238: assertEquals("/user.slice/user-1000.slice/session-3.scope",
> memoryInfo.getCgroupPath());
The gist of the Java equivalent of JDK-8253939 (which fixed this for Hotspot).
I.e. a regression test for JDK-8217766 and JDK-8254854.
test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java line 267:
> 265: assertFalse("Duplicate cpusets should not influence detection
> heuristic", res.isCgroupV2());
> 266: CgroupInfo cpuSetInfo = res.getInfos().get("cpuset");
> 267: assertEquals("/sys/fs/cgroup/cpuset",
> cpuSetInfo.getMountPoint());
We can now assert the proper mount point is being used for multiple cpuset
mounts.
-
PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
On Mon, 23 Nov 2020 15:46:56 GMT, Severin Gehwolf wrote: > This is an enhancement which solves two issues: > > 1. Multiple reads of relevant cgroup interface files. Now interface files are > only read once per file (just like Hotspot). > 2. Proxies creation of the impl specific subsystem via `determineType()` as > before, but now reads all relevant interface files: `/proc/cgroups`, > `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the > parsed information to the impl specific subsystem classes for instantiation. > This allows for more flexibility of testing as interface files can be mocked > and, thus, more cases can be tested that way without having access to these > specific systems. For example, proper regression tests for JDK-8217766 and > JDK-8253435 have been added now with this in place. > > * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests > pass. @poonamparhar Perhaps you want to take a look at this in the context of not regressing JDK-8255908? Thanks! src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java line 268: > 266: info.setMountPoint(mountPath); > 267: info.setMountRoot(mountRoot); > 268: } This is the actual fix of JDK-8253435 for the Java side. - PR: https://git.openjdk.java.net/jdk/pull/1393
RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection
This is an enhancement which solves two issues: 1. Multiple reads of relevant cgroup interface files. Now interface files are only read once per file (just like Hotspot). 2. Proxies creation of the impl specific subsystem via `determineType()` as before, but now reads all relevant interface files: `/proc/cgroups`, `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the parsed information to the impl specific subsystem classes for instantiation. This allows for more flexibility of testing as interface files can be mocked and, thus, more cases can be tested that way without having access to these specific systems. For example, proper regression tests for JDK-8217766 and JDK-8253435 have been added now with this in place. * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests pass. - Commit messages: - 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection Changes: https://git.openjdk.java.net/jdk/pull/1393/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1393=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254001 Stats: 537 lines in 5 files changed: 268 ins; 193 del; 76 mod Patch: https://git.openjdk.java.net/jdk/pull/1393.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1393/head:pull/1393 PR: https://git.openjdk.java.net/jdk/pull/1393
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v2]
> 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: Review changes @PaulSandoz and @rogermb - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/2b3e4ed7..9d6d1a94 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=00-01 Stats: 18 lines in 4 files changed: 2 ins; 5 del; 11 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: 8248862: Implement Enhanced Pseudo-Random Number Generators
On Fri, 20 Nov 2020 23:30:03 GMT, Roger Baumgartner 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 > > src/java.base/share/classes/java/util/random/package-info.java line 149: > >> 147: * >> 148: * For an application running in a 32-bit hardware environment and >> using >> 149: * only one thread or a small number of threads, may be a good choice. > > I think the name of the suitable algorithm is missing here. Yes - "L32X64MixRandom". Thank you. - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 18 Nov 2020 00:29:36 GMT, Paul Sandoz wrote:
>> Jim Laskey has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 40 commits:
>>
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Update package-info.java
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated RandomGeneratorFactory javadoc.
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated documentation for RandomGeneratorFactory.
>> - Merge branch 'master' into 8248862
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Move RandomGeneratorProperty
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Clear up javadoc
>> - 8248862; Implement Enhanced Pseudo-Random Number Generators
>>
>>remove RandomGeneratorProperty from API
>> - ... and 30 more:
>> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 173:
>
>> 171: @SuppressWarnings("unchecked")
>> 172: private Map getProperties() {
>> 173: if (properties == null) {
>
> `properties` needs to be marked volatile, and it needs to be assigned at line
> 182 or line 184.
One of them foggy days.
-
PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 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. CSR: https://bugs.openjdk.java.net/browse/JDK-8256866 specdiff: http://cr.openjdk.java.net/~mcimadamore/8256865_v1/specdiff_out/overview-summary.html javadoc: http://cr.openjdk.java.net/~mcimadamore/8256865_v1/javadoc - PR: https://git.openjdk.java.net/jdk/pull/1388
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Mon, 23 Nov 2020 14:57:59 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/module-info.java line 250:
>>
>>> 248: exports jdk.internal.util.xml.impl to
>>> 249: jdk.jfr;
>>> 250: exports jdk.internal.util.random;
>>
>> Unqualified export, should this be `to jdk.random`?
>
> I guess you are right. Until we have a defined SPI we should restrict.
On the other hand:
public class Random extends AbstractSpliteratorGenerator
^
error: warnings found and -Werror specified
public final class SplittableRandom extends AbstractSplittableGenerator {
-
PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Tue, 17 Nov 2020 23:46:12 GMT, Paul Sandoz wrote:
>> Jim Laskey has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 40 commits:
>>
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Update package-info.java
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated RandomGeneratorFactory javadoc.
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated documentation for RandomGeneratorFactory.
>> - Merge branch 'master' into 8248862
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Move RandomGeneratorProperty
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Clear up javadoc
>> - 8248862; Implement Enhanced Pseudo-Random Number Generators
>>
>>remove RandomGeneratorProperty from API
>> - ... and 30 more:
>> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68
>
> src/jdk.random/share/classes/module-info.java line 53:
>
>> 51: uses RandomSupport;
>> 52:
>> 53: exports jdk.random to
>
> Why is this needed?
Removing
> src/java.base/share/classes/java/util/random/package-info.java line 50:
>
>> 48: * given its name.
>> 49: *
>> 50: * The principal supporting class is {@link RandomGenertatorFactor}.
>> This
>
> s/RandomGenertatorFactor/RandomGenertatorFactory
fixing
> src/java.base/share/classes/java/util/random/package-info.java line 140:
>
>> 138: *
>> 139: * For applications with no special requirements,
>> 140: * "L64X128MixRandom" has a good balance among speed, space,
>
> The documentation assumes that the `jdk.random` module is present in the JDK
> image. Perhaps we need to spit the specifics to `jdk.random`?
But jdk.random isn't really a public API.
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
> 1211:
>
>> 1209: Udiff = -Udiff;
>> 1210: U2 = U1;
>> 1211: U1 -= Udiff;
>
> Updated `U1` never used (recommend running the code through a checker e.g.
> use IntelliJ)
I noticed that before. I think it's a symmetry thing - will check with Guy.
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
> 1157:
>
>> 1155: /*
>> 1156: * The tables themselves, as well as a number of associated
>> parameters, are
>> 1157: * defined in class java.util.DoubleZigguratTables, which is
>> automatically
>
> `DoubleZigguratTables` is an inner class of `RandomSupport`
Late change fixing.
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
> 2895:
>
>> 2893: * distribution: 0.0330
>> 2894: */
>> 2895: static class DoubleZigguratTables {
>
> make `final`
fixing
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
> 167:
>
>> 165: * Return the properties map for the specified provider.
>> 166: *
>> 167: * @param provider provider to locate.
>
> Method has no such parameter
Fixing
-
PR: https://git.openjdk.java.net/jdk/pull/1273
RFR: 8256865: Foreign Memory Access and Linker API are missing NPE checks
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. - Commit messages: - Add NPE checks in foreign ABI - Merge branch 'master' into npe-fixes - Fix memory access NPEs cont'd - Fix memory access NPEs - Merge branch 'master' into npe-fixes - make base() method abstract in HeapMemorySegmentImpl - Create specialized subclasses for all the heap segment cases Changes: https://git.openjdk.java.net/jdk/pull/1388/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1388=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256865 Stats: lines in 32 files changed: 939 ins; 137 del; 35 mod Patch: https://git.openjdk.java.net/jdk/pull/1388.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1388/head:pull/1388 PR: https://git.openjdk.java.net/jdk/pull/1388
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification
On Mon, 23 Nov 2020 01:43:39 GMT, Kim Barrett wrote: > Please review this change to Reference.clear() to address several issues. > > (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent > field to null may extend the lifetime of the referent value. > > (JDK-8240696) For GCs with concurrent reference processing, clearing the > referent field during reference processing may discard the expected > notification. > > Both of these are addressed by introducing a private native helper function > for clearing the referent, rather than using an ordinary in-Java field > assignment. Tests have been added for both of these issues. This required > adding a new breakpoint in reference processing for ZGC. > > Of course, finalization adds some complexity to the problem. We deal with > that by having FinalReference override clear. The implementation is > provided by a new package-private method in Reference. (There are a number > of alternatives, all of them clumsy; finalization is annoying that way.) > > While dealing with FinalReference clearing it was noted that the recent > JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not > updated to call the new Reference.getInactive(), instead still calling get() > on FinalReferences, with the JDK-8256106 problems. Fixing that showed the > assertion for inactive FinalReference added by JDK-8256370 used the wrong > test. > > Rather than tracking down and changing all get() and clear() calls on final > references and changing them to use getInactive and a new similar clear > function, I've changed FinalReference to override get and clear, which call > the helper functions in Reference. I've also renamed getInactive to be more > explanatory and less convenient to call directly, and similarly named the > helper for clear. This means that get/clear should never be called on an > active FinalReference. That's already never done, and would have problems > if it were. > > Testing: > mach5 tier1-6 > Local (linux-x64) tier1 using Shenandoah. > New TestReferenceClearDuringMarking fails for G1 without these changes. > New TestReferenceClearDuringReferenceProcessing fails for ZGC without these > changes. Looks good, except one small nit in one of the test configs. test/hotspot/jtreg/gc/TestReferenceClearDuringReferenceProcessing.java line 28: > 26: /* @test > 27: * @bug 8256517 > 28: * @requires vm.gc.Z Please add | vm.gc.Shenandoah here. - Changes requested by rkennke (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Tue, 17 Nov 2020 21:22:28 GMT, Paul Sandoz wrote:
>> Jim Laskey has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 40 commits:
>>
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Update package-info.java
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated RandomGeneratorFactory javadoc.
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Updated documentation for RandomGeneratorFactory.
>> - Merge branch 'master' into 8248862
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Move RandomGeneratorProperty
>> - Merge branch 'master' into 8248862
>> - 8248862: Implement Enhanced Pseudo-Random Number Generators
>>
>>Clear up javadoc
>> - 8248862; Implement Enhanced Pseudo-Random Number Generators
>>
>>remove RandomGeneratorProperty from API
>> - ... and 30 more:
>> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68
>
> src/java.base/share/classes/java/util/Random.java line 592:
>
>> 590:
>> 591: @Override
>> 592: public Spliterator.OfInt makeIntsSpliterator(long index, long
>> fence, int origin, int bound) {
>
> Unsure if this and the other two methods are intended to be public or not,
> since they are at the end of the class and override methods of a module
> private class. In principle there is nothing wrong with such `Spliterator`
> factories, but wonder if they are really needed given the `Stream` returning
> methods. The arrangement of classes makes it awkward to hide these methods.
Re the properties general comment: I moved properties to RandomSupport based on
the notion that the SPI work with come later.
Re makeIntsSpliterator: These methods aren't exposed in the java.util.Random
API I guess no harm done. The only solution I can think of is to create an
intermediate implementor, but that leaves the methods exposed as well.
> src/java.base/share/classes/java/util/SplittableRandom.java line 171:
>
>> 169: * RandomGenerator properties.
>> 170: */
>> 171: static Map getProperties() {
>
> With records exiting preview in 16 this map of properties could i think be
> represented as a record instance, with better type safety, where
> `RandomSupport.RandomGeneratorProperty` enum values become typed fields
> declared on the record class. Something to consider after integration perhaps?
Yes.
> src/java.base/share/classes/java/util/SplittableRandom.java line 211:
>
>> 209: *
>> http://zimbry.blogspot.com/2011/09/better-bit-mixing-improving-on.html
>> 210: */
>> 211: private static long mix64(long z) {
>
> Usages be replaced with calls to `RandomSupport.mixStafford13`?
We were careful to not change the sequences (from fixed seed) generated by
existing prngs. This was an edge case.
> src/java.base/share/classes/module-info.java line 250:
>
>> 248: exports jdk.internal.util.xml.impl to
>> 249: jdk.jfr;
>> 250: exports jdk.internal.util.random;
>
> Unqualified export, should this be `to jdk.random`?
I guess you are right. Until we have a defined SPI we should restrict.
-
PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 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. Filed https://bugs.openjdk.java.net/browse/JDK-8256862 to fix the test failures separately. - 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
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.
-
PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8180352: Add Stream.toList() method [v3]
On Sat, 21 Nov 2020 11:22:55 GMT, Rémi Forax
wrote:
>> `listFromTrustedArrayNullsAllowed` is clearly not an option, as it will
>> produce shared-secret leaking (see
>> [JDK-8254090](https://bugs.openjdk.java.net/browse/JDK-8254090) for a
>> similar case). StreamEx solution is dirty as it relies on the implementation
>> detail. I believe, OpenJDK team is not very interested in providing optimal
>> implementations for third-party stream implementations, as third-party
>> implementations will likely update by themselves when necessary. At least,
>> my suggestion to make the default `mapMulti` implementation better was
>> declined.
>
> This implementation duplicates the array twice, once in this.toArray() and
> once in the constructor of ArrayList that calls toArray() on
> Collections.ArrayList.
>
> I'm sure there is a better way
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.
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators
Rémi, > On Nov 21, 2020, at 8:03 AM, Remi Forax wrote: > > Ok, i've taking the time to read some literature about random generators > because for me the Mersenne Twister was still the king. > > The current API proposed as clearly two levels, you have the user level and > the implementation level, at least the implementation level should seen as a > SPI Yes, We agree. It was decided that we work on the SPI separately from the original JEP. IMHO the implementation issues are too complex for a single JEP. So, the goal is to release the user level now, and refine the SPI at a later date. Only RandomGenerator (with descendant interfaces) and RandomGeneratorFactory will be public facing in the first release. > > RandomGenerator is the user facing API, for me it should be the sole > interface exposed by the API, the others (leap, arbitrary leap and split) > should be optional methods. Fair enough, but if your task requires leapable, you might be disappointed when you get an UnsupportedOperationException invoking leap. I personally like the "to the quick" ability of using LeapableGenerator for narrowing down the search. LeapableGenerator leapable = LeapableGenerator.all().findFirst().orElseThrow(); Open for discussion. > > In term of factory methods, we should have user driven methods: > - getDefaultGenerator() that currently returns a L64X128MixRandom and can be > changed in the future > - getFastGenerator() that currently returns a Xoroshiro128PlusPlus and can be > changed in the future > - getDefault[Splitable|Leapable|etc]Generator that returns a default > generator with the methods splits|leaps|etc defined > - of / getByName that returns a specific generator by its name (but mov ed in > a SPI class) I'm concerned that the "can be changed in the future" aspect of your default methods will create a false reliance. We sort of have default now with the java.util.Random class. If we were to change the underpinnings of Random all heck would break loose. We already ran into that aspect during testing - tests that relied on sequences from fixed seeds. We try to discourage the use of 'of', but there is a class of user (machine learning for example) that wants to be able to specify exactly. Often, choosing a specific fast prng for testing and then a more sophisticated one for production. The main purpose for RandomGenerator is swapability. > > The example in the documentation should use getDefaultGenerator() and not > of() to avoid the problem all the programming languages currently have by > having over-specified that the default generator is a Mersenne Twister. I have a problem with this as well. It would make it difficult to deprecate java.util.Random. > > All methods that returns a stream of the available implementations should be > moved in the SPI package. Open for discussion. > > Rémi > Cheers, -- Jim > --- > An honest question, > why do we need so many interfaces for the different categories of > RandomGenerator ? > > My fear is that we are encoding the state of our knowledge of the different > kinds of random generators now so it will not be pretty in the future when > new categories of random generator are discovered/invented. > If we can take example of the past to predict the future, 20 years ago, what > should have been the hierarchy at that time. > Is it not reasonable to think that we will need new kinds of random generator > in the future ? > > I wonder if it's not better to have one interface and several optional > methods like we have with the collections, it means that we are loosing the > possibilities to precisely type a method that only works with a precise type > of generator but it will be more future proof. > > Rémi > > - Mail original - >> De: "Jim Laskey" >> À: "core-libs-dev" , "security-dev" >> >> Envoyé: Mercredi 18 Novembre 2020 14:52:56 >> Objet: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators > >> 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 >> >> - >> >> Commit messages: >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> - 8248862; Implement Enhanced Pseudo-Random Number Generators >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> - 8248862:
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
[Sorry it took so long. Have been on break.] From Guy: Thanks for the forward. Here are my thoughts: Good question from Rémi. If we consider PRNGs to have started at about the time of von Neumann, circa 1946, then I would say that we have been inventing a new category about once every 25 years or so: jumpable, multi-level jumpable, cryptographically secure, splittable. Twenty years ago we would just have one or more levels of jumping/leaping. I think SecureRandom appeared in 2002 (in J2SE 1.4), and the first version of SplittableRandom was in 2014. So I could be wrong, but I really don’t expect to have to add any more interfaces in the next decade or two. I think we will get more benefit from the better type checking than we would get with optional methods. —Guy > On Nov 17, 2020, at 7:18 PM, Remi Forax wrote: > > An honest question, > why do we need so many interfaces for the different categories of > RandomGenerator ? > > My fear is that we are encoding the state of our knowledge of the different > kinds of random generators now so it will not be pretty in the future when > new categories of random generator are discovered/invented. > If we can take example of the past to predict the future, 20 years ago, what > should have been the hierarchy at that time. > Is it not reasonable to think that we will need new kinds of random generator > in the future ? > > I wonder if it's not better to have one interface and several optional > methods like we have with the collections, it means that we are loosing the > possibilities to precisely type a method that only works with a precise type > of generator but it will be more future proof. > > Rémi > > - Mail original - >> De: "Jim Laskey" >> À: "build-dev" , "core-libs-dev" >> , >> security-...@openjdk.java.net >> Envoyé: Mardi 17 Novembre 2020 23:21:18 >> Objet: 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 . >> >> Jim Laskey has updated the pull request with a new target base due to a >> merge or >> a rebase. The pull request now contains 40 commits: >> >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Update package-info.java >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Updated RandomGeneratorFactory javadoc. >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Updated documentation for RandomGeneratorFactory. >> - Merge branch 'master' into 8248862 >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Move RandomGeneratorProperty >> - Merge branch 'master' into 8248862 >> - 8248862: Implement Enhanced Pseudo-Random Number Generators >> >> Clear up javadoc >> - 8248862; Implement Enhanced Pseudo-Random Number Generators >> >> remove RandomGeneratorProperty from API >> - ... and 30 more: >> https://git.openjdk.java.net/jdk/compare/f7517386...6fe94c68 >> >> - >> >> Changes: https://git.openjdk.java.net/jdk/pull/1273/files >> Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1273=02 >> Stats: 14891 lines in 31 files changed: 0 ins; 3704 del; 77 mod >> Patch: https://git.openjdk.java.net/jdk/pull/1273.diff >> Fetch: git fetch https://git.openjdk.java.net/jdk pull/1273/head:pull/1273 >> >> PR: https://git.openjdk.java.net/jdk/pull/1273
RFR: 8256486: Linux/Windows-x86 builds broken after JDK-8254231
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. - Commit messages: - 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 Changes: https://git.openjdk.java.net/jdk/pull/1266/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1266=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256486 Stats: 95 lines in 11 files changed: 55 ins; 26 del; 14 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: 8256517: (ref) Reference.clear during reference processing may lose notification
On Mon, 23 Nov 2020 01:43:39 GMT, Kim Barrett wrote:
> Please review this change to Reference.clear() to address several issues.
>
> (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent
> field to null may extend the lifetime of the referent value.
>
> (JDK-8240696) For GCs with concurrent reference processing, clearing the
> referent field during reference processing may discard the expected
> notification.
>
> Both of these are addressed by introducing a private native helper function
> for clearing the referent, rather than using an ordinary in-Java field
> assignment. Tests have been added for both of these issues. This required
> adding a new breakpoint in reference processing for ZGC.
>
> Of course, finalization adds some complexity to the problem. We deal with
> that by having FinalReference override clear. The implementation is
> provided by a new package-private method in Reference. (There are a number
> of alternatives, all of them clumsy; finalization is annoying that way.)
>
> While dealing with FinalReference clearing it was noted that the recent
> JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not
> updated to call the new Reference.getInactive(), instead still calling get()
> on FinalReferences, with the JDK-8256106 problems. Fixing that showed the
> assertion for inactive FinalReference added by JDK-8256370 used the wrong
> test.
>
> Rather than tracking down and changing all get() and clear() calls on final
> references and changing them to use getInactive and a new similar clear
> function, I've changed FinalReference to override get and clear, which call
> the helper functions in Reference. I've also renamed getInactive to be more
> explanatory and less convenient to call directly, and similarly named the
> helper for clear. This means that get/clear should never be called on an
> active FinalReference. That's already never done, and would have problems
> if it were.
>
> Testing:
> mach5 tier1-6
> Local (linux-x64) tier1 using Shenandoah.
> New TestReferenceClearDuringMarking fails for G1 without these changes.
> New TestReferenceClearDuringReferenceProcessing fails for ZGC without these
> changes.
Looks good. Just want to request that you also remove the following comment in
zReferenceProcessor.cpp, as it's no longer true.
--- a/src/hotspot/share/gc/z/zReferenceProcessor.cpp
+++ b/src/hotspot/share/gc/z/zReferenceProcessor.cpp
@@ -184,12 +184,6 @@ bool ZReferenceProcessor::should_discover(oop reference,
ReferenceType type) con
}
bool ZReferenceProcessor::should_drop(oop reference, ReferenceType type) const
{
- // This check is racing with a call to Reference.clear() from the
application.
- // If the application clears the reference after this check it will still end
- // up on the pending list, and there's nothing we can do about that without
- // changing the Reference.clear() API. This check is also racing with a call
- // to Reference.enqueue() from the application, which is unproblematic, since
- // the application wants the reference to be enqueued anyway.
const oop referent = reference_referent(reference);
if (referent == NULL) {
// Reference has been cleared, by a call to Reference.enqueue()
-
Changes requested by pliden (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1376
Integrated: 8256154: Some TestNG tests require default constructors
On Thu, 19 Nov 2020 13:50:30 GMT, Conor Cleary wrote:
> In TestNG 7, it is a requirement that TestNG is able to create a Test object
> using a default constructor.
>
> This simple fix addresses two such classes so that this requirement is
> satisfied by inserting default construtors. Example: `public GetPackages() {
> ... }`
>
> test/jdk/java/lang/Package/GetPackages.java
> test/jdk/java/lang/StackWalker/Basic.java
This pull request has now been integrated.
Changeset: 5ed70448
Author:Conor Cleary
Committer: Lance Andersen
URL: https://git.openjdk.java.net/jdk/commit/5ed70448
Stats: 13 lines in 2 files changed: 11 ins; 0 del; 2 mod
8256154: Some TestNG tests require default constructors
Reviewed-by: dfuchs, bpb
-
PR: https://git.openjdk.java.net/jdk/pull/1317
Integrated: 8254231: Implementation of Foreign Linker API (Incubator)
On Tue, 13 Oct 2020 13:08:14 GMT, Maurizio Cimadamore wrote: > This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and associated pull request [3]). > > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate JNI glue code. In order to do > this, native calls are modeled through the MethodHandle API. I suggest > reading the writeup [4] I put together few weeks ago, which illustrates what > the foreign linker support is, and how it should be used by clients. > > Disclaimer: the pull request mechanism isn't great at managing *dependent* > reviews. For this reasons, I'm attaching a webrev which contains only the > differences between this PR and the memory access PR. I will be periodically > uploading new webrevs, as new iterations come out, to try and make the life > of reviewers as simple as possible. > > A big thank to Jorn Vernee and Vladimir Ivanov - they are the main architects > of all the hotspot changes you see here, and without their help, the foreign > linker support wouldn't be what it is today. As usual, a big thank to Paul > Sandoz, who provided many insights (often by trying the bits first hand). > > Thanks > Maurizio > > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library by name, or absolute path, > and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory layouts for the function > arguments/return type. A function descriptor is used to describe the > signature of a native function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes a native symbol (as > obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` and > returns a `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, and a > `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a > code stub allocated by the VM which acts as a trampoline from native code to > the user-provided method handle. This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures (e.g. `C_LONG` and > friends); these layouts contain additional ABI classfication information (in > the form of layout attributes) which is used by the runtime to *infer* how > Java arguments should be shuffled for the native call to take place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than allocating separate memory segments > using separate *try-with-resource* constructs, a `NativeScope` allows clients > to use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a performance boost, since not all > allocation requests will be turned into `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For instance, the description of the > native signature might be wrong (e.g. have too many arguments) - and the > runtime has, in the general case, no way to detect such mismatches. For these > reasons, obtaining a `CLinker` instance is a *restricted* operation, which > can be enabled by specifying the usual JDK property > `-Dforeign.restricted=permit` (as it's the case for other restricted method > in the foreign memory API). > > ### Implementation changes > > The Java changes associated with `LibraryLookup` are relative > straightforward; the only interesting thing to note here is that library > loading does _not_ depend on class loaders, so `LibraryLookup` is not subject > to the same
Re: RFR: 8256370: Add asserts to Reference.getInactive()
On Sun, 22 Nov 2020 22:15:20 GMT, Kim Barrett wrote: > I didn't notice this before it was integrated. > > The test for inactive isn't right; rather than `next == this` it > should be `next != null`. This becomes apparent once > FinalizerHistogram is fixed to call getInactive() rather than get(). > > I noticed this while working on JDK-8256517, where I ran into some > similar issues. I will address these problems as part of that change. Oh ok. Thanks for taking care of it! - PR: https://git.openjdk.java.net/jdk/pull/1231
Re: RFR: 8230501: Class data support for hidden classes [v4]
On Thu, 19 Nov 2020 11:08:26 GMT, Jorn Vernee wrote: >> Mandy Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix the name passed to condy calling classData > > Left 2 minor comments on the new additions in line. It is my understanding that `Lookup` object returned from defineHiddenClass[WithClassData] features the ORIGINAL access lookup mode, right? I cannot find a normative statement to confirm this. If it is the case, then it would be good to clarify this. - PR: https://git.openjdk.java.net/jdk/pull/1171
