Re: RFR: 8278892: java.naming module description is missing @uses tags to document the services that it uses
On Wed, 12 Jan 2022 05:48:26 GMT, Masanori Yano wrote: > I have fixed the javadoc comments as the definition. Could you review this > fix? Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7041
Integrated: Merge jdk18
On Tue, 18 Jan 2022 01:13:45 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 18 -> JDK 19 This pull request has now been integrated. Changeset: 39f140a2 Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/39f140a20120300074167597580f9be34e812cad Stats: 403 lines in 9 files changed: 361 ins; 4 del; 38 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/7119
Re: RFR: JDK-8279954 - java/lang/StringBuffer(StringBuilder)/HugeCapacity.java intermittently fails
On Fri, 14 Jan 2022 14:33:13 GMT, Jim Laskey wrote: > Tests were fatally failing (windows) on Github actions. Pumping up the memory > requirements will hopefully alleviate. I also pointed out that we see the same error message in our CI and the infra folk assured me that the paging file was not misconfigured in any way. That said, while I understand the addition of the `@requires >= 8G`, I don't under stand why the actual Xms/Xmx values were bumped from 5G to 6G - that seems to be increasing the chances of running out of memory. ?? David - PR: https://git.openjdk.java.net/jdk/pull/7086
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v4]
On Mon, 17 Jan 2022 18:10:02 GMT, Lance Andersen wrote: >> Masanori Yano has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8272746: ZipFile can't open big file (NegativeArraySizeException) > > test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java line 82: > >> 80: >> 81: /** >> 82: * Validates that an appropriate exception is thrown when the >> ZipFile class > > Please change "appropriate" to "ZipException" Thank you very much. I fixed it. - PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v5]
> Could you please review the JDK-8272746 bug fixes? > Since the array index is of type int, the overflow occurs when the value of > end.cenlen is too large because of too many entries. > It is necessary to read a part of the CEN from the file to fix the problem > fundamentally, but the way will require an extensive fix and degrade > performance. > In practical terms, the size of the central directory rarely grows that > large. So, I fixed it to check the size of the central directory and throw an > exception if it is too large. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8272746: ZipFile can't open big file (NegativeArraySizeException) - Changes: - all: https://git.openjdk.java.net/jdk/pull/6927/files - new: https://git.openjdk.java.net/jdk/pull/6927/files/9ebb2a7a..c6b53732 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6927=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6927=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6927.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6927/head:pull/6927 PR: https://git.openjdk.java.net/jdk/pull/6927
RFR: Merge jdk18
Forwardport JDK 18 -> JDK 19 - Commit messages: - Merge remote-tracking branch 'jdk18/master' into Merge_jdk18 - 8279998: PPC64 debug builds fail with "untested: RangeCheckStub: predicate_failed_trap_id" - 8280034: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64 - 8279924: [PPC64, s390] implement frame::is_interpreted_frame_valid checks - 8279702: [macosx] ignore xcodebuild warnings on M1 - 8279930: Synthetic cast causes generation of store barriers when using heap segments - 8279597: [TESTBUG] ReturnBlobToWrongHeapTest.java fails with -XX:TieredStopAtLevel=1 on machines with many cores - 8278434: timeouts in test java/time/test/java/time/format/TestZoneTextPrinterParser.java The webrevs contain the adjustments done while merging with regards to each parent branch: - master: https://webrevs.openjdk.java.net/?repo=jdk=7119=00.0 - jdk18: https://webrevs.openjdk.java.net/?repo=jdk=7119=00.1 Changes: https://git.openjdk.java.net/jdk/pull/7119/files Stats: 403 lines in 9 files changed: 361 ins; 4 del; 38 mod Patch: https://git.openjdk.java.net/jdk/pull/7119.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7119/head:pull/7119 PR: https://git.openjdk.java.net/jdk/pull/7119
Integrated: Merge jdk18
On Thu, 13 Jan 2022 20:55:49 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 18 -> JDK 19 This pull request has now been integrated. Changeset: 37143c09 Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/37143c09ab56ff07767ab3ac392234e36ee82358 Stats: 124 lines in 6 files changed: 117 ins; 1 del; 6 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/7068
Re: RFR: 8278892: java.naming module description is missing @uses tags to document the services that it uses
On Wed, 12 Jan 2022 05:48:26 GMT, Masanori Yano wrote: > I have fixed the javadoc comments as the definition. Could you review this > fix? Hi @masyano, Thanks for fixing `java.naming` module javadoc. The fix looks fine to me. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/7041
Re: RFR: 8275731: CDS archived enums objects are recreated at runtime [v3]
On Sat, 11 Dec 2021 01:55:50 GMT, Ioi Lam wrote: >> **Background:** >> >> In the Java Language, Enums can be tested for equality, so the constants in >> an Enum type must be unique. Javac compiles an enum declaration like this: >> >> >> public enum Day { SUNDAY, MONDAY ... } >> >> >> to >> >> >> public class Day extends java.lang.Enum { >> public static final SUNDAY = new Day("SUNDAY"); >> public static final MONDAY = new Day("MONDAY"); ... >> } >> >> >> With CDS archived heap objects, `Day::` is executed twice: once >> during `java -Xshare:dump`, and once during normal JVM execution. If the >> archived heap objects references one of the Enum constants created at dump >> time, we will violate the uniqueness requirements of the Enum constants at >> runtime. See the test case in the description of >> [JDK-8275731](https://bugs.openjdk.java.net/browse/JDK-8275731) >> >> **Fix:** >> >> During -Xshare:dump, if we discovered that an Enum constant of type X is >> archived, we archive all constants of type X. At Runtime, type X will skip >> the normal execution of `X::`. Instead, we run >> `HeapShared::initialize_enum_klass()` to retrieve all the constants of X >> that were saved at dump time. >> >> This is safe as we know that `X::` has no observable side effect -- >> it only creates the constants of type X, as well as the synthetic value >> `X::$VALUES`, which cannot be observed until X is fully initialized. >> >> **Verification:** >> >> To avoid future problems, I added a new tool, CDSHeapVerifier, to look for >> similar problems where the archived heap objects reference a static field >> that may be recreated at runtime. There are some manual steps involved, but >> I analyzed the potential problems found by the tool are they are all safe >> (after the current bug is fixed). See cdsHeapVerifier.cpp for gory details. >> An example trace of this tool can be found at >> https://bugs.openjdk.java.net/secure/attachment/97242/enum_warning.txt >> >> **Testing:** >> >> Passed Oracle CI tiers 1-4. WIll run tier 5 as well. > > Ioi Lam has updated the pull request with a new target base due to a merge or > a rebase. The incremental webrev excludes the unrelated changes brought in by > the merge/rebase. The pull request contains four additional commits since the > last revision: > > - Merge branch 'master' into 8275731-heapshared-enum > - added exclusions needed by "java -Xshare:dump -ea -esa" > - Comments from @calvinccheung off-line > - 8275731: CDS archived enums objects are recreated at runtime I don't really know this code well enough to do a good code review. I had some comments though. src/hotspot/share/cds/cdsHeapVerifier.cpp line 165: > 163: > 164: ResourceMark rm; > 165: for (JavaFieldStream fs(ik); !fs.done(); fs.next()) { Can this call instead void InstanceKlass::do_local_static_fields(void f(fieldDescriptor*, Handle, TRAPS), Handle mirror, TRAPS) { and have this next few lines in the function? src/hotspot/share/cds/cdsHeapVerifier.cpp line 254: > 252: InstanceKlass* ik = InstanceKlass::cast(k); > 253: for (JavaFieldStream fs(ik); !fs.done(); fs.next()) { > 254: if (!fs.access_flags().is_static()) { same here. It only saves a couple of lines but then you can have the function outside this large function. src/hotspot/share/cds/cdsHeapVerifier.hpp line 52: > 50: mtClassShared, > 51: HeapShared::oop_hash> _table; > 52: Is this only used inside cdsHeapVerifier? if so it should be in the .cpp file. There's also an ArchiveableStaticFieldInfo. Not sure how they are related. src/hotspot/share/cds/heapShared.cpp line 433: > 431: oop mirror = k->java_mirror(); > 432: int i = 0; > 433: for (JavaFieldStream fs(k); !fs.done(); fs.next()) { This seems like it should also use InstanceKlass::do_local_static_fields. src/hotspot/share/cds/heapShared.cpp line 482: > 480: copy_open_objects(open_regions); > 481: > 482: CDSHeapVerifier::verify(); Should all this be DEBUG_ONLY ? src/hotspot/share/cds/heapShared.hpp line 236: > 234: oop _referrer; > 235: oop _obj; > 236: CachedOopInfo() :_subgraph_info(), _referrer(), _obj() {} Should these be initialized to nullptr? does this do this? - PR: https://git.openjdk.java.net/jdk/pull/6653
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v4]
On Mon, 17 Jan 2022 09:56:13 GMT, Masanori Yano wrote: >> Could you please review the JDK-8272746 bug fixes? >> Since the array index is of type int, the overflow occurs when the value of >> end.cenlen is too large because of too many entries. >> It is necessary to read a part of the CEN from the file to fix the problem >> fundamentally, but the way will require an extensive fix and degrade >> performance. >> In practical terms, the size of the central directory rarely grows that >> large. So, I fixed it to check the size of the central directory and throw >> an exception if it is too large. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8272746: ZipFile can't open big file (NegativeArraySizeException) Looks fine. One minor comment update but you can integrate after making the change test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java line 82: > 80: > 81: /** > 82: * Validates that an appropriate exception is thrown when the ZipFile > class Please change "appropriate" to "ZipException" - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v2]
On Sun, 19 Dec 2021 06:51:45 GMT, liach wrote: >> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), >> by design, duplicate initialization of ReflectionFactory should be safe as >> it performs side-effect-free property read actions, and the suggesting of >> making the `initted` field volatile cannot prevent concurrent initialization >> either; however, having `initted == true` published without the other >> fields' values is a possibility, which this patch addresses. >> >> This simulates what's done in `CallSite`'s constructor for >> `ConstantCallSite`. Please feel free to point out the problems with this >> patch, as I am relatively inexperienced in this field of fences and there >> are relatively less available documents. (Thanks to >> https://shipilev.net/blog/2014/on-the-fence-with-dependencies/) > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Just use volatile directly to ensure read order I will check the hotspot assembly generated for both the volatile and the stable fields to see if the annotation is a valid replacement. - PR: https://git.openjdk.java.net/jdk/pull/6889
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Mon, 17 Jan 2022 06:32:13 GMT, Prasadrao Koppula wrote: >> This system property should only be used for TLS, and the CBT can be used in >> both the SPNEGO mechanism and the Kerberos 5 mechanism. Therefore I suggest >> the name should probably contain "tls" (or maybe "https") and "negotiate". >> >> BTW, will you reuse this system property if we decide to support CBT in NTLM >> as well? > > I vote for "jdk.https.tls.cbt" > It's actually a purely system property rather than a Net property at the > moment (same as the other spnego ones). Maybe, I should convert them all to > net properties, so they can be documented/set in that file? AFAICS this file documents properties used by the networking task - not necessarily net properties (e.g. java.net.preferIPv6Addresses is documented there but AFAICT it is a plain system property) - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Sat, 15 Jan 2022 00:49:05 GMT, Weijun Wang wrote: >> Argh - you're right I missed the fact that the 3 expressions where included >> in parenthesis. I read it as >> >> ! (s.equals("always")) || ... > > Shall we log a message if the value is not one of the 3 forms? Usually malformed values are just ignored - and the property takes its default value. But yes - s.n.w.h.HttpClient has a logger so it wouldn't be much effort to log it as a DEBUG trace for better diagnostic. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Thu, 13 Jan 2022 12:10:11 GMT, Michael McMahon wrote: > Hi, > > This change adds Channel Binding Token (CBT) support to HTTPS > (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) > authentication scheme. When enabled, the implementation preemptively includes > a CBT with authentication requests over Kerberos. The feature is enabled as > follows: > > A system property "jdk.spnego.cbt" is defined which can have the values > "never" (default), which means the feature is disabled, "always", which means > the CBT is included for all https Negotiate authentications, or it can take > the form "domain:a,b.c,*.d.com" which is a comma separated list of > domains/hosts where the feature is enabled, and disabled everywhere else. In > the given example, the CBT would be included in authentication requests for > hosts "a", "b.c" and all hosts under the domain "d.com" and all of its > sub-domains. > > A test will be added separately to the implementation. > > Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842 > > Thanks, > Michael Add Release note and Doc subtasks to the JBS - PR: https://git.openjdk.java.net/jdk/pull/7065
Integrated: 8278831: Use table lookup for the last two bytes in Integer.getChars
On Wed, 15 Dec 2021 23:04:37 GMT, Claes Redestad wrote: > During TemplatedStrings work Jim has noticed that we could probably profit > from reusing the lookup tables also for the 1 or 2 leftmost bytes: > > // We know there are at most two digits left at this point. > buf[--charPos] = DigitOnes[-i]; > if (i < -9) { > buf[--charPos] = DigitTens[-i]; > } > > This avoids some arithmetic, and on average achieves a small speed-up since > the lookup tables are likely cached are likely to be in cache. Small > improvements on a few affected microbenchmarks, including the new > Integers.toStringTiny, can be observed. > > Baseline: > > Benchmark (size) Mode Cnt Score Error Units > Integers.toStringTiny 500 avgt 50 21.862 ± 0.211 us/op > > > Patch: > > Benchmark (size) Mode Cnt Score Error Units > Integers.toStringTiny 500 avgt 50 20.619 ± 0.330 us/op This pull request has now been integrated. Changeset: 71ca85f5 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/71ca85f5a6741a2db55a529192564f94b269fbd9 Stats: 42 lines in 4 files changed: 11 ins; 16 del; 15 mod 8278831: Use table lookup for the last two bytes in Integer.getChars Reviewed-by: jlaskey, rriggs - PR: https://git.openjdk.java.net/jdk/pull/6854
Re: RFR: 8278892: java.naming module description is missing @uses tags to document the services that it uses
On Wed, 12 Jan 2022 05:48:26 GMT, Masanori Yano wrote: > I have fixed the javadoc comments as the definition. Could you review this > fix? Could someone please review this pull request? - PR: https://git.openjdk.java.net/jdk/pull/7041
Re: Thread.dispatchUncaughtException possible NPE?
- Original Message - > From: "Andrey Turbanov" > To: "core-libs-dev" > Sent: Monday, January 17, 2022 10:37:04 AM > Subject: Thread.dispatchUncaughtException possible NPE? > Hello. Hello Andrey, > I see that Thread.dispatchUncaughtException calls > getUncaughtExceptionHandler() which reads volatile field twice: > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Thread.java#L1978 > > private volatile UncaughtExceptionHandler uncaughtExceptionHandler; > > public UncaughtExceptionHandler getUncaughtExceptionHandler() { >return uncaughtExceptionHandler != null ? >uncaughtExceptionHandler : group; > } > > private void dispatchUncaughtException(Throwable e) { >getUncaughtExceptionHandler().uncaughtException(this, e); > } > > > I wonder if it's possible to get a NPE here? Another thread could > change uncaughtExceptionHandler between 2 volatile reads. > Or JVM somehow forbid this? yes, it's a bug, the field should be read only once. > > > > > Andrey Turbanov Rémi
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v4]
> Could you please review the JDK-8272746 bug fixes? > Since the array index is of type int, the overflow occurs when the value of > end.cenlen is too large because of too many entries. > It is necessary to read a part of the CEN from the file to fix the problem > fundamentally, but the way will require an extensive fix and degrade > performance. > In practical terms, the size of the central directory rarely grows that > large. So, I fixed it to check the size of the central directory and throw an > exception if it is too large. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8272746: ZipFile can't open big file (NegativeArraySizeException) - Changes: - all: https://git.openjdk.java.net/jdk/pull/6927/files - new: https://git.openjdk.java.net/jdk/pull/6927/files/621f1a68..9ebb2a7a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6927=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6927=02-03 Stats: 14 lines in 1 file changed: 12 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6927.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6927/head:pull/6927 PR: https://git.openjdk.java.net/jdk/pull/6927
Re: RFR: 8272746: ZipFile can't open big file (NegativeArraySizeException) [v3]
On Fri, 14 Jan 2022 23:55:58 GMT, Lance Andersen wrote: >> Masanori Yano has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8272746: ZipFile can't open big file (NegativeArraySizeException) > > Thank you for making the changes. Overall it looks much better > > Please add comments describing your constants as well as a comment describing > the intent of your setup and test methods @LanceAndersen As you pointed out, I described the explanation of the test in the comment. Is there anything else I should fix? - PR: https://git.openjdk.java.net/jdk/pull/6927
Thread.dispatchUncaughtException possible NPE?
Hello. I see that Thread.dispatchUncaughtException calls getUncaughtExceptionHandler() which reads volatile field twice: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Thread.java#L1978 private volatile UncaughtExceptionHandler uncaughtExceptionHandler; public UncaughtExceptionHandler getUncaughtExceptionHandler() { return uncaughtExceptionHandler != null ? uncaughtExceptionHandler : group; } private void dispatchUncaughtException(Throwable e) { getUncaughtExceptionHandler().uncaughtException(this, e); } I wonder if it's possible to get a NPE here? Another thread could change uncaughtExceptionHandler between 2 volatile reads. Or JVM somehow forbid this? Andrey Turbanov
Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable
On Thu, 13 Jan 2022 08:39:04 GMT, Andrey Turbanov wrote: >> Method `Class.isAssignableFrom` is often used in form of: >> >> if (clazz.isAssignableFrom(obj.getClass())) { >> Such condition could be simplified to more shorter and performarnt code >> >> if (clazz.isInstance(obj)) { >> >> Replacement is equivalent if it's known that `obj != null`. >> In JDK codebase there are many such places. >> >> I tried to measure performance via JMH >> >> Class integerClass = Integer.class; >> Class numberClass = Number.class; >> >> Object integerObject = 45666; >> Object doubleObject = 8777d; >> >> @Benchmark >> public boolean integerInteger_isInstance() { >> return integerClass.isInstance(integerObject); >> } >> >> @Benchmark >> public boolean integerInteger_isAssignableFrom() { >> return integerClass.isAssignableFrom(integerObject.getClass()); >> } >> >> @Benchmark >> public boolean integerDouble_isInstance() { >> return integerClass.isInstance(doubleObject); >> } >> >> @Benchmark >> public boolean integerDouble_isAssignableFrom() { >> return integerClass.isAssignableFrom(doubleObject.getClass()); >> } >> >> @Benchmark >> public boolean numberDouble_isInstance() { >> return numberClass.isInstance(doubleObject); >> } >> >> @Benchmark >> public boolean numberDouble_isAssignableFrom() { >> return numberClass.isAssignableFrom(doubleObject.getClass()); >> } >> >> @Benchmark >> public boolean numberInteger_isInstance() { >> return numberClass.isInstance(integerObject); >> } >> >> @Benchmark >> public boolean numberInteger_isAssignableFrom() { >> return numberClass.isAssignableFrom(integerObject.getClass()); >> } >> >> @Benchmark >> public void numberIntegerDouble_isInstance(Blackhole bh) { >> bh.consume(numberClass.isInstance(integerObject)); >> bh.consume(numberClass.isInstance(doubleObject)); >> } >> >> @Benchmark >> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) { >> bh.consume(integerClass.isAssignableFrom(integerObject.getClass())); >> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass())); >> } >> >> `isInstance` is a bit faster than `isAssignableFrom` >> >> Benchmark Mode Cnt Score Error Units >> integerDouble_isAssignableFrom avgt5 1,173 ± 0,026 ns/op >> integerDouble_isInstance avgt5 0,939 ± 0,038 ns/op >> integerIntegerDouble_isAssignableFrom avgt5 2,106 ± 0,068 ns/op >> numberIntegerDouble_isInstance avgt5 1,516 ± 0,046 ns/op >> integerInteger_isAssignableFromavgt5 1,175 ± 0,029 ns/op >> integerInteger_isInstance avgt5 0,886 ± 0,017 ns/op >> numberDouble_isAssignableFrom avgt5 1,172 ± 0,007 ns/op >> numberDouble_isInstanceavgt5 0,891 ± 0,030 ns/op >> numberInteger_isAssignableFrom avgt5 1,169 ± 0,014 ns/op >> numberInteger_isInstance avgt5 0,887 ± 0,016 ns/op > > src/jdk.jfr/share/classes/jdk/jfr/internal/TypeLibrary.java line 405: > >> 403: Object res = m.invoke(a, new Object[0]); >> 404: if (res instanceof Annotation[]) { >> 405: for (Annotation rep : (Annotation[]) >> m.invoke(a, new Object[0])) { > > BTW it looks suspicious to have `m.invoke(a, new Object[0])` called again > here. Shouldn't it just reuse `res` variable instead? It looks unnecessary. Please feel free to fix. - PR: https://git.openjdk.java.net/jdk/pull/7061