Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e I have reviewed the following portions of this change: test/common, test/gtest, test/hotspot/runtime, test/jdk/jfr, test library Feedback: - see several minor comments inline - under runtime/cds/appcds/test-classes there is an empty "TEST.properties" file; was it added by mistake? With only a few minor comments, I approve of the code reviewed by me provided that my comments will be addressed. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e test/lib/jdk/test/lib/thread/VThreadRunner.java line 61: > 59: /** > 60: * Run a task in a virtual thread and wait for it to terminate. > 61: * If the task completse with an exception then it is thrown by this > method. typo: completse --> completes test/lib/jdk/test/lib/thread/VThreadRunner.java line 121: > 119: /** > 120: * Run a task in a virtual thread and wait for it to terminate. > 121: * If the task completse with an exception then it is thrown by this > method. completse --> completes - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) [v3]
> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare > values by identity. Updated API documentation of these two methods > ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object))) > to mention such behavior. liach has updated the pull request incrementally with one additional commit since the last revision: Fix assertions - Changes: - all: https://git.openjdk.java.net/jdk/pull/8259/files - new: https://git.openjdk.java.net/jdk/pull/8259/files/dd416079..fe91721d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8259=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8259=01-02 Stats: 15 lines in 1 file changed: 0 ins; 0 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/8259.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8259/head:pull/8259 PR: https://git.openjdk.java.net/jdk/pull/8259
Integrated: 8284992: Fix misleading Vector API doc for LSHR operator
On Tue, 19 Apr 2022 08:41:50 GMT, Jie Fu wrote: > Hi all, > > The Current Vector API doc for `LSHR` is > > Produce a>>>(n&(ESIZE*8-1)). Integral only. > > > This is misleading which may lead to bugs for Java developers. > This is because for negative byte/short elements, the results computed by > `LSHR` will be different from that of `>>>`. > For more details, please see > https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . > > After the patch, the doc for `LSHR` is > > Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. > > > Thanks. > Best regards, > Jie This pull request has now been integrated. Changeset: e54f26aa Author:Jie Fu URL: https://git.openjdk.java.net/jdk/commit/e54f26aa3d5d44264e052bc51d3d819a8da5d1e7 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod 8284992: Fix misleading Vector API doc for LSHR operator Reviewed-by: psandoz - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
On Thu, 28 Apr 2022 19:48:18 GMT, Paul Sandoz wrote: >> Jie Fu has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains six additional commits since >> the last revision: >> >> - Address review comments >> - Merge branch 'master' into JDK-8284992 >> - Merge branch 'master' into JDK-8284992 >> - Address review comments >> - Merge branch 'master' into JDK-8284992 >> - 8284992: Fix misleading Vector API doc for LSHR operator > > It should be possible for you finalize now. Thanks @PaulSandoz for the review and help. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
On Sun, 17 Apr 2022 16:17:30 GMT, liach wrote:
> Convert dynamic proxies to hidden classes. Modifies the serialization of
> proxies (requires change in "Java Object Serialization Specification"). Makes
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
>
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of
> the class is allocated... The contents restored appropriately" section, I
> propose explicitly state that handling of proxies are unspecified as to allow
> implementation freedom, though I've seen deliberate attempts for proxies to
> implement interfaces with `readResolve` in order to control their
> serialization behavior.
>- This is for the existing generated constructor accessor is
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization
> constructor calls `invokespecial` on the closest serializable superclass'
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and
> should be discouraged in the long term. Calling the existing constructor with
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method,
> implemented by the lambda, will not appear in the stack trace, and isn't too
> problematic.
>
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into
> one. This reduced a lot of code and allowed anchor class (for hidden class
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of
> `java.base`. This API is intended for implementation of legacy (pre
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959):
> passes methods computed by proxy generator as class data to the hidden proxy
> class to reduce generated proxy class size and improve performance.
>
> In addition, since this change is somewhat large, should we keep the old
> proxy generator as well and have it toggled through a command-line flag (like
> the old v49 proxy generator or the old reflection implementation)?
>
> Please feel free to comment or review. This change definitely requires a CSR,
> but I have yet to determine what specifications should be changed.
So, after reading the updated valhalla documentation, namely after the expert
group decides to represent identity vs value with flags without touching
inheritance (so saves serialization breakage when there's no spurious
`IdentityObject`) and that `LambdaMetafactory` will reject identity or value
interfaces per [Value Objects JEP](https://openjdk.java.net/jeps/8277163), I
wonder about the future of dynamic proxies as well. I expect proxies to reject
identity or value interfaces like `LambdaMetafactory`, and we most likely need
a new API, like Remi's, if we wish to support identity/value interfaces.
Also for deserialization, since we have `Unsafe.allocateInstance`, we might
alternatively replace the `anew` bytecode with such a call for the native
serialization constructor if we do serialize and deserialize any hidden class
like proxies, without touching the security part.
-
PR: https://git.openjdk.java.net/jdk/pull/8278
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v2]
On Fri, 22 Apr 2022 07:08:24 GMT, Xiaohong Gong wrote:
>> Currently the vector load with mask when the given index happens out of the
>> array boundary is implemented with pure java scalar code to avoid the IOOBE
>> (IndexOutOfBoundaryException). This is necessary for architectures that do
>> not support the predicate feature. Because the masked load is implemented
>> with a full vector load and a vector blend applied on it. And a full vector
>> load will definitely cause the IOOBE which is not valid. However, for
>> architectures that support the predicate feature like SVE/AVX-512/RVV, it
>> can be vectorized with the predicated load instruction as long as the
>> indexes of the masked lanes are within the bounds of the array. For these
>> architectures, loading with unmasked lanes does not raise exception.
>>
>> This patch adds the vectorization support for the masked load with IOOBE
>> part. Please see the original java implementation (FIXME: optimize):
>>
>>
>> @ForceInline
>> public static
>> ByteVector fromArray(VectorSpecies species,
>>byte[] a, int offset,
>>VectorMask m) {
>> ByteSpecies vsp = (ByteSpecies) species;
>> if (offset >= 0 && offset <= (a.length - species.length())) {
>> return vsp.dummyVector().fromArray0(a, offset, m);
>> }
>>
>> // FIXME: optimize
>> checkMaskFromIndexSize(offset, vsp, m, 1, a.length);
>> return vsp.vOp(m, i -> a[offset + i]);
>> }
>>
>> Since it can only be vectorized with the predicate load, the hotspot must
>> check whether the current backend supports it and falls back to the java
>> scalar version if not. This is different from the normal masked vector load
>> that the compiler will generate a full vector load and a vector blend if the
>> predicate load is not supported. So to let the compiler make the expected
>> action, an additional flag (i.e. `usePred`) is added to the existing
>> "loadMasked" intrinsic, with the value "true" for the IOOBE part while
>> "false" for the normal load. And the compiler will fail to intrinsify if the
>> flag is "true" and the predicate load is not supported by the backend, which
>> means that normal java path will be executed.
>>
>> Also adds the same vectorization support for masked:
>> - fromByteArray/fromByteBuffer
>> - fromBooleanArray
>> - fromCharArray
>>
>> The performance for the new added benchmarks improve about `1.88x ~ 30.26x`
>> on the x86 AVX-512 system:
>>
>> Benchmark before After Units
>> LoadMaskedIOOBEBenchmark.byteLoadArrayMaskIOOBE 737.542 1387.069 ops/ms
>> LoadMaskedIOOBEBenchmark.doubleLoadArrayMaskIOOBE 118.366 330.776 ops/ms
>> LoadMaskedIOOBEBenchmark.floatLoadArrayMaskIOOBE 233.832 6125.026 ops/ms
>> LoadMaskedIOOBEBenchmark.intLoadArrayMaskIOOBE233.816 7075.923 ops/ms
>> LoadMaskedIOOBEBenchmark.longLoadArrayMaskIOOBE 119.771 330.587 ops/ms
>> LoadMaskedIOOBEBenchmark.shortLoadArrayMaskIOOBE 431.961 939.301 ops/ms
>>
>> Similar performance gain can also be observed on 512-bit SVE system.
>
> Xiaohong Gong has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rename the "usePred" to "offsetInRange"
IIUC when the hardware does not support predicated loads then any false
`offsetIntRange` value causes the load intrinsic to fail resulting in the
fallback, so it would not be materially any different to the current behavior,
just more uniformly implemented.
Why can't the intrinsic support the passing a boolean directly? Is it something
to do with constants? If that is not possible I recommend creating named
constant values and pass those all the way through rather than converting a
boolean to an integer value. Then there is no need for a branch checking
`offsetInRange`.
Might be better to hold off until the JEP is integrated and then update, since
this will conflict (`byte[]` and `ByteBuffer` load methods are removed and
`MemorySegment` load methods are added). You could prepare for that now by
branching off `vectorIntrinsics`.
-
PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov wrote:
> `Map.containsKey` call is sometimes unnecessary, when it's known that Map
> doesn't contain `null` values.
> Instead we can just use Map.get and compare result with `null`.
> I found one of such place, where Map.containsKey calls could be eliminated -
> `java.time.format.ZoneName`.
> it gives a bit of performance for `toZid`.
>
>
> Benchmark Mode Cnt Score Error Units
> ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op
> ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op
>
>
>
> Benchmark
>
>
> @BenchmarkMode(value = Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
> @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
> @Fork(1)
> @State(Scope.Benchmark)
> public class ZoneNamesBench {
>
> @Benchmark
> public String useExistingCountry() {
> return ZoneName.toZid("Africa/Tunis", Locale.ITALY);
> }
>
> @Benchmark
> public String useExistingCountryOld() {
> return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY);
> }
> }
>
>
>
> public static String toZid(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null) {
> String alias = aliases.get(zid);
> if (alias != null) {
> zid = alias;
> mzone = zidToMzone.get(zid);
> }
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map == null || ((zid = map.get(locale.getCountry())) ==
> null)) {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZid(zid);
> }
>
> public static String toZid(String zid) {
> return aliases.getOrDefault(zid, zid);
> }
>
> public static String toZidOld(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null && aliases.containsKey(zid)) {
> zid = aliases.get(zid);
> mzone = zidToMzone.get(zid);
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map != null && map.containsKey(locale.getCountry())) {
> zid = map.get(locale.getCountry());
> } else {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZidOld(zid);
> }
>
> public static String toZidOld(String zid) {
> if (aliases.containsKey(zid)) {
> return aliases.get(zid);
> }
> return zid;
> }
>
>
Hm, build of this branch fails with Crash on `Optimizing the exploded image` on
one of my machines. 樂
EXCEPTION_ACCESS_VIOLATION (0xc005) at pc=0x, pid=57300,
tid=10552
[hs_err_pid49380.log](https://github.com/openjdk/jdk/files/8594756/hs_err_pid49380.log)
[hs_err_pid57300.log](https://github.com/openjdk/jdk/files/8594758/hs_err_pid57300.log)
-
PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Fri, 29 Apr 2022 18:27:27 GMT, Mikhailo Seledtsov wrote: >> Alan Bateman has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > > test/jdk/jdk/jfr/api/consumer/TestManyRecordings.java line 57: > >> 55: int classLoaderCount = Integer.parseInt(args[0]); >> 56: int classCount = Integer.parseInt(args[1]); >> 57: for (int i = 0; i > Did you mean classLoaderCount here instead of classCount? Also, please make > sure there is a space between < and classLoaderCount. The JFR "tests" look strange. I would expect a test called TestManyRecording to start recordings, not created a lot of classes, similar to TestManyClasses. How is this related to Loom? Could this be a merge gone bad? - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 20:45:20 GMT, Andrey Turbanov wrote:
>> src/java.base/share/classes/java/time/format/ZoneName.java.template line 60:
>>
>>> 58:
>>> 59: public static String toZid(String zid) {
>>> 60: return aliases.getOrDefault(zid, zid);
>>
>> Is the behavior if zid == null the same? aliases.getOrDefault will throw
>> NPE on null.
>> neither Hashmap.containsKey or .get throw on null.
>
>>aliases.getOrDefault will throw NPE on null
>
> No, It will not. `aliases` is a HashMap. And HashMap supports null values and
> keys.
Anyway, this method is used only in `DateTimeFormatterBuilder` and it doesn't
pass `null` value there.
-
PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 20:36:59 GMT, Roger Riggs wrote: >aliases.getOrDefault will throw NPE on null No, It will not. `aliases` is a HashMap. And HashMap supports null values and keys. - PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov wrote:
> `Map.containsKey` call is sometimes unnecessary, when it's known that Map
> doesn't contain `null` values.
> Instead we can just use Map.get and compare result with `null`.
> I found one of such place, where Map.containsKey calls could be eliminated -
> `java.time.format.ZoneName`.
> it gives a bit of performance for `toZid`.
>
>
> Benchmark Mode Cnt Score Error Units
> ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op
> ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op
>
>
>
> Benchmark
>
>
> @BenchmarkMode(value = Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
> @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
> @Fork(1)
> @State(Scope.Benchmark)
> public class ZoneNamesBench {
>
> @Benchmark
> public String useExistingCountry() {
> return ZoneName.toZid("Africa/Tunis", Locale.ITALY);
> }
>
> @Benchmark
> public String useExistingCountryOld() {
> return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY);
> }
> }
>
>
>
> public static String toZid(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null) {
> String alias = aliases.get(zid);
> if (alias != null) {
> zid = alias;
> mzone = zidToMzone.get(zid);
> }
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map == null || ((zid = map.get(locale.getCountry())) ==
> null)) {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZid(zid);
> }
>
> public static String toZid(String zid) {
> return aliases.getOrDefault(zid, zid);
> }
>
> public static String toZidOld(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null && aliases.containsKey(zid)) {
> zid = aliases.get(zid);
> mzone = zidToMzone.get(zid);
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map != null && map.containsKey(locale.getCountry())) {
> zid = map.get(locale.getCountry());
> } else {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZidOld(zid);
> }
>
> public static String toZidOld(String zid) {
> if (aliases.containsKey(zid)) {
> return aliases.get(zid);
> }
> return zid;
> }
>
>
src/java.base/share/classes/java/time/format/ZoneName.java.template line 60:
> 58:
> 59: public static String toZid(String zid) {
> 60: return aliases.getOrDefault(zid, zid);
Is the behavior if zid == null the same? aliases.getOrDefault will throw NPE
on null.
neither Hashmap.containsKey or .get throw on null.
-
PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote:
>> This is an early review of changes to better model JVM access flags, that is
>> "modifiers" like public, protected, etc. but explicitly at a VM level.
>>
>> Language level modifiers and JVM level access flags are closely related, but
>> distinct. There are concepts that overlap in the two domains (public,
>> private, etc.), others that only have a language-level modifier (sealed),
>> and still others that only have an access flag (synthetic).
>>
>> The existing java.lang.reflect.Modifier class is inadequate to model these
>> subtleties. For example, the bit positions used by access flags on different
>> kinds of elements overlap (such as "volatile" for fields and "bridge" for
>> methods. Just having a raw integer does not provide sufficient context to
>> decode the corresponding language-level string. Methods like
>> Modifier.methodModifiers() were introduced to cope with this situation.
>>
>> With additional modifiers and flags on the horizon with projects like
>> Valhalla, addressing the existent modeling deficiency now ahead of time is
>> reasonable before further strain is introduced.
>>
>> This PR in its current form is meant to give the overall shape of the API.
>> It is missing implementations to map from, say, method modifiers to access
>> flags, taking into account overlaps in bit positions.
>>
>> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in
>> once the API is further along.
>
> Joe Darcy 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 26 additional commits since
> the last revision:
>
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8266670
> - Make workding changes suggested in review feedback.
> - Merge branch 'master' into JDK-8266670
> - Typo fix; add implSpec to Executable.
> - Appease jcheck.
> - Fix some bugs found by inspection, docs cleanup.
> - Merge branch 'master' into JDK-8266670
> - Initial support for accessFlags methods
> - Add mask to access flag functionality.
> - ... and 16 more:
> https://git.openjdk.java.net/jdk/compare/7ac698ba...14980605
I took a closer look at the proposed APIs. I think it's in a good state to
target for 19. I skimmed on the existing JDK usage of `getModifiers` other
than a trivial test e.g. is public, final, etc and the proposed API works well
(btw no plan to convert them and just use those cases for validation).
The value of `ACC_SUPER` and `ACC_STRICT` could possibly be reused for new
access flags. It may be useful to document when the flag becomes obsolete.
Nit: the enum constants are listed in the order of the mask value, which I
like. Some enum constants reference the `Modifer` constants but I think it'd
help to see the mask value here consistently for all entries. One go-to place
in the source if I want to find the value of different flags.
src/java.base/share/classes/java/lang/Class.java line 1328:
> 1326: * @since 19
> 1327: */
> 1328: public Set accessFlags() {
The access flags of a hidden class has no difference than that of a normal
class. A hidden class is created with normal `ClassFile` except that it's
created via `Lookup::defineHiddenClass` API.
I think the `Class::accessFlags` method should specify the access flags for
primitive type, void, and array classes as `Class::getModifiers` specified.
src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 216:
> 214:
> 215: /**
> 216: * {@return an unmodifiable set of the module {@linkplain
> AccessFlag
Suggestion:
* {@return a possibly-empty unmodifiable set of the module {@linkplain
AccessFlag
as specified in the `@return` of the `modifiers()` method. Same comment
applies to the `accessFlags` method in `ModuleDescriptor.Opens` and other
classes.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 44:
> 42: * not have an access flag, such as {@code sealed} (JVMS
> 43: * {@jvms 4.7.31}) and some access flags have no corresponding
> 44: * modifier, such as {@linkplain SYNTHETIC synthetic}.
Suggestion:
* modifier, such as {@linkplain #SYNTHETIC synthetic}.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 186:
> 184: /**
> 185: * The access flag {@code ACC_ABSTRACT}, corresponding to the
> 186: * source modifier {@code link Modifier#ABSTRACT abstract}.
Suggestion:
* source modifier {@link Modifier#ABSTRACT abstract}.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 306:
> 304: * Note that since these locations represent class file structures
> 305: * rather than language structures many language structures, such
> 306: * as constructors and interfaces, are not present.
missing `@since 19`
-
PR:
RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
`Map.containsKey` call is sometimes unnecessary, when it's known that Map
doesn't contain `null` values.
Instead we can just use Map.get and compare result with `null`.
I found one of such place, where Map.containsKey calls could be eliminated -
`java.time.format.ZoneName`.
it gives a bit of performance for `toZid`.
Benchmark Mode Cnt Score Error Units
ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op
ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op
Benchmark
@BenchmarkMode(value = Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
@Fork(1)
@State(Scope.Benchmark)
public class ZoneNamesBench {
@Benchmark
public String useExistingCountry() {
return ZoneName.toZid("Africa/Tunis", Locale.ITALY);
}
@Benchmark
public String useExistingCountryOld() {
return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY);
}
}
public static String toZid(String zid, Locale locale) {
String mzone = zidToMzone.get(zid);
if (mzone == null) {
String alias = aliases.get(zid);
if (alias != null) {
zid = alias;
mzone = zidToMzone.get(zid);
}
}
if (mzone != null) {
Map map = mzoneToZidL.get(mzone);
if (map == null || ((zid = map.get(locale.getCountry())) == null)) {
zid = mzoneToZid.get(mzone);
}
}
return toZid(zid);
}
public static String toZid(String zid) {
return aliases.getOrDefault(zid, zid);
}
public static String toZidOld(String zid, Locale locale) {
String mzone = zidToMzone.get(zid);
if (mzone == null && aliases.containsKey(zid)) {
zid = aliases.get(zid);
mzone = zidToMzone.get(zid);
}
if (mzone != null) {
Map map = mzoneToZidL.get(mzone);
if (map != null && map.containsKey(locale.getCountry())) {
zid = map.get(locale.getCountry());
} else {
zid = mzoneToZid.get(mzone);
}
}
return toZidOld(zid);
}
public static String toZidOld(String zid) {
if (aliases.containsKey(zid)) {
return aliases.get(zid);
}
return zid;
}
-
Commit messages:
- [PATCH] Avoid redundant HashMap.containsKey calls in ZoneName
- [PATCH] Avoid redundant HashMap.containsKey calls in ZoneName
Changes: https://git.openjdk.java.net/jdk/pull/8463/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8463=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8285947
Stats: 14 lines in 1 file changed: 3 ins; 5 del; 6 mod
Patch: https://git.openjdk.java.net/jdk/pull/8463.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8463/head:pull/8463
PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 18:28:45 GMT, Weijun Wang wrote: >> Shouldn't the comparison be better implemented by the caller then, who will >> know whether spaces are important or not? That's why I had suggested taking >> a `Predicate` that could be called with each line removed, and the >> caller could interrupt the parsing by returning false when they detect a >> mismatch with what they expect. > > We can provide a more sophisticated `Function` replacer if we > want to let user to customize all the details. This time we still only want > them to be string literals. I agree we can keep the new lines inside, but > trimming on each line and the final block is still useful so caller does not > need to care about indentation and empty lines at both ends. OK - if you keep the internal new lines I have no objection. The API doc should however say that lines will be trimmed before comparing them. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator)
On Wed, 27 Apr 2022 11:03:48 GMT, Jatin Bhateja wrote: > Hi All, > > Patch adds the planned support for new vector operations and APIs targeted > for [JEP 426: Vector API (Fourth > Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173) > > Following is the brief summary of changes:- > > 1) Extends the scope of existing lanewise API for following new vector > operations. >- VectorOperations.BIT_COUNT: counts the number of one-bits >- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero > bits >- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing > zero bits >- VectorOperations.REVERSE: reversing the order of bits >- VectorOperations.REVERSE_BYTES: reversing the order of bytes >- compress and expand bits: Semantics are based on Hacker's Delight > section 7-4 Compress, or Generalized Extract. > > 2) Adds following new APIs to perform cross lane vector compress and > expansion operations under the influence of a mask. >- Vector.compress >- Vector.expand >- VectorMask.compress > > 3) Adds predicated and non-predicated versions of following new APIs to load > and store the contents of vector from foreign MemorySegments. > - Vector.fromMemorySegment > - Vector.intoMemorySegment > > 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support > for each newly added operation. > > > Patch has been regressed over AARCH64 and X86 targets different AVX levels. > > Kindly review and share your feedback. > > Best Regards, > Jatin Remind: please use the command `/jep JEP-426` [1] to mark this PR. [1] https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/jep - PR: https://git.openjdk.java.net/jdk/pull/8425
RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator)
Hi All, Patch adds the planned support for new vector operations and APIs targeted for [JEP 426: Vector API (Fourth Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173) Following is the brief summary of changes:- 1) Extends the scope of existing lanewise API for following new vector operations. - VectorOperations.BIT_COUNT: counts the number of one-bits - VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero bits - VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing zero bits - VectorOperations.REVERSE: reversing the order of bits - VectorOperations.REVERSE_BYTES: reversing the order of bytes - compress and expand bits: Semantics are based on Hacker's Delight section 7-4 Compress, or Generalized Extract. 2) Adds following new APIs to perform cross lane vector compress and expansion operations under the influence of a mask. - Vector.compress - Vector.expand - VectorMask.compress 3) Adds predicated and non-predicated versions of following new APIs to load and store the contents of vector from foreign MemorySegments. - Vector.fromMemorySegment - Vector.intoMemorySegment 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support for each newly added operation. Patch has been regressed over AARCH64 and X86 targets different AVX levels. Kindly review and share your feedback. Best Regards, Jatin - Commit messages: - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - 8284960: AARCH64 backend changes. - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - 8284960: Integration of JEP 426: Vector API (Fourth Incubator) Changes: https://git.openjdk.java.net/jdk/pull/8425/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8425=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284960 Stats: 37837 lines in 214 files changed: 16462 ins; 16923 del; 4452 mod Patch: https://git.openjdk.java.net/jdk/pull/8425.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8425/head:pull/8425 PR: https://git.openjdk.java.net/jdk/pull/8425
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 14:19:40 GMT, Daniel Fuchs wrote: >> The comparison is intentionally made lax so the caller does not need to >> provide the exact indentation or even new line characters. We think along >> with `fromLine` and `toLine` this is enough to make sure we are not >> modifying the wrong lines. > > Shouldn't the comparison be better implemented by the caller then, who will > know whether spaces are important or not? That's why I had suggested taking a > `Predicate` that could be called with each line removed, and the > caller could interrupt the parsing by returning false when they detect a > mismatch with what they expect. We can provide a more sophisticated `Function` replacer if we want to let user to customize all the details. This time we still only want them to be string literals. I agree we can keep the new lines inside, but trimming on each line and the final block is still useful so caller does not need to care about indentation and empty lines at both ends. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Fri, 29 Apr 2022 18:23:35 GMT, Mikhailo Seledtsov wrote: >> Alan Bateman has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > > test/jdk/jdk/jfr/api/consumer/TestManyClasses.java line 57: > >> 55: int classLoaderCount = Integer.parseInt(args[0]); >> 56: int classCount = Integer.parseInt(args[1]); >> 57: for (int i = 0; i > Minor: Style: please insert space between < and classCount Also, should this be i < classLoaderCount ?? - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e test/jdk/jdk/jfr/api/consumer/TestManyClasses.java line 57: > 55: int classLoaderCount = Integer.parseInt(args[0]); > 56: int classCount = Integer.parseInt(args[1]); > 57: for (int i = 0; i 62: theClass.newInstance(); > 63: TestEvent event = new TestEvent(); > 64: event.theClass = event; Did you mean event.theClass = theClass instead ? test/jdk/jdk/jfr/api/consumer/TestManyRecordings.java line 57: > 55: int classLoaderCount = Integer.parseInt(args[0]); > 56: int classCount = Integer.parseInt(args[1]); > 57: for (int i = 0; i https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e test/hotspot/jtreg/runtime/vthread/StackChunkClassLoaderTest.java line 2: > 1: /* > 2: * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. Please update copyright year. - PR: https://git.openjdk.java.net/jdk/pull/8166
Integrated: 8282227: Locale information for nb is not working properly
On Tue, 26 Apr 2022 00:32:48 GMT, Naoto Sato wrote: > This was caused by incorporating CLDR v39, which switched the Norwegian > locale inheritance from `no` -> `nb` to `nb` ->`no` and moved the resources > from `nb` to `no`, which now contradicts Java's locale fallback. Explicitly > inheriting `no` from `nb` will fix the issue. This pull request has now been integrated. Changeset: 3d07b3c7 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/3d07b3c7f01b60ff4dc38f62407c212b48883dbf Stats: 79 lines in 2 files changed: 74 ins; 1 del; 4 mod 8282227: Locale information for nb is not working properly Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/8394
Re: RFR: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs LGTM also. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8465
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v5]
On Fri, 29 Apr 2022 02:12:19 GMT, Iris Clark wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Change punctuation from review feedback.
>
> src/java.base/share/classes/java/lang/System.java line 743:
>
>> 741: * have the value {@code "1"}; after a second maintenance
>> 742: * release, this property will have the value {@code "2"},
>> 743: * and so on.
>
> There should be no requirement that values be allocated sequentially. In
> other words, if JCP MR does not require an RI, then it should not be
> surprising if there is no JDK build with maintenance version . As an
> example, JSR 337 MR 1 and MR 2 both used the same RI. If this system
> property had existed during development of MR 1, it would return "1". Since
> no RI was submitted for MR 2, a build returning "2" should never exist. MR 3
> did update the RI, so it would return "3".
@irisclark does raise an interesting point: If, say, MR 2 doesn’t require a
change to the RI then the MR 1 RI is also the MR 2 RI, but its
`java.specification.maintenance.version` property will report that it’s the MR
1 RI.
One way to fix this would be to require an RI update with every MR just to
update this property, even if no other code in the RI changes — but we prefer
to avoid doing RI builds unnecessarily.
Another way to fix it would be to finesse the specification of this property,
perhaps:
* {@systemProperty
java.specification.maintenance.version}
* Java Runtime Environment specification maintenance version,
which may
* be interpreted as a non-zero integer. If defined, the value of
this
* property is the identifying number of the most recent https://jcp.org/en/procedures/jcp2#3.6.4;>specification
* maintenance release that required a change to the runtime
* (optional).
*
-
PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 8282227: Locale information for nb is not working properly
On Tue, 26 Apr 2022 00:32:48 GMT, Naoto Sato wrote: > This was caused by incorporating CLDR v39, which switched the Norwegian > locale inheritance from `no` -> `nb` to `nb` ->`no` and moved the resources > from `nb` to `no`, which now contradicts Java's locale fallback. Explicitly > inheriting `no` from `nb` will fix the issue. LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8394
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:29:35 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > 8285452: updated to use lines() test/jdk/java/nio/file/Files/FileUtilsTest.java line 111: > 109: test(abcList, 1, 3, "ab", xyz, "xyz"); > 110: } > 111: Any thought of using TestNG with a DataProvider? Seems more efficient - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v3]
On Mon, 11 Apr 2022 14:55:32 GMT, Thomas Schatzl wrote: >> Hi all, >> >> can I have reviews for this change that adds dedicated filler objects to >> the VM? >> >> Currently, when formatting areas of dead objects all gcs use instances of >> j.l.Object and int-arrays. >> >> This has the drawback of not being easily able to discern whether a given >> object is actually dead (and should never be referenced) or just a regular >> j.l.Object/int array. >> >> This also makes enhanced error detection (any reference to such an object is >> an error - i.e. detecting references to such objects) and to skip >> potentially already unloaded classes when scanning areas of the heap below >> TAMS, G1 uses its prev bitmap. >> Other collectors do not have this extra information at the moment, so they >> can't (and don't) do this kind of verification. >> >> With [JDK-8210708](https://bugs.openjdk.java.net/browse/JDK-8210708) the >> prev bitmap will effectively be removed in G1; G1 will format the dead areas >> with these filler objects to avoid coming across unloaded classes. This is >> fine wrt to normal operation, however, this looses the existing enhanced >> verification mentioned above. >> >> This change proposes to add dedicated VM-internal filler objects, i.e. >> equivalents of j.l.Object and int-arrays. >> >> This has the following benefits: >> >> - keep this error detection (actually making it much simpler) and allowing >> similar verification for other collectors. (This change does not add this) >> >> - this also makes it "easy" to detect references to filler objects in >> debugging tools - you only need to know the two klasses (or just get their >> friendly name) to see whether that reference may actually be valid (or >> refers to the inside such an object). References to these classes in the >> crash file may also allow the issue to be more clear. >> >> This causes some minor changes to external behavior: >> >> - logs/heap dumps now contain instances of these objects - which seems fine >> as previously they have just been reported as part of j.l.Object/int-arrays >> statistics. The VM spec also does not guarantee whether a particular kind of >> object should/should not show there anyway afaik. >> >> - if the application ever gets to instantiate a reference to such an object >> somehow, any enabled verification will crash the VM. That's bad luck for >> messing with internal classes, but that's the purpose of these objects. >> >> The change takes care that getting a reference will not be possible by >> normal means (i.e. via Class.forName() etc) - which should be sufficient to >> avoid the issue. Actually, existing mechanisms seem to be sufficient. >> >> >> Testing: tier1-8 >> >> There is one question I would like the reviewers to specially think about, >> the name of the filler array klass. I just used >> `Ljava/internal/vm/FillerArray;` for that, looking at other internal >> symbols/klasses, but I'm not sure this adheres to naming guidelines. >> >> Thanks go to @iklam for helping out with the change. >> >> Thanks, >> Thomas > > Thomas Schatzl has updated the pull request incrementally with one additional > commit since the last revision: > > Fix test The latest version looks good to me. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8156
Re: RFR: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8465
Re: RFR: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs Thanks for fixing these Pavel. - Marked as reviewed by darcy (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8465
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v4]
On Fri, 29 Apr 2022 06:35:44 GMT, Jie Fu wrote: >> Hi all, >> >> The Current Vector API doc for `LSHR` is >> >> Produce a>>>(n&(ESIZE*8-1)). Integral only. >> >> >> This is misleading which may lead to bugs for Java developers. >> This is because for negative byte/short elements, the results computed by >> `LSHR` will be different from that of `>>>`. >> For more details, please see >> https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . >> >> After the patch, the doc for `LSHR` is >> >> Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral >> only. >> >> >> Thanks. >> Best regards, >> Jie > > Jie Fu has updated the pull request with a new target base due to a merge or > a rebase. The incremental webrev excludes the unrelated changes brought in by > the merge/rebase. The pull request contains eight additional commits since > the last revision: > > - Address CSR review comments > - Merge branch 'master' into JDK-8284992 > - Address review comments > - Merge branch 'master' into JDK-8284992 > - Merge branch 'master' into JDK-8284992 > - Address review comments > - Merge branch 'master' into JDK-8284992 > - 8284992: Fix misleading Vector API doc for LSHR operator Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v5]
On Fri, 29 Apr 2022 12:02:59 GMT, Jaikiran Pai wrote:
>> Sibabrata Sahoo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> updated to add space character in begining of multiline string
>
> test/lib/jdk/test/lib/util/FileUtils.java line 389:
>
>> 387: * @throws IOException
>> 388: */
>> 389: public static void patch(Path path, int fromLine, int toLine,
>> String from, String to) throws IOException {
>
> Should this method check whether the `fromLine` and `toLine` are valid
> values? Things like, negative value or 0 or `toLine` being less than
> `fromLine`. I'm not familiar with the expectations of test library code -
> maybe those checks aren't necessary since this is a test util?
`lines.remove()` and `lines.subList()` will throw the correct exception. Since
you asked, we can add it.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 13:02:06 GMT, Weijun Wang wrote: >> Also calling trim() assumes that white spaces are not significant. This >> might not be the case in the general case (for instance - think of markdown >> files were leading spaces are significant). > > The comparison is intentionally made lax so the caller does not need to > provide the exact indentation or even new line characters. We think along > with `fromLine` and `toLine` this is enough to make sure we are not modifying > the wrong lines. Shouldn't the comparison be better implemented by the caller then, who will know whether spaces are important or not? That's why I had suggested taking a `Predicate` that could be called with each line removed, and the caller could interrupt the parsing by returning false when it detects a mismatch with what they expect. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v2]
> Removing the Duplicate keys present in XSLTErrorResources.java and > XPATHErrorResources.java > > The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097 Shruthi has updated the pull request incrementally with one additional commit since the last revision: Updating last modified tag and XRTreeFragSelectWrapper.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/8318/files - new: https://git.openjdk.java.net/jdk/pull/8318/files/8c93a25b..d53ca37e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8318=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8318=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8318.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8318/head:pull/8318 PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8285915: failure_handler: gather the contents of /etc/hosts file
On Fri, 29 Apr 2022 12:51:21 GMT, Jaikiran Pai wrote: > Quick question - the path you note, is that even applicable for x64? I see > that it has a "System32" so just curious. Yes, System32 is not related to 32 vs 64 bit. As I understand it, that name was introduced when moving from 16 to 32 bit. - PR: https://git.openjdk.java.net/jdk/pull/8466
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:44:26 GMT, Daniel Fuchs wrote:
>> test/lib/jdk/test/lib/util/FileUtils.java line 394:
>>
>>> 392: var removed = "";
>>> 393: for (int i = fromLine; i <= toLine; i++) {
>>> 394: removed += lines.remove(fromLine - 1).trim();
>>
>> shouldn't you insert a "\n" ? otherwise concatenating lines "ab" and "c"
>> will be the same as concatenating lines "a" and "bc".
>
> Also calling trim() assumes that white spaces are not significant. This might
> not be the case in the general case (for instance - think of markdown files
> were leading spaces are significant).
The comparison is intentionally made lax so the caller does not need to provide
the exact indentation or even new line characters. We think along with
`fromLine` and `toLine` this is enough to make sure we are not modifying the
wrong lines.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285915: failure_handler: gather the contents of /etc/hosts file
On Fri, 29 Apr 2022 11:28:32 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which addresses > https://bugs.openjdk.java.net/browse/JDK-8285915? > > With this change, the environment details collected by the failure handler > will now include the contents of the `/etc/hosts/` file, which can be useful > in certain cases when debugging network tests that fail. > > Testing done (on macOS): > > > cd test/failure_handler > make test > > Then verified that the generated environment.html had the `/etc/hosts` file > content Hello Erik, > Not sure if it's relevant, but did you consider doing this for Windows as > well? The file is located at `"$WINDIR/System32/drivers/etc/hosts"`. I hadn't investigated what the corresponding command would be for Windows, so had left it out. Quick question - the path you note, is that even applicable for x64? I see that it has a "System32" so just curious. I'll experiment a bit shortly against some CI setups to see how this goes on Windows. - PR: https://git.openjdk.java.net/jdk/pull/8466
Re: RFR: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs GSSHeader looks fine. - Marked as reviewed by mullan (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8465
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:35:59 GMT, Daniel Fuchs wrote:
>> Sibabrata Sahoo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8285452: updated to use lines()
>
> test/lib/jdk/test/lib/util/FileUtils.java line 394:
>
>> 392: var removed = "";
>> 393: for (int i = fromLine; i <= toLine; i++) {
>> 394: removed += lines.remove(fromLine - 1).trim();
>
> shouldn't you insert a "\n" ? otherwise concatenating lines "ab" and "c" will
> be the same as concatenating lines "a" and "bc".
Also calling trim() assumes that white spaces are not significant. This might
not be the case in the general case (for instance - think of markdown files
were leading spaces are significant).
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285915: failure_handler: gather the contents of /etc/hosts file
On Fri, 29 Apr 2022 11:28:32 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which addresses > https://bugs.openjdk.java.net/browse/JDK-8285915? > > With this change, the environment details collected by the failure handler > will now include the contents of the `/etc/hosts/` file, which can be useful > in certain cases when debugging network tests that fail. > > Testing done (on macOS): > > > cd test/failure_handler > make test > > Then verified that the generated environment.html had the `/etc/hosts` file > content Marked as reviewed by erikj (Reviewer). Not sure if it's relevant, but did you consider doing this for Windows as well? The file is located at `"$WINDIR/System32/drivers/etc/hosts"`. - PR: https://git.openjdk.java.net/jdk/pull/8466
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:29:35 GMT, Sibabrata Sahoo wrote:
>> A new API to support replacing selective lines with desired content.
>
> Sibabrata Sahoo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8285452: updated to use lines()
test/lib/jdk/test/lib/util/FileUtils.java line 394:
> 392: var removed = "";
> 393: for (int i = fromLine; i <= toLine; i++) {
> 394: removed += lines.remove(fromLine - 1).trim();
shouldn't you insert a "\n" ? otherwise concatenating lines "ab" and "c" will
be the same as concatenating lines "a" and "bc".
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
> A new API to support replacing selective lines with desired content. Sibabrata Sahoo has updated the pull request incrementally with one additional commit since the last revision: 8285452: updated to use lines() - Changes: - all: https://git.openjdk.java.net/jdk/pull/8360/files - new: https://git.openjdk.java.net/jdk/pull/8360/files/da6a214a..0b7dc2f9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8360=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8360=05-06 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8360.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8360/head:pull/8360 PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v6]
> A new API to support replacing selective lines with desired content. Sibabrata Sahoo has updated the pull request incrementally with one additional commit since the last revision: 8285452: updated to from.lines() - Changes: - all: https://git.openjdk.java.net/jdk/pull/8360/files - new: https://git.openjdk.java.net/jdk/pull/8360/files/e18cd8cc..da6a214a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8360=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8360=04-05 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8360.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8360/head:pull/8360 PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285915: failure_handler: gather the contents of /etc/hosts file
On Fri, 29 Apr 2022 11:28:32 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which addresses > https://bugs.openjdk.java.net/browse/JDK-8285915? > > With this change, the environment details collected by the failure handler > will now include the contents of the `/etc/hosts/` file, which can be useful > in certain cases when debugging network tests that fail. > > Testing done (on macOS): > > > cd test/failure_handler > make test > > Then verified that the generated environment.html had the `/etc/hosts` file > content Thanks for doing this Jaikiran! That should be helpful. Please get approval from someone from build-dev before integrating. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8466
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v5]
On Fri, 29 Apr 2022 10:46:31 GMT, Sibabrata Sahoo wrote:
>> A new API to support replacing selective lines with desired content.
>
> Sibabrata Sahoo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> updated to add space character in begining of multiline string
test/lib/jdk/test/lib/util/FileUtils.java line 383:
> 381: * Patches a part of a file.
> 382: * @param path of file
> 383: * @param fromLine the first line to patch. This is the number you
> see in an editor, 1-based.
Perhaps this should mention whether the `fromLine` is inclusive, like it's
noted for the `toLine`?
test/lib/jdk/test/lib/util/FileUtils.java line 389:
> 387: * @throws IOException
> 388: */
> 389: public static void patch(Path path, int fromLine, int toLine, String
> from, String to) throws IOException {
Should this method check whether the `fromLine` and `toLine` are valid values?
Things like, negative value or 0 or `toLine` being less than `fromLine`. I'm
not familiar with the expectations of test library code - maybe those checks
aren't necessary since this is a test util?
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v4]
On Fri, 29 Apr 2022 10:36:50 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > 8285452: Add a new test library API to replace a file content using > FileUtils.java test/lib/jdk/test/lib/util/FileUtils.java line 396: > 394: removed += lines.remove(fromLine - 1).trim(); > 395: } > 396: var froms = > Arrays.asList(from.split(System.lineSeparator())).stream() How about just using `from.lines()`? - PR: https://git.openjdk.java.net/jdk/pull/8360
RFR: 8285915: failure_handler: gather the contents of /etc/hosts file
Can I please get a review of this change which addresses https://bugs.openjdk.java.net/browse/JDK-8285915? With this change, the environment details collected by the failure handler will now include the contents of the `/etc/hosts/` file, which can be useful in certain cases when debugging network tests that fail. Testing done (on macOS): cd test/failure_handler make test Then verified that the generated environment.html had the `/etc/hosts` file content - Commit messages: - 8285915: failure_handler: gather the contents of /etc/hosts file Changes: https://git.openjdk.java.net/jdk/pull/8466/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8466=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285915 Stats: 8 lines in 2 files changed: 6 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8466.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8466/head:pull/8466 PR: https://git.openjdk.java.net/jdk/pull/8466
Re: RFR: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8465
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v5]
On Fri, 29 Apr 2022 10:46:58 GMT, Sibabrata Sahoo wrote: >> Sibabrata Sahoo has updated the pull request incrementally with one >> additional commit since the last revision: >> >> updated to add space character in begining of multiline string > > test/jdk/java/nio/file/Files/FileUtilsTest.java line 51: > >> 49: c"""; >> 50: // 1st line has a space character >> 51: String sabc = " " + System.lineSeparator() + abc; > > It's strange that jcheck fails, if there is space character in beginning of > line in a multiline string. So i have to follow this way add a space > character in the beginning of multiline string. You can use \s instead of space. Then you will have no complaints. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v5]
On Fri, 29 Apr 2022 10:46:31 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > updated to add space character in begining of multiline string test/jdk/java/nio/file/Files/FileUtilsTest.java line 51: > 49: c"""; > 50: // 1st line has a space character > 51: String sabc = " " + System.lineSeparator() + abc; It's strange that jcheck fails, if there is space character in beginning of line in a multiline string. So i have to follow this way add a space character in the beginning of multiline string. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v5]
> A new API to support replacing selective lines with desired content. Sibabrata Sahoo has updated the pull request incrementally with one additional commit since the last revision: updated to add space character in begining of multiline string - Changes: - all: https://git.openjdk.java.net/jdk/pull/8360/files - new: https://git.openjdk.java.net/jdk/pull/8360/files/14125936..e18cd8cc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8360=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8360=03-04 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8360.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8360/head:pull/8360 PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v4]
> A new API to support replacing selective lines with desired content. Sibabrata Sahoo has updated the pull request incrementally with one additional commit since the last revision: 8285452: Add a new test library API to replace a file content using FileUtils.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/8360/files - new: https://git.openjdk.java.net/jdk/pull/8360/files/ef5dc31a..14125936 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8360=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8360=02-03 Stats: 143 lines in 2 files changed: 140 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8360.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8360/head:pull/8360 PR: https://git.openjdk.java.net/jdk/pull/8360
Re: [External] : Re: java.net.http.HttpClient do NOT support Digest authentication while HttpURLConnection do
Hi, Thanks for the pointers. > Just to mention a few: > https://urldefense.com/v3/__https://www.baeldung.com/java-9-http-client__;!!ACWV5N9M2RV99hQ!MbomkSmr-rXolK88z_JdINs3IEG8MTH_B7DHDq6wYgUeJQvZNGT7Iwb3yFswd57S1l85R53Yo_zq8zIvWdw$ > https://urldefense.com/v3/__https://developer.ibm.com/tutorials/java-theory-and-practice-3/__;!!ACWV5N9M2RV99hQ!MbomkSmr-rXolK88z_JdINs3IEG8MTH_B7DHDq6wYgUeJQvZNGT7Iwb3yFswd57S1l85R53Yo_zqgE8E3ik$ "An Authenticator is an object that negotiates credentials (HTTP authentication) for a connection. It provides different authentication schemes (such as basic or digest authentication)." I believe that's a misunderstanding: you will notice that the text talks about the Authenticator - not the HttpClient. But I do agree that this text is misleading (even WRT Authenticator). HttpURLConnection does support digest authentication out of the box. It was a choice we made to not support any authentication scheme out of the box, except Basic, in the new API. As I said Digest can be easily implemented at the application level if you need it, by handling the 401/407 responses at the application level. best regards, -- daniel
Re: java.net.http.HttpClient do NOT support Digest authentication while HttpURLConnection do
Hi,
Just to mention a few:
https://www.baeldung.com/java-9-http-client
https://developer.ibm.com/tutorials/java-theory-and-practice-3/
Also from this issue also assume it's support:
https://bugs.openjdk.java.net/browse/JDK-8138990
and this issue also contains a reference to:
http://hg.openjdk.java.net/jdk9/dev/jdk/file/4204dbf90380/src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
which makes assume it IS already implemented and as I wrote
HttpURLConnection also support it.
That's why I suppose just should have to in add it to HttpClient.
IMHO it's still a big barrier to use HttpClient instead of the other 3th
party http client implementations:
https://www.mocklab.io/blog/which-java-http-client-should-i-use-in-2020/
as a quote from:
https://medium.com/@kir.maxim/lesson-i-have-learned-from-using-jdk11-http-client-2cf990daba03
"JDK11 HTTP client supports only basic authentication scheme. From my
point of view, this will block users from migrating their code to use
the new library."
What's more even you wrote
"It can be easily implemented at the application level"
there is not ANY such public implementation...
Probably it has reasons...
Regards.
Levente "Si vis pacem para bellum!"
On 2022. 04. 29. 11:34, Daniel Fuchs wrote:
Hi,
java.net.http.HttpClient only supports Basic authentication out
of the box.
Which tutorials are claiming that Digest authentication is supported?
Can you send some links?
At the moment there is no plan to support digest authentication out of
the box. It can be easily implemented at the application level on top
of the existing APIs, by *not* registering an authenticator with
the client and dealing with 401/407 response from the application code.
best regards,
-- daniel
On 28/04/2022 23:40, Farkas Levente wrote:
Hi,
Even though many tutorial said that the new java.net.http.HttpClient
support Digest authentication it's not true:
https://github.com/openjdk/jdk/blob/master/src/java.net.http/share/classes/jdk/internal/net/http/AuthenticationFilter.java#L278
But at the same time HttpURLConnection support it through the simple:
---
Authenticator.setDefault(new Authenticator() {
@Override
protected PasswordAuthentication getPasswordAuthentication() {
return new PasswordAuthentication (
"username",
"password".toCharArray()
);
}
});
---
Is it a bug or you don't even plan to support it with HttpClient?
Regards.
Re: java.net.http.HttpClient do NOT support Digest authentication while HttpURLConnection do
Hi,
java.net.http.HttpClient only supports Basic authentication out
of the box.
Which tutorials are claiming that Digest authentication is supported?
Can you send some links?
At the moment there is no plan to support digest authentication out of
the box. It can be easily implemented at the application level on top
of the existing APIs, by *not* registering an authenticator with
the client and dealing with 401/407 response from the application code.
best regards,
-- daniel
On 28/04/2022 23:40, Farkas Levente wrote:
Hi,
Even though many tutorial said that the new java.net.http.HttpClient
support Digest authentication it's not true:
https://github.com/openjdk/jdk/blob/master/src/java.net.http/share/classes/jdk/internal/net/http/AuthenticationFilter.java#L278
But at the same time HttpURLConnection support it through the simple:
---
Authenticator.setDefault(new Authenticator() {
@Override
protected PasswordAuthentication getPasswordAuthentication() {
return new PasswordAuthentication (
"username",
"password".toCharArray()
);
}
});
---
Is it a bug or you don't even plan to support it with HttpClient?
Regards.
RFR: 8285890: Fix some @param tags
* Syntactically improves a few cases from 8285676 (https://github.com/openjdk/jdk/pull/8410) * Fixes a few misspelled `@param` tags for elements that, although are not included in the API Documentation, are visible in and processed by IDEs - Commit messages: - Fix misspelled `@param` - Clarify with whitespace tags from 8285676 Changes: https://git.openjdk.java.net/jdk/pull/8465/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8465=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285890 Stats: 16 lines in 8 files changed: 0 ins; 0 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/8465.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8465/head:pull/8465 PR: https://git.openjdk.java.net/jdk/pull/8465
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Fri, 29 Apr 2022 08:45:42 GMT, Pavel Rappo wrote: > Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files. I've created a PR; feel free to review it at your convenience. - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Thu, 28 Apr 2022 19:06:04 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/java/lang/ref/PhantomReference.java line 48:
>>
>>> 46: * The {@link #refersTo(Object) refersTo} method can be used to test
>>> 47: * whether some object is the referent of a phantom reference.
>>> 48: * @param the type of the referent
>>
>> Shouldn't there be a space after `@param` ?
>
> Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.
> Shouldn't there be a space after `@param` ?
> Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.
I built the API documentation after this PR has been integrated and the result
was okay. I saw this output in every such case:
Type Parameters:
T - the type of the referent
javadoc is quite robust. However, for some IDEs such missing whitespace seems
significant. Not only do they highlight the `@param` tag, but the type
parameter information is missing from the rendered output.
Although it's not critical, we should fix it; I have filed JDK-8285890.
-
PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
On Fri, 29 Apr 2022 08:06:24 GMT, Maurizio Cimadamore wrote: > would a jep unneeded be enough to "unstuck" this PR? Yes if no bug. Conceptually, the `/jep unneeded` will behave as no jep command. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v35]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak Addressable javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/945317ec..d1fcf012 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=34 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=33-34 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
On Fri, 29 Apr 2022 00:44:01 GMT, Guoxiong Li wrote: > Remind: please use the command `/jep JEP-424` [1] to mark this PR. > > [1] > https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/jep Question: I'm willing to try it out. If something goes wrong - would a `jep unneeded` be enough to "unstuck" this PR? - PR: https://git.openjdk.java.net/jdk/pull/7888
Integrated: 8285523: Improve test java/io/FileOutputStream/OpenNUL.java
On Mon, 25 Apr 2022 04:35:13 GMT, Sergey Bylokhov wrote: > The new test added as part of the > [JDK-8285445](https://bugs.openjdk.java.net/browse/JDK-8285445) cannot > trigger that bug and pass w/ and w/o fix. > > An updated test validates the "default" case when the `jdk.io.File.enableADS` > property is not set, in this case the ADS should be > [accepted](https://github.com/openjdk/jdk/blob/9d9f4e502f6ddc3116ed9b80f7168a1edfce839e/src/java.base/windows/classes/java/io/WinNTFileSystem.java#L59). This pull request has now been integrated. Changeset: f42631e3 Author:Sergey Bylokhov URL: https://git.openjdk.java.net/jdk/commit/f42631e354d4abf7994abd92aa5def6b2ceeab3a Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod 8285523: Improve test java/io/FileOutputStream/OpenNUL.java Reviewed-by: andrew, bpb - PR: https://git.openjdk.java.net/jdk/pull/8379
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings
On Fri, 29 Apr 2022 05:09:35 GMT, David Holmes wrote: > That only seems to be half of the issue though. If we are defining > _WIN32_WINNT=0x0601 because the minimum required OS API support level is > Windows 7, then don't we need a check that the build platform is also at > least Windows 7? Hi David, on older OS than Windows 7 we wouldn't be able to build OJDK anyway currently. Our oldest VS we still support (see toolchain_microsoft.m4) is VS2017. VS2017 has the following system requirements https://docs.microsoft.com/en-us/visualstudio/releases/2017/vs2017-system-requirements-vs see supported OS , there the oldest supported OS is Windows Server 2012 R2 and Windows 7 SP1. So on older than Win7 we would not even have a compiler anymore that passes configure. - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e I'm sorry for the noise with my comments. I'll continue to discuss it privately unless there is something really important. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
test/hotspot/jtreg/serviceability/jvmti/events/ClassPrepare/classprep01/libclassprep01.cpp
line 59:
> 57: static jvmtiEventCallbacks callbacks;
> 58: static jint result = PASSED;
> 59: static volatile size_t eventsCount = 0; // TODO these 2 vars mofified
> from different threads in getReady/check. What to DO???
I'd suggest to use RawMonitorLocker with event_lock or counter_lock to protect
updates of these counters.
You can remove this comment then.
test/hotspot/jtreg/serviceability/jvmti/events/ClassPrepare/classprep01/libclassprep01.cpp
line 201:
> 199: LOG("\n");
> 200:
> 201:
Too many empty lines. It might make sense to do a common cleanup with all edits
like this.
test/hotspot/jtreg/serviceability/jvmti/events/ClassPrepare/classprep01/libclassprep01.cpp
line 258:
> 256: return JNI_VERSION_1_8;
> 257: }
> 258: #endif
Empty line is missed => common cleanup.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e test/hotspot/jtreg/serviceability/jvmti/events/Exception/exception01/libexception01.cpp line 178: > 176:ex.c_cls, ex.c_name, ex.c_sig, > 177:(jint)(ex.c_loc >> 32), (jint)ex.c_loc); > 178: result = STATUS_FAILED; Unaligned lines in the range: 172-177. There are some non-uinified unneeded spaces at lines 172,175. test/hotspot/jtreg/serviceability/jvmti/events/Exception/exception01/libexception01.cpp line 287: > 285: } > 286: > 287: eventsCount = 0; Counters are not protected with locks. I'm not sure it is a real problem here but would be better to double-check. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v4]
> Hi all, > > The Current Vector API doc for `LSHR` is > > Produce a>>>(n&(ESIZE*8-1)). Integral only. > > > This is misleading which may lead to bugs for Java developers. > This is because for negative byte/short elements, the results computed by > `LSHR` will be different from that of `>>>`. > For more details, please see > https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . > > After the patch, the doc for `LSHR` is > > Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. > > > Thanks. > Best regards, > Jie Jie Fu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Address CSR review comments - Merge branch 'master' into JDK-8284992 - Address review comments - Merge branch 'master' into JDK-8284992 - Merge branch 'master' into JDK-8284992 - Address review comments - Merge branch 'master' into JDK-8284992 - 8284992: Fix misleading Vector API doc for LSHR operator - Changes: - all: https://git.openjdk.java.net/jdk/pull/8291/files - new: https://git.openjdk.java.net/jdk/pull/8291/files/7e82e721..0161571b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8291=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8291=02-03 Stats: 6657 lines in 233 files changed: 5591 ins; 490 del; 576 mod Patch: https://git.openjdk.java.net/jdk/pull/8291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8291/head:pull/8291 PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
On Thu, 28 Apr 2022 19:48:18 GMT, Paul Sandoz wrote: >> Jie Fu has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains six additional commits since >> the last revision: >> >> - Address review comments >> - Merge branch 'master' into JDK-8284992 >> - Merge branch 'master' into JDK-8284992 >> - Address review comments >> - Merge branch 'master' into JDK-8284992 >> - 8284992: Fix misleading Vector API doc for LSHR operator > > It should be possible for you finalize now. Hi @PaulSandoz , the CSR had been approved and I pushed one more commit to address the CSR review comments. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e test/hotspot/jtreg/serviceability/jvmti/events/ClassLoad/classload01/libclassload01.cpp line 139: > 137: primClsEvents[i]++; > 138: LOG( > 139: "TEST FAILED: JVMTI_EVENT_CLASS_LOAD event received for\n" Combine 138+138. I won't comment this more in hope the same will be fixed in all tests. test/hotspot/jtreg/serviceability/jvmti/events/ClassLoad/classload01/libclassload01.cpp line 177: > 175: return JNI_VERSION_1_8; > 176: } > 177: #endif One empty line would be nice to add after 177. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp
line 139:
> 137:thr_info.name,(jni->IsVirtualThread(thread) ==
> JNI_TRUE) ? "virtual" : "kernel",
> 138: (thr_info.is_daemon == JNI_TRUE) ? "deamon" : "user");
> 139: }
I'd suggest to add locals (their names are up to you):
const char* thr_virtual_tag = jni->IsVirtualThread(thread) == JNI_TRUE ?
"virtual" : "kernel";
const char* thr_daemon_tag == JNI_TRUE) ? "deamon" : "user";
It is better to place togeter lines: 129+130.
Line 137 is not aligned, and there are many unneeded spaces.
The '()' around conditions are not needed. At least, I do not see them
increasing readability.
test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp
line 167:
> 165: result = checkStatus = STATUS_FAILED;
> 166: LOG(
> 167: "TEST FAILED: Breakpoint event with unexpected class
> signature:\n"
Combine lines 166+167.
test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp
line 187:
> 185: jboolean isVirtual = jni->IsVirtualThread(thread);
> 186: if (isVirtual != METHODS_ATTRS[i]) {
> 187: LOG("TEST FAILED: IsVirtualThread check failed with unexpected
> result %d when expected is %d\n", isVirtual, METHODS_ATTRS[i]);
Line 187 is too long and can be splitted.
test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp
line 231:
> 229: result = STATUS_FAILED;
> 230: LOG(
> 231: "TEST FAILED: wrong number of Breakpoint events\n"
Combine 230+231.
test/hotspot/jtreg/serviceability/jvmti/events/Breakpoint/breakpoint01/libbreakpoint01.cpp
line 307:
> 305: if (agent_lock == NULL) {
> 306: return JNI_ERR;
> 307: }
Better to also use same style with curly brackets in fragments: 293, 296, 299.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
