Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]
On Wed, 21 Oct 2020 02:28:30 GMT, Kim Barrett wrote: >> Finally returning to this review that was started in April 2020. I've >> recast it as a github PR. I think the security concern raised by Gil >> has been adequately answered. >> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html >> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html >> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html >> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html >> >> Please review a new function: java.lang.ref.Reference.refersTo. >> >> This function is needed to test the referent of a Reference object without >> artificially extending the lifetime of the referent object, as may happen >> when calling Reference.get. Some garbage collectors require extending the >> lifetime of a weak referent when accessed, in order to maintain collector >> invariants. Lifetime extension may occur with any collector when the >> Reference is a SoftReference, as calling get indicates recent access. This >> new function also allows testing the referent of a PhantomReference, which >> can't be accessed by calling get. >> >> The new function uses native methods whose implementations are in the VM so >> they can use the Access API. It is the intent that these methods will be >> intrinsified by optimizing compilers like C2 or graal, but that hasn't been >> implemented yet. Bear that in mind before rushing off to change existing >> uses of Reference.get. >> >> There are two native methods involved, one in Reference and an override in >> PhantomReference, both package private in java.lang.ref. The reason for this >> split is to simplify the intrinsification. This is a change from the version >> from April 2020; that version had a single native method in Reference, >> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category. >> However, adding support for that category in the compilers adds significant >> implementation effort and complexity. Splitting avoids that complexity. >> >> Testing: >> mach5 tier1 >> Locally (linux-x64) verified the new test passes with various garbage >> collectors. > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > improve wording in refersTo javadoc Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/498
Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]
On Sat, 24 Oct 2020 22:22:56 GMT, Peter Levart wrote: >> Reference instances should not be leaked and so I don't see very common that >> caller of `Reference::get` does not know the referent's type. It also >> depends on the `refersTo` check against `null` vs an object. Any known use >> case would be helpful if any (some existing code that wants to call >> `refersTo` to compare a `Reference` of raw type with an object of unknown >> type). >> >> FWIW, when converting a few use of `Reference::get` to `refersTo` in JDK, >> there is only one case (`equals(Object o)` method that needs the cast. >> >> http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8188055/jdk-use-refersTo/index.html > > @mlchung I don't have many known use cases, but how about > WeakHashMap.containsKey(Object key) for example? Currently > `WeakHashMap.Entry extends WeakReference` but it would be more > type safe if it extended `WeakReference`. In that case an > `entry.refersTo(key)` would not compile... > What I'm trying to say is that even if `Reference` instances are not > "leaked", you can get an untyped object reference from outside and you may > want to identity-compare it with the Reference's referent. Some thoughts regarding the parameter type of refersTo. Summary: I think `refersTo(T)` is fine and that we don't want to change it to `refersTo(Object)`. I don't think we have a migration issue similar to generifying collections, where there was a possibility of changing `contains(Object)` to `contains(E)`. If that had been done, it would have been a source compatibility issue, because changing the signature of the method potentially affects existing code that calls the method. That doesn't apply here because we're adding a new method. The question now falls to whether it's preferable to have more convenience with `refersTo(Object)` or more type-safety with `refersTo(T)`. With the generic collections issue, the migration issue probably drove the decision to keep `contains(Object)`, but this has resulted in a continual set of complaints about the lack of an error when code passes an instance of the "wrong" type. I think that kind of error is likely to occur with `refersTo`. Since we don't have a source compatibility issue here, we can choose the safer API and avoid this kind of problem entirely. The safer API does raise the possibility of having to add inconvenient unchecked casts and local variables in certain places, but I think Mandy's comment about the code already having a reference of the "right" type is correct. Her prototype webrev linked above shows that having to add unchecked casts is fairly infrequent. - PR: https://git.openjdk.java.net/jdk/pull/498
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v17]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can largely ignore this interface, which comes in > really handy when
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Tue, 27 Oct 2020 14:04:04 GMT, Andrew Haley wrote: >> Bernhard Urban-Forster has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - uppercase suffix >> - add assert > > Marked as reviewed by aph (Reviewer). Thank you for the reviews, Magnus and Andrew! - PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Fri, 23 Oct 2020 17:22:33 GMT, Jonathan Gibbons wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Removing unnecessary cast.
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java
> line 88:
>
>> 86:
>> 87: @Override
>> 88: protected Content getDeprecatedOrPreviewLink(Element member) {
>
> This name is a side-effect of the `ElementFlag` question. We should either
> use explicit field and method names, or we should use `ElementFlag` more
> consistently. This method name works OK for now, but if if ever gets to
> have another `orFoo` component in the name, the signature should be
> parameterized with something like `ElementFlag` or its equivalent.
Note this method returns the same link for deprecate or preview - it just was
named "getDeprecatedLink", and when using it work preview, I renamed it
"getDeprecatedOrPreviewLink". We may need to think of a better name.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java
> line 2980:
>
>> 2978: }
>> 2979:
>> 2980: public enum DeclarationPreviewLanguageFeatures {
>
> General thinking aloud question ... how does all this interact with source or
> release options for an earlier release?
I don't think there should be much interaction with -source . We
don't support preview features from previous version or preview class files
from previous versions, so I think it should be enough to handle the currently
present preview features.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java
> line 2984:
>
>> 2982: SEALED(List.of("sealed")),
>> 2983: SEALED_PERMITS(List.of("sealed", "permits")),
>> 2984: RECORD(List.of("record"));
>
> I'm guessing this is about to go away soon?
Right, this is likely to go away soon.
-
PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v16]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can largely ignore this interface, which comes in > really handy when
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
On Wed, 7 Oct 2020 17:13:22 GMT, Maurizio Cimadamore wrote: > This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v15]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can largely ignore this interface, which comes in > really handy when
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
On Wed, 7 Oct 2020 17:13:22 GMT, Maurizio Cimadamore wrote: > This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Thu, 15 Oct 2020 18:35:30 GMT, Bernhard Urban-Forster
wrote:
>> I organized this PR so that each commit contains the warning emitted by MSVC
>> as commit message and its relevant fix.
>>
>> Verified on
>> * Linux+ARM64: `{hotspot,jdk,langtools}:tier1`, no failures.
>> * Windows+ARM64: `{hotspot,jdk,langtools}:tier1`, no (new) failures.
>> * internal macOS+ARM64 port: build without `--disable-warnings-as-errors`
>> still works. Just mentioning this here, because it's yet another toolchain
>> (Xcode / clang) that needs to be kept happy [going
>> forward](https://openjdk.java.net/jeps/391).
>
> Bernhard Urban-Forster has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - uppercase suffix
> - add assert
Marked as reviewed by aph (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/530
Integrated: 8255305: Add Linux x86_32 tier1 to submit workflow
On Fri, 23 Oct 2020 10:04:49 GMT, Aleksey Shipilev wrote: > Following JDK-8254282, it seems useful to also run tier1 tests on 32-bit > platform, because that finds problems with runtime tests and internal > asserts. Those problems do not show up during the plain build. Again, while > x86_32 might not be widely deployed by itself, but 32/64 bit cleanliness > detects bugs that manifest on ARM32 early. > > I did most of the work to fix all current regressions in x86_32 tier1, the > only exception is JDK-8255331, which is supposed to be fixed by #833 This pull request has now been integrated. Changeset: 552192ff Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/552192ff Stats: 199 lines in 1 file changed: 191 ins; 0 del; 8 mod 8255305: Add Linux x86_32 tier1 to submit workflow Reviewed-by: ihse - PR: https://git.openjdk.java.net/jdk/pull/830
Integrated: 8252113: Move jfr man page into jfr module
On Tue, 27 Oct 2020 12:50:39 GMT, Magnus Ihse Bursie wrote: > Man pages should be placed in the same module as the corresponding > executable. For some reason, this was done incorrectly for jfr, where the > executable is provided by jdk.jfr, but the man page was in java.base. This pull request has now been integrated. Changeset: 504cb005 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/504cb005 Stats: 0 lines in 1 file changed: 0 ins; 0 del; 0 mod 8252113: Move jfr man page into jfr module Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/879
Re: RFR: 8255305: Add Linux x86_32 tier1 to submit workflow [v2]
On Tue, 27 Oct 2020 13:32:41 GMT, Magnus Ihse Bursie wrote: >> Testing is still clean apart for missing #833 change. Okay to push then? > > I think you should wait for the problem list. Otherwise you'll introduce > intentional test breakage in GH actions. #833 is now merged, I am pushing this to get other PRs retested with x86_32. - PR: https://git.openjdk.java.net/jdk/pull/830
Withdrawn: 8255412: "Linux x32" should be "Linux x86" in submit workflow
On Mon, 26 Oct 2020 18:45:31 GMT, Aleksey Shipilev wrote: > John Paul Adrian Glaubitz mentions "x32" is actually the designation for > "x86_64 with 32-bit pointers". Let's rename "Linux x32" to "Linux x86". I > also realized that JDK-8254282 missed "Linux x32" for the platform selection. > While you can guess it is accepted from reading the workflow, it would be > more convenient to have it in the default argument list. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8255305: Add Linux x86_32 tier1 to submit workflow [v2]
On Tue, 27 Oct 2020 13:22:49 GMT, Aleksey Shipilev wrote: >> Looks good to me now. Thank you! > > Testing is still clean apart for missing #833 change. Okay to push then? I think you should wait for the problem list. Otherwise you'll introduce intentional test breakage in GH actions. - PR: https://git.openjdk.java.net/jdk/pull/830
Re: RFR: 8255305: Add Linux x86_32 tier1 to submit workflow [v2]
On Tue, 27 Oct 2020 10:49:12 GMT, Magnus Ihse Bursie wrote: >> Aleksey Shipilev has updated the pull request incrementally with four >> additional commits since the last revision: >> >> - Rename "x32" -> "x86" in the test steps >> - Merge branch 'JDK-8255412-add-x32-default' into JDK-8255305-32bit-tier1 >> - Rename x32 -> x86. >> - 8255412: Add "Linux x32" to platform selection default in custom submit >> workflow > > Looks good to me now. Thank you! Testing is still clean apart for missing #833 change. Okay to push then? - PR: https://git.openjdk.java.net/jdk/pull/830
make test TEST="micro:" got java.lang.UnsupportedClassVersionError
Hi, I follow the instruction on https://htmlpreview.github.io/?https://github.com/openjdk/jdk/blob/master/doc/testing.html#microbenchmarks to run a new microbenchmark. I got error message as follows. java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/java/lang/reflect/proxy/jmh_generated/ProxyBench_newProxyInstance1i_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1010) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:855) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:753) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:676) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:634) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:182) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:519) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:232) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:138) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:75) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) If I add an extra flag “—enable-preview” , the micro bench will work. make test TEST="micro:java.lang.reflect" MICRO="VM_OPTIONS=--enable-preview" I am using jmh from make/devkit/createJMHBundle.sh, which gives me jmh-core-1.26.jar Is that jmh-core too old or we should adjust RunTest.gmk a little bit to have that flag? Does Openjdk CI run benchmarks to detect performance regression? If so, could you point me to a guideline of microbenmark? I lack of the basic sense of a “good” microbenchmark. My concern is the new micro I added is too long to break the established test run. Thanks, --lx
Re: RFR: 8252113: Move jfr man page into jfr module
On Tue, 27 Oct 2020 12:50:39 GMT, Magnus Ihse Bursie wrote: > Man pages should be placed in the same module as the corresponding > executable. For some reason, this was done incorrectly for jfr, where the > executable is provided by jdk.jfr, but the man page was in java.base. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/879
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v14]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can largely ignore this interface, which comes in > really handy when
RFR: 8252113: Move jfr man page into jfr module
Man pages should be placed in the same module as the corresponding executable. For some reason, this was done incorrectly for jfr, where the executable is provided by jdk.jfr, but the man page was in java.base. - Commit messages: - 8252113: Move jfr man page into jfr module Changes: https://git.openjdk.java.net/jdk/pull/879/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=879=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8252113 Stats: 0 lines in 1 file changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/879.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/879/head:pull/879 PR: https://git.openjdk.java.net/jdk/pull/879
Re: make test TEST="micro:" got java.lang.UnsupportedClassVersionError
Hi lx, the need to add --enable-preview is known, and it's a bug introduced a couple of months ago thanks to a suggestion I did (without realizing that compiling one micro with --enable-preview would poison all of them..). This issue is tracked by a few bugs, see https://bugs.openjdk.java.net/browse/JDK-8250669 and https://bugs.openjdk.java.net/browse/JDK-8253828 The fix will likely be to remove the --enable-preview from the micros build after Records goes out of preview and then think more carefully on how to add support for benchmarking preview features. As to your other question: When adding a microbenchmark it's appreciated if the default configuration makes it as quick to run as possible without sacrificing quality. Often the default number of forks, iteration runtime et.c. can be tuned down by using custom settings in @Fork et.c. Custom parameters used can be limited to a select handful. Still it's normal for a "good" microbenchmark to require at least five, usually ten minutes per @Benchmark to produce reasonably trustworthy results. I can't speak for what others do, but we (Oracle) select what to run regularly from the available microbenchmarks at our discretion, though. That means adding a microbenchmark that takes a prohibitively long time won't be disruptive to our nightly testing, but for obvious (mostly economic) reasons we'd be unlikely to add an extremely long-running micro to regular coverage. /Claes On 2020-10-27 10:06, Liu, Xin wrote: Hi, I follow the instruction on https://htmlpreview.github.io/?https://github.com/openjdk/jdk/blob/master/doc/testing.html#microbenchmarks to run a new microbenchmark. I got error message as follows. java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/java/lang/reflect/proxy/jmh_generated/ProxyBench_newProxyInstance1i_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1010) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:855) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:753) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:676) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:634) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:182) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:519) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:232) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:138) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:75) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) If I add an extra flag “—enable-preview” , the micro bench will work. make test TEST="micro:java.lang.reflect" MICRO="VM_OPTIONS=--enable-preview" I am using jmh from make/devkit/createJMHBundle.sh, which gives me jmh-core-1.26.jar Is that jmh-core too old or we should adjust RunTest.gmk a little bit to have that flag? Does Openjdk CI run benchmarks to detect performance regression? If so, could you point me to a guideline of microbenmark? I lack of the basic sense of a “good” microbenchmark. My concern is the new micro I added is too long to break the established test run. Thanks, --lx
Re: RFR: 8255305: Add Linux x86_32 tier1 to submit workflow [v2]
On Tue, 27 Oct 2020 10:42:36 GMT, Aleksey Shipilev wrote: >> Following JDK-8254282, it seems useful to also run tier1 tests on 32-bit >> platform, because that finds problems with runtime tests and internal >> asserts. Those problems do not show up during the plain build. Again, while >> x86_32 might not be widely deployed by itself, but 32/64 bit cleanliness >> detects bugs that manifest on ARM32 early. >> >> I did most of the work to fix all current regressions in x86_32 tier1, the >> only exception is JDK-8255331, which is supposed to be fixed by #833 > > Aleksey Shipilev has updated the pull request incrementally with four > additional commits since the last revision: > > - Rename "x32" -> "x86" in the test steps > - Merge branch 'JDK-8255412-add-x32-default' into JDK-8255305-32bit-tier1 > - Rename x32 -> x86. > - 8255412: Add "Linux x32" to platform selection default in custom submit > workflow Looks good to me now. Thank you! - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/830
Re: RFR: 8255305: Add Linux x86_32 tier1 to submit workflow [v2]
On Tue, 27 Oct 2020 10:36:08 GMT, Magnus Ihse Bursie wrote: >> Aleksey Shipilev has updated the pull request incrementally with four >> additional commits since the last revision: >> >> - Rename "x32" -> "x86" in the test steps >> - Merge branch 'JDK-8255412-add-x32-default' into JDK-8255305-32bit-tier1 >> - Rename x32 -> x86. >> - 8255412: Add "Linux x32" to platform selection default in custom submit >> workflow > > Please incorporate the additional fixes in > https://github.com/openjdk/jdk/pull/869 into this PR. That also means that > the new code here that mentions "linux-x32" needs to be renamed. After that, > you can close #869. While I am still not sure folding these two together is perfect, did so! Please re-review, @magicus. (Now to wait and see that testing is still fine). - PR: https://git.openjdk.java.net/jdk/pull/830
Re: RFR: 8255305: Add Linux x86_32 tier1 to submit workflow [v2]
> Following JDK-8254282, it seems useful to also run tier1 tests on 32-bit > platform, because that finds problems with runtime tests and internal > asserts. Those problems do not show up during the plain build. Again, while > x86_32 might not be widely deployed by itself, but 32/64 bit cleanliness > detects bugs that manifest on ARM32 early. > > I did most of the work to fix all current regressions in x86_32 tier1, the > only exception is JDK-8255331, which is supposed to be fixed by #833 Aleksey Shipilev has updated the pull request incrementally with four additional commits since the last revision: - Rename "x32" -> "x86" in the test steps - Merge branch 'JDK-8255412-add-x32-default' into JDK-8255305-32bit-tier1 - Rename x32 -> x86. - 8255412: Add "Linux x32" to platform selection default in custom submit workflow - Changes: - all: https://git.openjdk.java.net/jdk/pull/830/files - new: https://git.openjdk.java.net/jdk/pull/830/files/d8e687a9..ff6ca97d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=830=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=830=00-01 Stats: 29 lines in 1 file changed: 0 ins; 0 del; 29 mod Patch: https://git.openjdk.java.net/jdk/pull/830.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/830/head:pull/830 PR: https://git.openjdk.java.net/jdk/pull/830
Re: RFR: 8255412: "Linux x32" should be "Linux x86" in submit workflow
On Tue, 27 Oct 2020 09:38:41 GMT, Aleksey Shipilev wrote: >> Do you mean that you consider #505 and #548 dependent on this bug? If so, I >> think you misunderstand the purpose of the Github submit testing hook. You >> can -- and should! -- still do proper testing just as you did before the >> migration to Github. The Github action hook is a convenience. >> >> Unless there is a breakage or a regression, I don't think we should rush >> patches in. It's better to get things right. You say "then do everything >> else". Are there more functionality regarding x86 that you would like to see >> integrated in the Github submit hook? If so, I think it's better that you >> present it now, than just a piece at a time. >> >>> I believe the choice between "making the test code sparkling clean in one >>> go" and "making the product code sparking clean in one go" is obvious. >> >> I don't even understand what you mean by this. >> >>> These changes are logically independent. >> >> No, they are not. For one thing, this patch would explicitly introduce the >> "x32" naming, something JDK-8255305 is set. to remove. And this is just a >> trivial indication that both issues revolves around giving proper support >> for x86 in the submit flow. >> >> Just to be clear: I do not object to making x86 a first-class citizen in the >> submit hook! What I *do* object to is this piece-meal integration of two, or >> worse, three, PRs for what should have been one comprehensive PR. > >> Do you mean that you consider #505 and #548 dependent on this bug? If so, I >> think you misunderstand the purpose of the Github submit testing hook. You >> can -- and should! -- still do proper testing just as you did before the >> migration to Github. The Github action hook is a convenience. > > Oh yes, I must be misunderstanding this. Because I thought pre-submit tests > were the replacement for jdk-submit functionality, which was the Oracle's > requirement for push to shared code. Good to know it had been demoted to mere > suggestion. > >> Unless there is a breakage or a regression, I don't think we should rush >> patches in. It's better to get things right. You say "then do everything >> else". Are there more functionality regarding x86 that you would like to see >> integrated in the Github submit hook? If so, I think it's better that you >> present it now, than just a piece at a time. > > No, I mean to stop at `tier1` for x86_32. "Do everything else" means doing > the cosmetics like in this PR that does not affect the actual testing. > >> > I believe the choice between "making the test code sparkling clean in one >> > go" and "making the product code sparking clean in one go" is obvious. >> >> I don't even understand what you mean by this. > > I mean that bikeshedding the testing code means holding off the testing of > the product code. Making sure that product code is correct on x86_32 is more > important than making sure the testing code is cosmetically clean. > >> > These changes are logically independent. >> >> No, they are not. For one thing, this patch would explicitly introduce the >> "x32" naming, something JDK-8255305 is set. to remove. And this is just a >> trivial indication that both issues revolves around giving proper support >> for x86 in the submit flow. > > JDK-8255305 (#830) is set to add another instance of "x32", to already > existing "x32" in the build steps. That means #830 yields a logically > consistent `submit.yml`: it builds as "x32", it tests as "x32". This patch is > set to rename "x32" -> "x86" (*if*, *IF* we agree that rename even makes > sense!), yielding another logically consistent `submit.yml`. In this sense, > they are logically independent. > >> Just to be clear: I do not object to making x86 a first-class citizen in the >> submit hook! What I _do_ object to is this piece-meal integration of two, or >> worse, three, PRs for what should have been one comprehensive PR. > > There are "Request Changes" in #830 that you can use as Reviewer. I would > then oblige, but also reference this discussion. Because the next thing I > expect is someone else to come along and ask why "x32" -> "x86" rename is not > done separately from the addition of the actual testing. I would make that > comment myself for someone else's PR, because I prefer changes to do one > thing at a time. > @shipilev My suggestion is that you take the commits from this PR and fold > them into #830, and update the code added in #830 to match the naming. If you > do that, I'll accept your patch. Thanks. Again: please record this suggestion as formal "request changes" in #830, so that #830 PR history is consistent and clear to outside observer without reading the whole thread. - PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8255412: "Linux x32" should be "Linux x86" in submit workflow
On Tue, 27 Oct 2020 09:38:41 GMT, Aleksey Shipilev wrote: >> Do you mean that you consider #505 and #548 dependent on this bug? If so, I >> think you misunderstand the purpose of the Github submit testing hook. You >> can -- and should! -- still do proper testing just as you did before the >> migration to Github. The Github action hook is a convenience. >> >> Unless there is a breakage or a regression, I don't think we should rush >> patches in. It's better to get things right. You say "then do everything >> else". Are there more functionality regarding x86 that you would like to see >> integrated in the Github submit hook? If so, I think it's better that you >> present it now, than just a piece at a time. >> >>> I believe the choice between "making the test code sparkling clean in one >>> go" and "making the product code sparking clean in one go" is obvious. >> >> I don't even understand what you mean by this. >> >>> These changes are logically independent. >> >> No, they are not. For one thing, this patch would explicitly introduce the >> "x32" naming, something JDK-8255305 is set. to remove. And this is just a >> trivial indication that both issues revolves around giving proper support >> for x86 in the submit flow. >> >> Just to be clear: I do not object to making x86 a first-class citizen in the >> submit hook! What I *do* object to is this piece-meal integration of two, or >> worse, three, PRs for what should have been one comprehensive PR. > >> Do you mean that you consider #505 and #548 dependent on this bug? If so, I >> think you misunderstand the purpose of the Github submit testing hook. You >> can -- and should! -- still do proper testing just as you did before the >> migration to Github. The Github action hook is a convenience. > > Oh yes, I must be misunderstanding this. Because I thought pre-submit tests > were the replacement for jdk-submit functionality, which was the Oracle's > requirement for push to shared code. Good to know it had been demoted to mere > suggestion. > >> Unless there is a breakage or a regression, I don't think we should rush >> patches in. It's better to get things right. You say "then do everything >> else". Are there more functionality regarding x86 that you would like to see >> integrated in the Github submit hook? If so, I think it's better that you >> present it now, than just a piece at a time. > > No, I mean to stop at `tier1` for x86_32. "Do everything else" means doing > the cosmetics like in this PR that does not affect the actual testing. > >> > I believe the choice between "making the test code sparkling clean in one >> > go" and "making the product code sparking clean in one go" is obvious. >> >> I don't even understand what you mean by this. > > I mean that bikeshedding the testing code means holding off the testing of > the product code. Making sure that product code is correct on x86_32 is more > important than making sure the testing code is cosmetically clean. > >> > These changes are logically independent. >> >> No, they are not. For one thing, this patch would explicitly introduce the >> "x32" naming, something JDK-8255305 is set. to remove. And this is just a >> trivial indication that both issues revolves around giving proper support >> for x86 in the submit flow. > > JDK-8255305 (#830) is set to add another instance of "x32", to already > existing "x32" in the build steps. That means #830 yields a logically > consistent `submit.yml`: it builds as "x32", it tests as "x32". This patch is > set to rename "x32" -> "x86" (*if*, *IF* we agree that rename even makes > sense!), yielding another logically consistent `submit.yml`. In this sense, > they are logically independent. > >> Just to be clear: I do not object to making x86 a first-class citizen in the >> submit hook! What I _do_ object to is this piece-meal integration of two, or >> worse, three, PRs for what should have been one comprehensive PR. > > There are "Request Changes" in #830 that you can use as Reviewer. I would > then oblige, but also reference this discussion. Because the next thing I > expect is someone else to come along and ask why "x32" -> "x86" rename is not > done separately from the addition of the actual testing. I would make that > comment myself for someone else's PR, because I prefer changes to do one > thing at a time. @shipilev My suggestion is that you take the commits from this PR and fold them into #830, and update the code added in #830 to match the naming. If you do that, I'll accept your patch. - PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8255412: "Linux x32" should be "Linux x86" in submit workflow
On Tue, 27 Oct 2020 09:38:41 GMT, Aleksey Shipilev wrote: > > Do you mean that you consider #505 and #548 dependent on this bug? If so, I > > think you misunderstand the purpose of the Github submit testing hook. You > > can -- and should! -- still do proper testing just as you did before the > > migration to Github. The Github action hook is a convenience. > Oh yes, I must be misunderstanding this. Because I thought pre-submit tests > were the replacement for jdk-submit functionality, which was the Oracle's > requirement for push to shared code. Good to know it had been demoted to mere > suggestion. I did not say it was a "suggestion". I said it was a "convenience". Those two are not the same. As a Committer, it is your responsibility, now and as it always has been, to make sure the code is tested to the best of your abilities. As a Reviewer, it is your responsibility to make make sure that no significant testing has been left out by the Committer. > Making sure that product code is correct on x86_32 is more important than > making sure the testing code is cosmetically clean. Thankfully, those two are not orthogonal! The testing for x86 on product changes can proceed just as before the move to Github, and we can make sure to keep the testing code up to the same quality as one should expect. - PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
On Sun, 18 Oct 2020 09:07:17 GMT, Magnus Ihse Bursie wrote: >> Bernhard Urban-Forster has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - uppercase suffix >> - add assert > > Build changes look fine now. @theRealAph does the PR look okay to you now? - PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8255412: "Linux x32" should be "Linux x86" in submit workflow
On Tue, 27 Oct 2020 09:22:39 GMT, Magnus Ihse Bursie wrote: > Do you mean that you consider #505 and #548 dependent on this bug? If so, I > think you misunderstand the purpose of the Github submit testing hook. You > can -- and should! -- still do proper testing just as you did before the > migration to Github. The Github action hook is a convenience. Oh yes, I must be misunderstanding this. Because I thought pre-submit tests were the replacement for jdk-submit functionality, which was the Oracle's requirement for push to shared code. Good to know it had been demoted to mere suggestion. > Unless there is a breakage or a regression, I don't think we should rush > patches in. It's better to get things right. You say "then do everything > else". Are there more functionality regarding x86 that you would like to see > integrated in the Github submit hook? If so, I think it's better that you > present it now, than just a piece at a time. No, I mean to stop at `tier1` for x86_32. "Do everything else" means doing the cosmetics like in this PR that does not affect the actual testing. > > I believe the choice between "making the test code sparkling clean in one > > go" and "making the product code sparking clean in one go" is obvious. > > I don't even understand what you mean by this. I mean that bikeshedding the testing code means holding off the testing of the product code. Making sure that product code is correct on x86_32 is more important than making sure the testing code is cosmetically clean. > > These changes are logically independent. > > No, they are not. For one thing, this patch would explicitly introduce the > "x32" naming, something JDK-8255305 is set. to remove. And this is just a > trivial indication that both issues revolves around giving proper support for > x86 in the submit flow. JDK-8255305 (#830) is set to add another instance of "x32", to already existing "x32" in the build steps. That means #830 yields a logically consistent `submit.yml`: it builds as "x32", it tests as "x32". This patch is set to rename "x32" -> "x86" (*if*, *IF* we agree that rename even makes sense!), yielding another logically consistent `submit.yml`. In this sense, they are logically independent. > Just to be clear: I do not object to making x86 a first-class citizen in the > submit hook! What I _do_ object to is this piece-meal integration of two, or > worse, three, PRs for what should have been one comprehensive PR. There are "Request Changes" in #830 that you can use as Reviewer. I would then oblige, but also reference this discussion. Because the next thing I expect is someone else to come along and ask why "x32" -> "x86" rename is not done separately from the addition of the actual testing. I would make that comment myself for someone else's PR, because I prefer changes to do one thing at a time. - PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8255412: "Linux x32" should be "Linux x86" in submit workflow
On Tue, 27 Oct 2020 08:53:29 GMT, Aleksey Shipilev wrote: >> Let me coop this PR to rename x32 -> x86, among other cosmetic changes. Once >> #830 lands, I'll update x32 -> x86 in those new blocks as well. This way, we >> can have best of both worlds: x86_32 would start testing, and we would do >> the follow-ups here. > >> @shipilev I realize you are eager to have Github run automatic testing on >> platforms that you care about, but it's not like it's a regression that >> needs to be taken care of immediately. I strongly prefer to have a correct >> solution implemented once, before committing, than this piece-meal splitting >> of what's actually a single issue into three different PRs. > > I very much care that the in-flight PRs (notably #505 and #548) would get > their x86_32 testing early, hopefully before their integration. Because, > having the same argument here, I strongly prefer to have a correct solution > implemented once for the *actual product code*. I believe the choice between > "making the test code sparkling clean in one go" and "making the product code > sparking clean in one go" is obvious. > >> Now, the first one has already gone in, so not much to do about that, but I >> strongly believe that this and JDK-8255305 should be folded together in a >> single issue. > > I disagree. These changes are logically independent. #830 adds testing step, > it is self-sufficient, already tested, and already in review. This one cleans > up cosmetics, is relatively new, has a potential to accumulate other > followups, and thus risks to drag on. Let's do the MVP in #830, and then do > everything else. Do you mean that you consider #505 and #548 dependent on this bug? If so, I think you misunderstand the purpose of the Github submit testing hook. You can -- and should! -- still do proper testing just as you did before the migration to Github. The Github action hook is a convenience. Unless there is a breakage or a regression, I don't think we should rush patches in. It's better to get things right. You say "then do everything else". Are there more functionality regarding x86 that you would like to see integrated in the Github submit hook? If so, I think it's better that you present it now, than just a piece at a time. > I believe the choice between "making the test code sparkling clean in one go" > and "making the product code sparking clean in one go" is obvious. I don't even understand what you mean by this. > These changes are logically independent. No, they are not. For one thing, this patch would explicitly introduce the "x32" naming, something JDK-8255305 is set. to remove. And this is just a trivial indication that both issues revolves around giving proper support for x86 in the submit flow. Just to be clear: I do not object to making x86 a first-class citizen in the submit hook! What I *do* object to is this piece-meal integration of two, or worse, three, PRs for what should have been one comprehensive PR. - PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8255412: "Linux x32" should be "Linux x86" in submit workflow
On Tue, 27 Oct 2020 06:35:33 GMT, Aleksey Shipilev wrote: >>> > My rationale is simple: I am playing the whack-a-mole against the ongoing >>> > x86_32 regressions here >>> >>> Would it be possible that we add a Travis hook so that commits get tested >>> once before they are merged? >> >> Maybe, but that would be beyond the scope of this PR. These GH actions >> workflows go the long way already to make pre-integration tests, and they >> already run and used by OpenJDK contributors. I see immediate value in >> improving GH workflows, and see no clear estimate how much work would be >> involved in hooking up Travis, and what the project-wide consequences would >> be. If you want, open a new bug, prototype the change, discuss it there, and >> see if that is an acceptable thing to do. > > Let me coop this PR to rename x32 -> x86, among other cosmetic changes. Once > #830 lands, I'll update x32 -> x86 in those new blocks as well. This way, we > can have best of both worlds: x86_32 would start testing, and we would do the > follow-ups here. > @shipilev I realize you are eager to have Github run automatic testing on > platforms that you care about, but it's not like it's a regression that needs > to be taken care of immediately. I strongly prefer to have a correct solution > implemented once, before committing, than this piece-meal splitting of what's > actually a single issue into three different PRs. I very much care that the in-flight PRs (notably #505 and #548) would get their x86_32 testing early, hopefully before their integration. Because, having the same argument here, I strongly prefer to have a correct solution implemented once for the *actual product code*. I believe the choice between "making the test code sparkling clean in one go" and "making the product code sparking clean in one go" is obvious. > Now, the first one has already gone in, so not much to do about that, but I > strongly believe that this and JDK-8255305 should be folded together in a > single issue. I disagree. These changes are logically independent. #830 adds testing step, it is self-sufficient, already tested, and already in review. This one cleans up cosmetics, is relatively new, has a potential to accumulate other followups, and thus risks to drag on. Let's do the MVP in #830, and then do everything else. - PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8255412: "Linux x32" should be "Linux x86" in submit workflow
On Tue, 27 Oct 2020 06:35:33 GMT, Aleksey Shipilev wrote: >>> > My rationale is simple: I am playing the whack-a-mole against the ongoing >>> > x86_32 regressions here >>> >>> Would it be possible that we add a Travis hook so that commits get tested >>> once before they are merged? >> >> Maybe, but that would be beyond the scope of this PR. These GH actions >> workflows go the long way already to make pre-integration tests, and they >> already run and used by OpenJDK contributors. I see immediate value in >> improving GH workflows, and see no clear estimate how much work would be >> involved in hooking up Travis, and what the project-wide consequences would >> be. If you want, open a new bug, prototype the change, discuss it there, and >> see if that is an acceptable thing to do. > > Let me coop this PR to rename x32 -> x86, among other cosmetic changes. Once > #830 lands, I'll update x32 -> x86 in those new blocks as well. This way, we > can have best of both worlds: x86_32 would start testing, and we would do the > follow-ups here. @shipilev I realize you are eager to have Github run automatic testing on platforms that you care about, but it's not like it's a regression that needs to be taken care of immediately. I strongly prefer to have a correct solution implemented once, before committing, than this piece-meal splitting of what's actually a single issue into three different PRs. Now, the first one has already gone in, so not much to do about that, but I strongly believe that this and JDK-8255305 should be folded together in a single issue. - PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8255412: "Linux x32" should be "Linux x86" in submit workflow
On Tue, 27 Oct 2020 06:29:54 GMT, Aleksey Shipilev wrote: >>> My rationale is simple: I am playing the whack-a-mole against the ongoing >>> x86_32 regressions here >> >> Would it be possible that we add a Travis hook so that commits get tested >> once before they are merged? >> >> Travis supports all of the important architectures of OpenJDK on Linux these >> days: https://docs.travis-ci.com/user/multi-cpu-architectures/ > >> > My rationale is simple: I am playing the whack-a-mole against the ongoing >> > x86_32 regressions here >> >> Would it be possible that we add a Travis hook so that commits get tested >> once before they are merged? > > Maybe, but that would be beyond the scope of this PR. These GH actions > workflows go the long way already to make pre-integration tests, and they > already run and used by OpenJDK contributors. I see immediate value in > improving GH workflows, and see no clear estimate how much work would be > involved in hooking up Travis, and what the project-wide consequences would > be. If you want, open a new bug, prototype the change, discuss it there, and > see if that is an acceptable thing to do. Let me coop this PR to rename x32 -> x86, among other cosmetic changes. Once #830 lands, I'll update x32 -> x86 in those new blocks as well. This way, we can have best of both worlds: x86_32 would start testing, and we would do the follow-ups here. - PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8255412: "Linux x32" should be "Linux x86" in submit workflow [v2]
> John Paul Adrian Glaubitz mentions "x32" is actually the designation for > "x86_64 with 32-bit pointers". Let's rename "Linux x32" to "Linux x86". I > also realized that JDK-8254282 missed "Linux x32" for the platform selection. > While you can guess it is accepted from reading the workflow, it would be > more convenient to have it in the default argument list. Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Rename x32 -> x86. - Changes: - all: https://git.openjdk.java.net/jdk/pull/869/files - new: https://git.openjdk.java.net/jdk/pull/869/files/0dabc471..a155127a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=869=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=869=00-01 Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/869.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/869/head:pull/869 PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8255412: Add "Linux x32" to platform selection default in custom submit workflow
On Tue, 27 Oct 2020 06:24:32 GMT, John Paul Adrian Glaubitz wrote: > > My rationale is simple: I am playing the whack-a-mole against the ongoing > > x86_32 regressions here > > Would it be possible that we add a Travis hook so that commits get tested > once before they are merged? Maybe, but that would be beyond the scope of this PR. These GH actions workflows go the long way already to make pre-integration tests, and they already run and used by OpenJDK contributors. I see immediate value in improving GH workflows, and see no clear estimate how much work would be involved in hooking up Travis, and what the project-wide consequences would be. If you want, open a new bug, prototype the change, discuss it there, and see if that is an acceptable thing to do. - PR: https://git.openjdk.java.net/jdk/pull/869
Re: RFR: 8255412: Add "Linux x32" to platform selection default in custom submit workflow
On Tue, 27 Oct 2020 05:48:49 GMT, Aleksey Shipilev wrote: > My rationale is simple: I am playing the whack-a-mole against the ongoing > x86_32 regressions here Would it be possible that we add a Travis hook so that commits get tested once before they are merged? Travis supports all of the important architectures of OpenJDK on Linux these days: https://docs.travis-ci.com/user/multi-cpu-architectures/ - PR: https://git.openjdk.java.net/jdk/pull/869
