Re: RFR: 8286849: Use @Stable for generic repositories
On Tue, 17 May 2022 04:40:50 GMT, liach wrote: > Generic repositories, the implementation detail for generic information in > core reflection, can be updated to use the `@Stable` annotation to replace > their `volatile` access. Their existing accessor code is already safe, > reading the cache fields only once. > > In addition, fixed potentially non-thread-safe `genericInfo` access in > `Method`, `Field`, and `RecordComponent`. Just curious about any performance benefits of this patch? - PR: https://git.openjdk.java.net/jdk/pull/8742
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: 8285523: Improve test java/io/FileOutputStream/OpenNUL.java
On Thu, 28 Apr 2022 15:43:09 GMT, Andrew John Hughes 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). > > test/jdk/java/io/FileOutputStream/OpenNUL.java line 31: > >> 29: * @run main/othervm OpenNUL >> 30: * @run main/othervm -Djdk.io.File.enableADS OpenNUL >> 31: * @run main/othervm -Djdk.io.File.enableADS=FalsE OpenNUL > > Is there a reason for the mixed case? Just to check equalsIgnoreCase is being > used? Yes only for equalsIgnoreCase - PR: https://git.openjdk.java.net/jdk/pull/8379
RFR: 8285523: The test for JDK-8285445 can be improved
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). - Commit messages: - Initial version of 8285523 Changes: https://git.openjdk.java.net/jdk/pull/8379/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8379=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285523 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8379.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8379/head:pull/8379 PR: https://git.openjdk.java.net/jdk/pull/8379
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 13:37:44 GMT, Alexey Ivanov wrote: >> ??? You want to check and cleanup if NewStringUTF fails. You only want to >> call SetObjectElementArray if NewStringUTF succeeds. > > Can `SetObjectElementArray` raise an exception? > The index is within the array length and we store a string. I assume > `cacheDirArray` is a string array. > ??? You want to check and cleanup if NewStringUTF fails. You only want to > call SetObjectElementArray if NewStringUTF succeeds. If the SetObjectArrayElement will throw an exception we will call the NewStringUTF while the exception is raised which is a kind of "jni" issue. If such an exception is possible we should check and cleanup. - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()
On Fri, 4 Mar 2022 13:25:12 GMT, Zhengyu Gu wrote:
> Please review this small patch that fixes a potential memory leak that
> exception return fails to release allocated `cacheDirs`
>
> Test:
>
> - [x] jdk_desktop on Linux x86_64
src/java.desktop/unix/native/common/awt/fontpath.c line 938:
> 936: while ((cnt < max) && (cacheDir =
> (*FcStrListNext)(cacheDirs))) {
> 937: jstr = (*env)->NewStringUTF(env, (const char*)cacheDir);
> 938: JNU_CHECK_EXCEPTION_AND_CLEANUP(env,
> (*FcStrListDone)(cacheDirs));
I think you do not need to create an additional macro here, just inline it and
call "(*FcStrListDone)(cacheDirs);" directly. Something like:
if (IS_NULL(jstr) {
(*FcStrListDone)(cacheDirs);
return;
}
Note that the "IS_NULL" is used in this file after NewStringUTF. Any objections?
src/java.desktop/unix/native/common/awt/fontpath.c line 940:
> 938: JNU_CHECK_EXCEPTION_AND_CLEANUP(env,
> (*FcStrListDone)(cacheDirs));
> 939:
> 940: (*env)->SetObjectArrayElement(env, cacheDirArray, cnt++,
> jstr);
Probably we should add the check+cleanup after the SetObjectArrayElement?
Otherwise, we may call NewStringUTF while an exception is raised by the
SetObjectArrayElement.
-
PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: JDK-8280534: Enable compile-time doclint reference checking
On Wed, 26 Jan 2022 20:05:07 GMT, Joe Darcy wrote:
> The changes in this PR on top of the out-for-review changes in
> https://git.openjdk.java.net/jdk/pull/7222 allow compile-time doclint
> checking to be enabled in all JDK modules.
>
> Typically, a @SuppressWarnings("doclint:refernce") annotation is added to
> declaration with javadoc blocks that have already had distinguished
> cross-module links added (JDK-8280492).
>
> One exception is in src/java.base/share/classes/java/net/package-info.java
> where the cross-module link was (for now) removed. Currently the
> SuppressWarnings annotation type is not declared to allow its annotations to
> be applied to package declarations. I'll look into amending that, but in the
> mean time, I think it is beneficial for the JDK build, and the base module in
> particular, to have compile-time doclint protections turned on.
Marked as reviewed by serb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/7237
Re: RFR: JDK-8280492: Address remaining doclint issues in JDK build
On Sat, 22 Jan 2022 21:09:03 GMT, Joe Darcy wrote: > Use presumed syntax that will be introduced by JDK-8280488. Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7189
Re: RFR: 8280010: Remove double buffering of InputStream for Properties.load
On Mon, 10 Jan 2022 20:46:36 GMT, Andrey Turbanov wrote: > `Properties.load` uses `java.util.Properties.LineReader`. LineReader already > buffers input stream. Hence wrapping InputStream in BufferedInputStream is > redundant. Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7021
Re: RFR: 8280010: Remove double buffering of InputStream for Properties.load
On Mon, 10 Jan 2022 20:46:36 GMT, Andrey Turbanov wrote: > `Properties.load` uses `java.util.Properties.LineReader`. LineReader already > buffers input stream. Hence wrapping InputStream in BufferedInputStream is > redundant. The code change looks fine, but can you please check the actual performance impact, will the perf be the same after the change? - PR: https://git.openjdk.java.net/jdk/pull/7021
Integrated: 8279134: Fix Amazon copyright in various files
On Wed, 22 Dec 2021 09:07:24 GMT, Sergey Bylokhov wrote: > This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8244094 > > Currently, some of the files in the OpenJDK repo have Amazon copyright > notices which are all slightly different and do not conform to Amazons > preferred copyright notice which is simply (intentionally without copyright > year): > > "Copyright Amazon.com Inc. or its affiliates. All Rights Reserved." > > @simonis @phohensee This pull request has now been integrated. Changeset: 7fea1032 Author:Sergey Bylokhov URL: https://git.openjdk.java.net/jdk/commit/7fea10327ed27fcf8eae474ca5b15c3b4bafff2a Stats: 15 lines in 14 files changed: 0 ins; 1 del; 14 mod 8279134: Fix Amazon copyright in various files Reviewed-by: xliu, phh - PR: https://git.openjdk.java.net/jdk/pull/6915
Re: RFR: 8279134: Fix Amazon copyright in various files [v2]
> This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8244094 > > Currently, some of the files in the OpenJDK repo have Amazon copyright > notices which are all slightly different and do not conform to Amazons > preferred copyright notice which is simply (intentionally without copyright > year): > > "Copyright Amazon.com Inc. or its affiliates. All Rights Reserved." > > @simonis @phohensee Sergey Bylokhov has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'master' into JDK-8279134 - Initial fix JDK-8279134 - Changes: - all: https://git.openjdk.java.net/jdk/pull/6915/files - new: https://git.openjdk.java.net/jdk/pull/6915/files/cb05f5bb..52c5d9c3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6915=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6915=00-01 Stats: 619 lines in 42 files changed: 446 ins; 100 del; 73 mod Patch: https://git.openjdk.java.net/jdk/pull/6915.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6915/head:pull/6915 PR: https://git.openjdk.java.net/jdk/pull/6915
RFR: 8279134: Fix Amazon copyright in various files
This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8244094 Currently, some of the files in the OpenJDK repo have Amazon copyright notices which are all slightly different and do not conform to Amazons preferred copyright notice which is simply (intentionally without copyright year): "Copyright Amazon.com Inc. or its affiliates. All Rights Reserved." @simonis @phohensee - Commit messages: - Initial fix JDK-8279134 Changes: https://git.openjdk.java.net/jdk/pull/6915/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6915=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8279134 Stats: 15 lines in 14 files changed: 0 ins; 1 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/6915.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6915/head:pull/6915 PR: https://git.openjdk.java.net/jdk/pull/6915
Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging [v2]
> At the time Java supported applets and webstart, a special mechanism for > launching various applications in one JVM was used to reduce memory usage and > each application was isolated from each other. > > This isolation was implemented via ThreadGroups where each application > created its own thread group and all data for the application was stored in > the context of its own group. > > Some parts of the JDK used ThreadGroups directly, others used special classes > like sun.awt.AppContext. > > Such sandboxing became useless since the plugin and webstart are no longer > supported and were removed in jdk11. > > Note that this is just a first step for the overall cleanup, here is my plan: > 1. Cleanup the usage of AppContext in the “java.util.logging.LogManager" > class in the java.base module. > 2. Cleanup the usage of TheadGroup in the JavaSound > 3. Cleanup the usage of TheadGroup in the JavaBeans > 4. Cleanup the usage of AppContext in the Swing > 5. Cleanup the usage of AppContext in the AWT > 6. Delete the AppContext class. > > The current PR is for the first step. So this is the request to delete the > usage of AppContext in the LogManager and related tests. > > Tested by tier1/tier2/tier3 and jdk_desktop tests. Sergey Bylokhov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - Merge branch 'master' into JDK-8273101 - Some tests deleted - Update the RootLevelInConfigFile test - Initial version of JDK-8273101 - Changes: https://git.openjdk.java.net/jdk/pull/5326/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5326=01 Stats: 1423 lines in 10 files changed: 0 ins; 1418 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5326.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5326/head:pull/5326 PR: https://git.openjdk.java.net/jdk/pull/5326
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation
On Thu, 2 Dec 2021 10:55:57 GMT, Andrew Leonard wrote: > This is the case at Adoptium for example, which uses the Mozilla trusted CA > certs. But they didn't think skipping this test was too strong a step? For example validation of the certs expiration is quite useful. I tried to update the test to take into account additional certs, but it caused a merge conflict each time the certs in OpenJDK are updated. Probably we can add a config file that can inject/override some info in the test(at least skip the checksum validation)? By default this config file will be empty and will not be modified in the OpenJDK, but the vendors will be able to modify it. @wangweij @rhalade what do you think? - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable determinsitic cacerts generation
On Wed, 1 Dec 2021 18:30:06 GMT, Andrew Leonard wrote: > Addition of a configure option --with-cacerts-src='user cacerts folder' to > allow developers to specify their own cacerts PEM folder for generation of > the cacerts store using the deterministic openjdk GenerateCacerts tool. > > Signed-off-by: Andrew Leonard I have a question related to the custom cacerts which can be added to the OpenJDK bundle. How do you pass the tests like test/jdk/sun/security/lib/cacerts/VerifyCACerts.java using that custom jdk bundle? Probably we can add an additional configuration to that test so it will check the custom cacerts passed to the build as well? - PR: https://git.openjdk.java.net/jdk/pull/6647
Integrated: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND
On Sat, 13 Nov 2021 23:16:22 GMT, Sergey Bylokhov wrote: > The ZipOutputStream class may create bogus zip data which cannot be opened by > the ZipFile. The root cause is how the comment field is stored by the > ZipOutputStream. According to the zip specification, the comment field should > not be longer than 0x bytes, and we try to validate the length of the > comment, but unfortunately, we do this after the comment was assigned > already. So if the application saves the comment based on the user's input > and then gets an exception from the ZipOutputStream.setComment() it may > assume that the comment is too long and it will be ignored, but it will be > saved as-is to the file. > > Please take a look at > [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117) > refactoring, and note: > * The comment field is assigned before the length check > * The null comment is ignored > > The current fix will move the length validation before being assigned and > will use the null comment as an empty text. Note that the behavior of the > null parameter is not specified in the method/class/package so we are free > here to implement it in any way, any thoughts/suggestions on which is better? This pull request has now been integrated. Changeset: e3243ee9 Author:Sergey Bylokhov URL: https://git.openjdk.java.net/jdk/commit/e3243ee963d074c892a0ed16a00fd91b440c96ac Stats: 117 lines in 2 files changed: 114 ins; 0 del; 3 mod 8277087: ZipException: zip END header not found at ZipFile#Source.findEND Reviewed-by: lancea - PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
On Wed, 17 Nov 2021 10:34:52 GMT, Sergey Bylokhov wrote: >>> Yes it would be nice to clarify that a null is accepted by setComment. When >>> null is specified, the comment length is written as 0 >> >> @mrserb Are you taking the spec clarification or should we just created >> another issue in JBS to track it? > > I would like to update the spec for the null value for this method, and > probably others in a separate CR, since this fix could be backported to the > early releases. Will create such CR after agreement. I have filed a separate issue: https://bugs.openjdk.java.net/browse/JDK-8277495 - PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Marked as reviewed by serb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
On Wed, 17 Nov 2021 18:48:00 GMT, Lance Andersen wrote: > Sorry if my point was not clear. I would prefer to have 1 test to exercise a > Zip file comment vs have tests in multiple areas. Expanding the existing test > in this case keeps the primary coverage in one location and makes it easier > for future maintainers. I have preferred to have separate tests for the separate use cases, other than one big testcases which cover all possible combinations of exceptions and parameters. > I am not suggesting to not make your change, I am suggesting to include this > change as well. I understood that you suggest adding this additional change as well, and I pointed out why it is not necessary, if that code will be executed with the long comment it will break the specification. - PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
On Wed, 17 Nov 2021 12:08:37 GMT, Lance Andersen wrote: > There appears to be a similar test, > open/test/jdk/java/util/zip/ZipFile/Comment.java, I think we probably want to > fold your changes into the existing test and possibly convert to use TestNG. I know that test, and I explicitly created a new one, since the old one covers the positive cases of reading the different comments from the data by the ZipFile including an empty comment. This one is different, it checks the different use-cases all of which cause to save the empty comment into the data. > If you prefer to keep this test separate, the test should have expanded > coverage to validate that a comment that is set can be successfully read back > and the test should be renamed as it does more than just validate an > Empty/null comment. It is already checked by the ZipFile test cases. > To be: writeBytes(comment, 0, 0, Math.min(comment.length, 0x)); > Which is done when writing an entry comment out in writeCEN. It has a different implementation because of different specifications, the writeCEN codepath specified to cut long comments and save the first part, this method specified an exception to be thrown if a comment is too long-> an empty comment is saved. - PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
On Wed, 17 Nov 2021 09:59:43 GMT, Alan Bateman wrote: >> ZipEntry::setComment indicates that the comment will be truncated if needed >> and ZipOutputStream takes care of this. >> >> Perhaps writeEND() should also be updated to something like: >> `writeBytes(comment, 0, Math.min(comment.length, 0x))` >> >> Which is similar to what happens in writeCEN >> >> Yes it would be nice to clarify that a null is accepted by setComment. When >> null is specified, the comment length is written as 0 > >> Yes it would be nice to clarify that a null is accepted by setComment. When >> null is specified, the comment length is written as 0 > > @mrserb Are you taking the spec clarification or should we just created > another issue in JBS to track it? I would like to update the spec for the null value for this method, and probably others in a separate CR, since this fix could be backported to the early releases. Will create such CR after agreement. - PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
> The ZipOutputStream class may create bogus zip data which cannot be opened by > the ZipFile. The root cause is how the comment field is stored by the > ZipOutputStream. According to the zip specification, the comment field should > not be longer than 0x bytes, and we try to validate the length of the > comment, but unfortunately, we do this after the comment was assigned > already. So if the application saves the comment based on the user's input > and then gets an exception from the ZipOutputStream.setComment() it may > assume that the comment is too long and it will be ignored, but it will be > saved as-is to the file. > > Please take a look at > [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117) > refactoring, and note: > * The comment field is assigned before the length check > * The null comment is ignored > > The current fix will move the length validation before being assigned and > will use the null comment as an empty text. Note that the behavior of the > null parameter is not specified in the method/class/package so we are free > here to implement it in any way, any thoughts/suggestions on which is better? Sergey Bylokhov has updated the pull request incrementally with one additional commit since the last revision: Update EmptyComment.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/6380/files - new: https://git.openjdk.java.net/jdk/pull/6380/files/5198d657..33617622 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6380=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6380=00-01 Stats: 50 lines in 1 file changed: 27 ins; 11 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/6380.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6380/head:pull/6380 PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
On Sun, 14 Nov 2021 16:14:54 GMT, Martin Buchholz wrote: >> Sergey Bylokhov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update EmptyComment.java > > test/jdk/java/util/zip/ZipOutputStream/EmptyComment.java line 88: > >> 86: byte[] data = baos.toByteArray(); >> 87: // Since the comment is empty this will be two last bytes >> 88: int pos = data.length - ZipFile.ENDHDR + ZipFile.ENDCOM; > > I would probably check that I could find Phil Katz' initials at data.length - > ZipFile.ENDHDR Done. - PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
On Sun, 14 Nov 2021 14:44:08 GMT, Lance Andersen wrote:
>> Sergey Bylokhov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update EmptyComment.java
>
> test/jdk/java/util/zip/ZipOutputStream/EmptyComment.java line 35:
>
>> 33: * @summary Verifies various use cases when the zip comment should be
>> empty
>> 34: */
>> 35: public final class EmptyComment {
>
> Please consider converting this to use TestNG and a DataProvider as it will
> simply the code even further
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/6380
RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND
The ZipOutputStream class may create bogus zip data which cannot be opened by the ZipFile. The root cause is how the comment field is stored by the ZipOutputStream. According to the zip specification, the comment field should not be longer than 0x bytes, and we try to validate the length of the comment, but unfortunately, we do this after the comment was assigned already. So if the application saves the comment based on the user's input and then gets an exception from the ZipOutputStream.setComment() it may assume that the comment is too long and it will be ignored, but it will be saved as-is to the file. Please take a look at [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117) refactoring, and note: * The comment field is assigned before the length check * The null comment is ignored The current fix will move the length validation before being assigned and will use the null comment as an empty text. Note that the behavior of the null parameter is not specified in the method/class/package so we are free here to implement it in any way, any thoughts/suggestions on which is better? - Commit messages: - Initial version JDK-8277087 Changes: https://git.openjdk.java.net/jdk/pull/6380/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6380=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277087 Stats: 101 lines in 2 files changed: 98 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6380.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6380/head:pull/6380 PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging
On Fri, 29 Oct 2021 16:32:43 GMT, Mandy Chung wrote: > The change looks okay. My suggestion is to get 1-6 all ready to push around > the same time. It's okay to have separate JBS issues and PRs. ok, I'll continue to work using the plan from the description. - PR: https://git.openjdk.java.net/jdk/pull/5326
Re: RFR: 8262297: ImageIO.write() method will throw IndexOutOfBoundsException
On Thu, 28 Oct 2021 14:30:20 GMT, Alan Bateman wrote:
>> Could you please review the 8262297 bug fixes?
>>
>> In this case, ImageIO.write() should throw java.io.IOException rather than
>> java.lang.IndexOutOfBoundsException. IndexOutOfBoundsException is caught and
>> wrapped in IIOException in ImageIO.write() with this fix. In addition,
>> IndexOutOfBoundsException is not expected to throw by
>> RandomAccessFile#write() according to its API specification. So it should be
>> fixed.
>
> src/java.base/share/classes/java/io/RandomAccessFile.java line 558:
>
>> 556: * @throws IndexOutOfBoundsException If {@code off} is negative,
>> 557: * {@code len} is negative, or {@code len} is greater
>> than
>> 558: * {@code b.length - off}
>
> The IOOBE is specified in the super interface, it's just not shown in the
> javadoc because it's a runtime exception. So I think what you want here is:
>
> @throws IndexOutOfBoundsException {@inheritDoc}
Unfortunately the parent class does not specify this exception via javadoc tags
like "@throws IndexOutOfBoundsException", but instead just describe it in the
method description
should we fix it separately from the imageio fix?
-
PR: https://git.openjdk.java.net/jdk/pull/6151
Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging
On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov wrote: > At the time Java supported applets and webstart, a special mechanism for > launching various applications in one JVM was used to reduce memory usage and > each application was isolated from each other. > > This isolation was implemented via ThreadGroups where each application > created its own thread group and all data for the application was stored in > the context of its own group. > > Some parts of the JDK used ThreadGroups directly, others used special classes > like sun.awt.AppContext. > > Such sandboxing became useless since the plugin and webstart are no longer > supported and were removed in jdk11. > > Note that this is just a first step for the overall cleanup, here is my plan: > 1. Cleanup the usage of AppContext in the “java.util.logging.LogManager" > class in the java.base module. > 2. Cleanup the usage of TheadGroup in the JavaSound > 3. Cleanup the usage of TheadGroup in the JavaBeans > 4. Cleanup the usage of AppContext in the Swing > 5. Cleanup the usage of AppContext in the AWT > 6. Delete the AppContext class. > > The current PR is for the first step. So this is the request to delete the > usage of AppContext in the LogManager and related tests. > > Tested by tier1/tier2/tier3 and jdk_desktop tests. Does anybody have any other suggestions? - PR: https://git.openjdk.java.net/jdk/pull/5326
Re: RFR: 8275293: A change done with JDK-8268764 mismatches the java.rmi.server.ObjID.hashCode spec
On Fri, 15 Oct 2021 07:17:52 GMT, Сергей Цыпанов wrote: > It looks like we cannot use `Long.hashCode(long)` for > `java.rmi.server.ObjID.hashCode()` due to specification: > https://docs.oracle.com/en/java/javase/17/docs/api/java.rmi/java/rmi/server/ObjID.html#hashCode(). I think this one was missed due to the absence of the coverage in the jtreg test suite, and some people have no access to the jck to run it in advance. - PR: https://git.openjdk.java.net/jdk/pull/5963
Re: RFR: 8275293: A change done with JDK-8268764 mismatches the java.rmi.server.ObjID.hashCode spec
On Fri, 15 Oct 2021 07:17:52 GMT, Сергей Цыпанов wrote: > It looks like we cannot use `Long.hashCode(long)` for > `java.rmi.server.ObjID.hashCode()` due to specification: > https://docs.oracle.com/en/java/javase/17/docs/api/java.rmi/java/rmi/server/ObjID.html#hashCode(). Just a suggestion, it is always a good thing to add a jtreg test case, especially if it is regression. - PR: https://git.openjdk.java.net/jdk/pull/5963
Re: RFR: 8270490: Charset.forName() taking fallback default value [v4]
On Sat, 23 Oct 2021 22:13:35 GMT, Naoto Sato wrote: >> During the review of JEP 400, a proposal to provide an overloaded method to >> `Charset.forName()` was suggested >> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This >> PR is to implement the proposal. A CSR is also drafted as >> https://bugs.openjdk.java.net/browse/JDK-8275348 > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflecting review comments Thank you for clarification. - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8275170: Some jtreg sound tests should be marked headful
On Sun, 24 Oct 2021 16:45:15 GMT, Phil Race wrote: > This makes the tests eligible to be run on headful systems. In practice it > seems like ONLY headful systems actually have the sound devices. The sound > keyword isn't understood by anything yet, so adding headful is the only way > we have right now of ensuring these tests are eligible to be run on systems > that have the necessary support. This is not the right approach, it was made a hard work to eliminate the headful keyword from any tests including sound which are not throw the headless exception or something. The "headful" means that the test will fail otherwise due to UI session missing. We should not abuse it to select some other unrelated tests. Like we do not doing this for printer. The problem you try to solve is that you have a "bad" system where nothing is configured and the "good" system where UI/sound(what about printer?) are configured. And you want to exclude all tests from the "bad" host and run them on the "good" one by using the headful keyword which is not targeted for this usecase. > As I explained in the initial PR description headful is also useful for > implying exclusive access. Such exclusive access should not be needed by the sound, in fact it was found a few hard too spot concurrency bugs when we start to run the sound tests in parallel. > And without this keyword it means that these tests are ALWAYS run on systems > without sound device support. Is it always just because the sound is not configured on the systems where you run the tests without headful keyword? How it is related to the "headfulness" of the systems? > So for years we've effectively problem listed these tests due to the tests > not having headful and silently passing when there's no sound. Without > headful no one runs these on systems that have the needed device. This was done intentionally that everybody who run the tests also run the tests for the sound, moreover it was included the openjdk tier3, before it was accidentally removed from there. > The 2 keywords give flexibility - anyone who wants to filter when its marked > headful and needing sound can do this, but just adding sound right now will > solve nothing. Such flexibility exists already, it is possible to run test/jdk/javax/sound on any computer you want and exclude it from the jdk_desktop test run. > And there are a couple of tests NOT in OpenJDK that already do this so we > have precedent - and in a previous life one was added by you and the other > approved by you .. If it is not in the OpenJDK then we do not need to discuss it here and that's not actually a precedent. We should investigate each case one by one, the headful keyword does not solve any issue on windows, since all windows are headful(or most of them where we interested in the audio), on macOS the sound subsystem is built-in. And on Linux it is possible to configure sound as well and it will work fine w/o X server. I think most of the sound tests are written according to the spec which does not required UI, and if it was necessary to make it pass by running on the system which have UI system configured make me think that some product bug was workaround. - PR: https://git.openjdk.java.net/jdk/pull/6086
Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]
On Fri, 22 Oct 2021 17:33:43 GMT, Naoto Sato wrote:
>> During the review of JEP 400, a proposal to provide an overloaded method to
>> `Charset.forName()` was suggested
>> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This
>> PR is to implement the proposal. A CSR is also drafted as
>> https://bugs.openjdk.java.net/browse/JDK-8275348
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Removed `@throws IllegalCharsetNameException`
Just an idea, should we check that the second parameter is null or not? Any
pros and cons of that?
For example should this code be allowed:
var cs = Charset.forName(charsetName, null);
if (cs == null) {
System.err.println("Used UTF-8 encoding instead of "+charsetName+");
cs = StandardCharsets.UTF_8;
}
Or something like this should be forced:
var cs = Charset.forName(charsetName, fallback);
if (cs == fallback) {
System.err.println("Used UTF-8 encoding instead of "+charsetName+");
}
I have no preference.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]
On Fri, 22 Oct 2021 17:33:43 GMT, Naoto Sato wrote:
>> During the review of JEP 400, a proposal to provide an overloaded method to
>> `Charset.forName()` was suggested
>> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This
>> PR is to implement the proposal. A CSR is also drafted as
>> https://bugs.openjdk.java.net/browse/JDK-8275348
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Removed `@throws IllegalCharsetNameException`
src/java.base/share/classes/java/io/Console.java line 589:
> 587: }
> 588: if (cs == null) {
> 589: cs = Charset.forName(StaticProperty.nativeEncoding(),
> Charset.defaultCharset());
Not sure but looks like this class tries to maintain 80 chars per line rule?
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8275170: Some jtreg sound tests should be marked headful
On Fri, 22 Oct 2021 19:01:27 GMT, Phil Race wrote: > This fix adds "headful sound" keywords to a number of javax/sound jtreg tests > > The jtreg javax.sound tests are written in such a way that if a needed audio > device > or other platform resource is not available, they just exit with a "pass" for > the test. > It is as if headful tests just swallowed HeadlessException and issued a pass. > If we allowed that we'd be effectively testing almost nothing of real > importance. > > Currently almost all sound tests have no keyword like "headful" to indicate > they > may need access to a hardware resource to do anything useful so a similar > situation arises there except when someone runs those tests manually on > a local system which has audio devices. > > Of course "headful" and "audio device" aren't exactly interchangeable terms > but if tests are allowed to be run - or worse usually or always run - in a > situation > where they potentially are on a system without an audio device (a headless > VM) or are running in > a session which doesn't have full desktop access - which may in some > cases be how audio device access is granted, then they are more likely > to be running in this scenario where the testing isn't valid. > > Another point is that headful tests must be run one at a time - no > concurrency because > you can't have multiple tests moving the mouse at the same time > Whereas for headless tests, a test harness may choose to run concurrently - > perhaps even in the same VM > When tests that really need access to an audio device are run it is probably > safer to consider > the headful attribute as implying one test at a time is best .. granted on a > desktop it isn't > usual for a single app to be able to monopolize access to a speaker, but for > input devices > that is what you'd generally expect. > > By no means am I sure that the tests being updated here are the full set that > need tagging. > There are a lot of tests and they are all known to pass on systems with NO > audio > devices, so the search has been for tests which call APIs which will > definitely > need access to some device in order to do anything useful. > So likely there are more to be added - either by a reviewer pointing them out > or by later discovery. > Alternatively we could mark ALL sound tests headful .. but given where we are > starting from > that might be erring unnecessarily far in the opposite direction. > > By adding both headful and sound keywords it gives options to someone who > wants to identify the tests and perhaps run just tests which need "sound" on > some > config they know supports audio but isn't headful. I am not sure this is the right thing to do, as you pointed out the headless system may or may not have the sound configured, similar to the headful system may not have a sound. I see a lot of headful systems in the cloud where the audio device is not configured, and the opposite where the headless system actually has some audio devices. And this change just makes things complicated by assignee the headful keyword to the unrelated sound system. Actually, it is the step in the opposite direction that was done when these tests were moved to tier3, at that the headful keyword was removed from these tests to make tier3 coverage as much as possible. I am still not sure what problem do you want to solve? If the problem is to run the tests on the system where the sound is configured then just run them there, so it will not be necessary to validate each test does it really need a sound/headful keyword or not, otherwise what is the benefit? - PR: https://git.openjdk.java.net/jdk/pull/6086
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 16:03:29 GMT, Naoto Sato wrote:
>> Apparently `IllegalCharsetNameException` or `IllegalArgumentException` could
>> still be thrown - so removing the `try-catch` would be a change of behaviour
>> in those cases. It all depends on whether there is a chance that these
>> exceptions could be thrown in this particular context (with these particular
>> input parameters) - which I am not able to tell - but maybe someone more
>> familiar with this code could...
>
> I first thought of swallowing all exceptions in 2-arg forName(), but decided
> not to do that. Because `IllegalArgumentException` and
> `IllegalCharsetNameException` are for the validity of the passed
> `charsetName`, like detecting `null` or invalid chars like "". On the other
> hand, `UnsupportedCharsetException` is for the availability which varies
> depending on the user's settings and or platform, which can be safely
> replaced with `fallback` charset. So yes, it is not totally getting rid of
> `try-catch` but it avoids `UnsupportedCharsetException` which is only
> detectable at runtime.
Then what is the benefit, if the user will have to write such code anyway?:
try {
cs = Charset.forName(StaticProperty.nativeEncoding(), fallback);
} catch (Exception ignored) {
cs = fallback;
}
Even in the current code update it can work well w/o the second parameter.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 19:02:30 GMT, Naoto Sato wrote:
>> During the review of JEP 400, a proposal to provide an overloaded method to
>> `Charset.forName()` was suggested
>> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This
>> PR is to implement the proposal. A CSR is also drafted as
>> https://bugs.openjdk.java.net/browse/JDK-8275348
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Moved the null sentence into @param tag.
src/java.base/share/classes/java/io/Console.java line 587:
> 585: try {
> 586: cs = Charset.forName(csname, null);
> 587: } catch (Exception ignored) { }
The comment which suggests this enhancement was about eliminating such
"exception ignored" code paths. Is it still needed here? And if it is needed
why do we pass the null as a fallback?
src/java.base/share/classes/java/io/Console.java line 594:
> 592: cs = Charset.forName(StaticProperty.nativeEncoding(),
> Charset.defaultCharset());
> 593: } catch (Exception ignored) {
> 594: cs = Charset.defaultCharset();
What kind of actual improvements do we get here since the catch block is still
in place?
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8268764: Use Long.hashCode() instead of int-cast where applicable [v4]
On Thu, 1 Jul 2021 12:19:53 GMT, Сергей Цыпанов wrote:
>> In some JDK classes there's still the following hashCode() implementation:
>>
>> long objNum;
>>
>> public int hashCode() {
>> return (int) objNum;
>> }
>>
>> This outdated expression should be replaced with Long.hashCode(long) as it
>>
>> - uses all bits of the original value, does not discard any information
>> upfront. For example, depending on how you are generating the IDs, the upper
>> bits could change more frequently (or the opposite).
>>
>> - does not introduce any bias towards values with more ones (zeros), as it
>> would be the case if the two halves were combined with an OR (AND) operation.
>>
>> See https://stackoverflow.com/a/4045083
>>
>> This is related to https://github.com/openjdk/jdk/pull/4309
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8268764: Update copy-right year
Marked as reviewed by serb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4491
Re: RFR: 8177814: jdk/editpad is not in jdk TEST.groups
On Thu, 23 Sep 2021 08:54:48 GMT, Aleksey Shipilev wrote: > @prrace notices this here: > https://github.com/openjdk/jdk/pull/5544#issuecomment-925396869. And I think > it is the already open issue that this patch is fixing. While the original > patch added the test in `jdk_other`, Phil suggests it to be added to > `jdk_desktop`. > > Additional testing: > - [x] `jdk_editpad` is passing Let's fix it this way - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5648
Re: RFR: 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE
On Sat, 9 Oct 2021 17:54:16 GMT, Andrey Turbanov wrote: > 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 240: > 238: * OutOfMemoryError: Requested array size exceeds VM limit > 239: */ > 240: private static final int MAX_ARRAY_SIZE = Integer.MAX_VALUE - 8; Looks like the usage of this field was removed by the https://github.com/openjdk/lanai/commit/03642a01, note that the doc for the "newCapacity" is still mentioned this field. - PR: https://git.openjdk.java.net/jdk/pull/5878
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v5]
On Tue, 5 Oct 2021 00:25:41 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code Looks fine - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v4]
On Fri, 1 Oct 2021 21:10:27 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging
On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov wrote: > At the time Java supported applets and webstart, a special mechanism for > launching various applications in one JVM was used to reduce memory usage and > each application was isolated from each other. > > This isolation was implemented via ThreadGroups where each application > created its own thread group and all data for the application was stored in > the context of its own group. > > Some parts of the JDK used ThreadGroups directly, others used special classes > like sun.awt.AppContext. > > Such sandboxing became useless since the plugin and webstart are no longer > supported and were removed in jdk11. > > Note that this is just a first step for the overall cleanup, here is my plan: > 1. Cleanup the usage of AppContext in the “java.util.logging.LogManager" > class in the java.base module. > 2. Cleanup the usage of TheadGroup in the JavaSound > 3. Cleanup the usage of TheadGroup in the JavaBeans > 4. Cleanup the usage of AppContext in the Swing > 5. Cleanup the usage of AppContext in the AWT > 6. Delete the AppContext class. > > The current PR is for the first step. So this is the request to delete the > usage of AppContext in the LogManager and related tests. > > Tested by tier1/tier2/tier3 and jdk_desktop tests. Does anybody have any other suggestions? - PR: https://git.openjdk.java.net/jdk/pull/5326
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]
On Mon, 27 Sep 2021 23:50:38 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher > code src/java.base/macosx/native/libjli/java_md_macosx.m line 879: > 877: } > 878: > 879: (*env)->DeleteLocalRef(env, jKey); I am not sure about error handling, the jkey is not removed when NULL_CHECK is used. Also an exceptions are not cleared in case of NULL_CHECK like in other cases, is it intentional? - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8177814: jdk/editpad is not in jdk TEST.groups
On Thu, 23 Sep 2021 08:54:48 GMT, Aleksey Shipilev wrote: > @prrace notices this here: > https://github.com/openjdk/jdk/pull/5544#issuecomment-925396869. And I think > it is the already open issue that this patch is fixing. While the original > patch added the test in `jdk_other`, Phil suggests it to be added to > `jdk_desktop`. > > Additional testing: > - [x] `jdk_editpad` is passing There is also one such test in the test/hotspot. @prrace Not sure what is the best fix for this. Probably the test automation should run the headful tests from t1/2/3/etc instead of just jdk_desktop? - PR: https://git.openjdk.java.net/jdk/pull/5648
Re: RFR: 8273314: Add tier4 test groups [v4]
On Fri, 17 Sep 2021 06:59:09 GMT, Aleksey Shipilev wrote:
>> During the review of JDK-8272914 that added hotspot:tier{2,3} groups,
>> @iignatev suggested to create tier4 groups that capture all tests not in
>> tiers{1,2,3}.
>>
>> Caveats:
>> - I excluded `applications` from `hotspot:tier4`, because they require test
>> dependencies (e.g. jcstress).
>> - `jdk:tier4` only runs well with `JTREG_KEYWORDS=!headful` or reduced
>> concurrency with `TEST_JOBS=1`, because headful tests cannot run in parallel
>>
>> Sample run with `JTREG_KEYWORDS=!headful`:
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:tier4 3585 3584 0 1
<<
jtreg:test/jdk:tier4 2893 2887 5 1
<<
>>jtreg:test/langtools:tier40 0 0 0
>>
>>jtreg:test/jaxp:tier4 0 0 0 0
>>
>> ==
>>
>> real 699m39.462s
>> user 6626m8.448s
>> sys 1110m43.704s
>>
>>
>> There are interesting test failures on my machine, which I would address
>> separately.
>
> Aleksey Shipilev 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 five additional
> commits since the last revision:
>
> - Merge branch 'master' into JDK-8273314-tier4
> - Merge branch 'master' into JDK-8273314-tier4
> - Drop applications and fix the comment
> - Drop exceptions
> - Add tier4 test groups
It is fine to run headful and headless tests separately.
-
Marked as reviewed by serb (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5357
Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging
On Mon, 6 Sep 2021 09:39:58 GMT, Alan Bateman wrote: > No objection to removing this legacy/unused code but I think it would be > useful to useful to have the JBS issue or the PR summary provide a bit more > context. As I see it, this is just one piece of the overall cleanup and I > assume there are more substantive changes to the java.desktop module coming, > is that right? I have updated the PR and JBS - PR: https://git.openjdk.java.net/jdk/pull/5326
Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging
On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov wrote: > The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" > via an AppContext to support "applet logging isolation". The AppContext class > became useless since the plugin and webstart are no longer supported and > removed in jdk11. > > This is the request to delete the usage of AppContext in the LogManager and > related tests. > > Tested by tier1/tier2/tier3 and jdk_desktop tests. I'll remove its used one by one from the external usage like in "java.base" here, then in Swing, then in 2D like fonts, then last in AWT. That change should not break something other than the tests which intentionally create different appcontexts. And it's easy to review. - PR: https://git.openjdk.java.net/jdk/pull/5326
Re: [jdk17] RFR: JDK-8272639: jpackaged applications using microphone on mac
On Thu, 9 Sep 2021 20:14:01 GMT, Andy Herrick wrote: > backport from jdk-18 Is it for jdk17 or for jdk17u? - PR: https://git.openjdk.java.net/jdk17/pull/306
Re: RFR: 8273314: Add tier4 test groups
On Sat, 4 Sep 2021 02:51:50 GMT, Sergey Bylokhov wrote:
>> During the review of JDK-8272914 that added hotspot:tier{2,3} groups,
>> @iignatev suggested to create tier4 groups that capture all tests not in
>> tiers{1,2,3}. I have excluded `vmTestbase` and `hotspot:tier4,` because they
>> take 10+ hours on my highly parallel machine. I have also excluded
>> `applications` from `hotspot:tier4`, because they require test dependencies
>> (e.g. jcstress).
>>
>> Sample run:
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>>> jtreg:test/hotspot/jtreg:tier4 426 425 1 0
>>>> <<
>>>> jtreg:test/jdk:tier4 2891 2885 4 2
>>>> <<
>>jtreg:test/langtools:tier40 0 0 0
>>
>>jtreg:test/jaxp:tier4 0 0 0 0
>>
>> ==
>>
>> real 64m13.994s
>> user 1462m1.213s
>> sys 39m38.032s
>>
>>
>> There are interesting test failures on my machine, which I would address
>> separately.
>
> it looks like the results above do not include the headful tests did you
> filter them out?
>>> jtreg:test/jdk:tier4 2891 2885 4 2 <<
> @mrserb: Yes, I ran this on a headless config, so headful tests were skipped,
> apparently. I'll try to arrange runs on my desktop.
Then you probably need to skip "printer" tests as well. BTW it will be really
good somehow to execute headless tests in tier4 concurrently, and run the
headful tests in tier5 sequentially.
-
PR: https://git.openjdk.java.net/jdk/pull/5357
Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging
On Mon, 6 Sep 2021 09:39:58 GMT, Alan Bateman wrote: > As I see it, this is just one piece of the overall cleanup and I assume there > are more substantive changes to the java.desktop module coming, is that right? Yes, you are right. - PR: https://git.openjdk.java.net/jdk/pull/5326
Re: RFR: 8273314: Add tier4 test groups
On Fri, 3 Sep 2021 09:10:20 GMT, Aleksey Shipilev wrote:
> During the review of JDK-8272914 that added hotspot:tier{2,3} groups,
> @iignatev suggested to create tier4 groups that capture all tests not in
> tiers{1,2,3}. I have excluded `vmTestbase` and `hotspot:tier4,` because they
> take 10+ hours on my highly parallel machine. I have also excluded
> `applications` from `hotspot:tier4`, because they require test dependencies
> (e.g. jcstress).
>
> Sample run:
>
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>>> jtreg:test/hotspot/jtreg:tier4 426 425 1 0 <<
>>> jtreg:test/jdk:tier4 2891 2885 4 2 <<
>jtreg:test/langtools:tier40 0 0 0
>
>jtreg:test/jaxp:tier4 0 0 0 0
>
> ==
>
> real 64m13.994s
> user 1462m1.213s
> sys 39m38.032s
>
>
> There are interesting test failures on my machine, which I would address
> separately.
it looks like the results above do not include the headful tests did you filter
them out?
>> jtreg:test/jdk:tier4 2891 2885 4 2 <<
-
PR: https://git.openjdk.java.net/jdk/pull/5357
Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging
On Thu, 2 Sep 2021 09:59:51 GMT, Daniel Fuchs wrote: >> The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" >> via an AppContext to support "applet logging isolation". The AppContext >> class became useless since the plugin and webstart are no longer supported >> and removed in jdk11. >> >> This is the request to delete the usage of AppContext in the LogManager and >> related tests. >> >> Tested by tier1/tier2/tier3 and jdk_desktop tests. > > test/jdk/java/util/logging/TestLoggingWithMainAppContext.java line 1: > >> 1: /* > > Humm... There's no direct reference to AppContext here. Maybe this test > should be preserved? It uses it indirectly, the next line under security manager trigger creation of the appcontext: `ImageIO.read(is); // triggers calls to system loggers & creation of main AppContext` but I can preserve/rename it for sure. - PR: https://git.openjdk.java.net/jdk/pull/5326
Re: RFR: 8273329: Remove redundant null check from String.getBytes(String charsetName)
On Fri, 3 Sep 2021 17:37:08 GMT, Sergey Bylokhov wrote:
>> Current implementation looks like this:
>>
>> public byte[] getBytes(String charsetName)
>> throws UnsupportedEncodingException {
>> if (charsetName == null) throw new NullPointerException();
>> return encode(lookupCharset(charsetName), coder(), value);
>> }
>>
>> Null check seems to be redundant here because the same check of
>> `charsetName` is done within `String.lookupCharset(String)`:
>>
>> private static Charset lookupCharset(String csn) throws
>> UnsupportedEncodingException {
>> Objects.requireNonNull(csn);
>> try {
>> return Charset.forName(csn);
>> } catch (UnsupportedCharsetException | IllegalCharsetNameException x) {
>> throw new UnsupportedEncodingException(csn);
>> }
>> }
>
> In such cases when the specific exception throwing is removed from the method
> because it can be produced by some other used method, the test might be
> useful. So if the code in the method will be changed, or the usage of other
> method will be removed the exception still be thrown. Probably such test
> exists already, then just point to it here.
> @mrserb you are right, there's such test, see
> `/test/jdk/java/lang/String/Exceptions.getBytes()`, line 384.
Thank you for confirmation, looks fine.
-
PR: https://git.openjdk.java.net/jdk/pull/5361
Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging
On Fri, 3 Sep 2021 17:19:05 GMT, Phil Race wrote: > Perhaps this isn't the change that requires the CSR but it then leaves an > inconsistent state where desktop supports AppContext still but other modules > don't ... Even java.desktop does not support it fully, since for a couple of years nobody care about appcontext when write a new code. Note that I mentioned the "threadgroup sandboxing" in the subject, which is not necessary implemented via Appcontext. The JavaBeans and JavaSound use the thread group directly, I plan to remove them as well. - PR: https://git.openjdk.java.net/jdk/pull/5326
Re: RFR: 8273329: Remove redundant null check from String.getBytes(String charsetName)
On Fri, 3 Sep 2021 13:22:54 GMT, Сергей Цыпанов
wrote:
> Current implementation looks like this:
>
> public byte[] getBytes(String charsetName)
> throws UnsupportedEncodingException {
> if (charsetName == null) throw new NullPointerException();
> return encode(lookupCharset(charsetName), coder(), value);
> }
>
> Null check seems to be redundant here because the same check of `charsetName`
> is done within `String.lookupCharset(String)`:
>
> private static Charset lookupCharset(String csn) throws
> UnsupportedEncodingException {
> Objects.requireNonNull(csn);
> try {
> return Charset.forName(csn);
> } catch (UnsupportedCharsetException | IllegalCharsetNameException x) {
> throw new UnsupportedEncodingException(csn);
> }
> }
In such cases when the specific exception throwing is removed from the method
because it can be produced by some other used method, the test might be useful.
So if the code in the method will be changed, or the usage of other method will
be removed the exception still be thrown. Probably such test exists already,
then just point to it here.
-
PR: https://git.openjdk.java.net/jdk/pull/5361
RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging
The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" via an AppContext to support "applet logging isolation". The AppContext class became useless since the plugin and webstart are no longer supported and removed in jdk11. This is the request to delete the usage of AppContext in the LogManager and related tests. Tested by tier1/tier2/tier3 and jdk_desktop tests. - Commit messages: - Some tests deleted - Update the RootLevelInConfigFile test - Initial version of JDK-8273101 Changes: https://git.openjdk.java.net/jdk/pull/5326/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5326=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273101 Stats: 1423 lines in 10 files changed: 0 ins; 1418 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5326.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5326/head:pull/5326 PR: https://git.openjdk.java.net/jdk/pull/5326
Re: RFR: 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302
On Tue, 31 Aug 2021 09:40:14 GMT, xpbob wrote: > 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302 Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5315
Re: RFR: 8271302: Regex Test Refresh [v5]
On Mon, 30 Aug 2021 15:52:05 GMT, Ian Graves wrote: >> 8271302: Regex Test Refresh > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Removing some notes re JUnit5 Looks like it was missed that the test fails oi a github actions: https://github.com/igraves/jdk/runs/3463998089 Run if ! grep --include=test-summary.txt -lqr build/*/test-results -e "TEST SUCCESS" ; then newfailures.txt java/util/regex/NegativeArraySize.java Error: Process completed with exit code 1. Invalid initial heap size: -Xms5G The specified size exceeds the maximum representable size. Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit. - PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Mon, 23 Aug 2021 21:01:48 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. The changes in the src/java.desktop/ looks fine. Filed: https://bugs.openjdk.java.net/browse/JDK-8272863 - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: JDK-8272639: jpackaged applications using microphone on mac [v2]
On Mon, 23 Aug 2021 14:34:52 GMT, Andy Herrick wrote: >> JDK-8272639: jpackaged applications using microphone on mac > > Andy Herrick has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8272639: jpackaged applications using microphone on mac Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5186
RFR: 8272805: Avoid looking up standard charsets
This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
See https://github.com/openjdk/jdk/pull/5063 and
https://github.com/openjdk/jdk/pull/4951
In many places standard charsets are looked up via their names, for example:
absolutePath.getBytes("UTF-8");
This could be done more efficiently(up to x20 time faster) with use of
java.nio.charset.StandardCharsets:
absolutePath.getBytes(StandardCharsets.UTF_8);
The later variant also makes the code cleaner, as it is known not to throw
UnsupportedEncodingException in contrary to the former variant.
This change includes:
* demo/utils
* jdk.xx packages
* Some places were missed in the previous changes. I have found it by tracing
the calls to the Charset.forName() by executing tier1,2,3 and desktop tests.
Also checked for "aliases" usage.
Some performance discussion: https://github.com/openjdk/jdk/pull/5063
Code excluded in this fix: the Xerces library(should be fixed upstream),
J2DBench(should be compatible to 1.4), some code in the "java.naming"(the
change there is not straightforward, will do it later).
Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
-
Commit messages:
- Fix related imports
- Merge branch 'master' into standard-encodings-in-non-public-modules
- Cleanup UnsupportedEncodingException
- Update PacketStream.java
- Rollback TextTests, should be compatible with jdk1.4
- Rollback TextRenderTests, should be compatible with jdk1.4
- Cleanup the UnsupportedEncodingException
- aliases for ISO_8859_1
- Update imageio
- Initial fix
Changes: https://git.openjdk.java.net/jdk/pull/5210/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5210=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8272805
Stats: 333 lines in 48 files changed: 91 ins; 123 del; 119 mod
Patch: https://git.openjdk.java.net/jdk/pull/5210.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5210/head:pull/5210
PR: https://git.openjdk.java.net/jdk/pull/5210
Re: RFR: JDK-8272639: jpackaged applications using microphone on mac
On Thu, 19 Aug 2021 14:12:00 GMT, Andy Herrick wrote: > JDK-8272639: jpackaged applications using microphone on mac src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/resources/Info-lite.plist.template line 37: > 35: true > 36: NSMicrophoneUsageDescription > 37: The application is requesting access to the microphone. Should not it include the "application name" here? This is different from the java tool, where we do not know that info in advance. - PR: https://git.openjdk.java.net/jdk/pull/5186
Integrated: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
This pull request has now been integrated.
Changeset: ec2fc384
Author:Sergey Bylokhov
URL:
https://git.openjdk.java.net/jdk/commit/ec2fc384e50668b667335f973ffeb5a19bbcfb9b
Stats: 127 lines in 15 files changed: 24 ins; 53 del; 50 mod
8272120: Avoid looking for standard encodings in "java." modules
Reviewed-by: alanb, dfuchs, naoto
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 09:18:39 GMT, Alan Bateman wrote: > It would be useful to get up to date performance data on using Charset vs. > charset name. At least historically, the charset name versions were faster > (see [JDK-6764325](https://bugs.openjdk.java.net/browse/JDK-6764325)). The code in question was changed a few times since then, the last change was done by the https://github.com/openjdk/jdk/pull/2102. So currently the code for string.getBytes String/Charset uses the same code paths, except that the string version has an additional call to lookup the charset. The string version: https://github.com/openjdk/jdk/blob/66d1faa7847b645f20ab2e966adf0a523e3ffeb2/src/java.base/share/classes/java/lang/String.java#L1753 The charset version: https://github.com/openjdk/jdk/blob/66d1faa7847b645f20ab2e966adf0a523e3ffeb2/src/java.base/share/classes/java/lang/String.java#L1777 I checked the performance and the charset is always faster, depending on the string size, up to x20. @cl4es please confirm my observation since you did it already for https://github.com/openjdk/jdk/pull/2102 - PR: https://git.openjdk.java.net/jdk/pull/5063
RFR: 8272120: Avoid looking for standard encodings in "java." modules
This is the continuation of JDK-8233884 and JDK-8271456. This change affects
fewer cases so I fix all "java." modules at once.
In many places standard charsets are looked up via their names, for example:
absolutePath.getBytes("UTF-8");
This could be done more efficiently(up to x20 time faster) with use of
java.nio.charset.StandardCharsets:
absolutePath.getBytes(StandardCharsets.UTF_8);
The later variant also makes the code cleaner, as it is known not to throw
UnsupportedEncodingException in contrary to the former variant.
tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
-
Commit messages:
- Initial fix of JDK-8272120
Changes: https://git.openjdk.java.net/jdk/pull/5063/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5063=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8272120
Stats: 127 lines in 15 files changed: 24 ins; 53 del; 50 mod
Patch: https://git.openjdk.java.net/jdk/pull/5063.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5063/head:pull/5063
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov wrote: > I found few places, where code initially perform `Object[] > Colleciton.toArray()` call and then manually copy array into another array > with required type. > This PR cleanups such places to more shorter call `T[] > Collection.toArray(T[])`. Changes in the desktop module looks fine. - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov wrote: > I found few places, where code initially perform `Object[] > Colleciton.toArray()` call and then manually copy array into another array > with required type. > This PR cleanups such places to more shorter call `T[] > Collection.toArray(T[])`. src/java.desktop/share/classes/sun/java2d/SunGraphicsEnvironment.java line 191: > 189: installed[i]); > 190: } > 191: String[] retval = map.keySet().toArray(new String[0]); Looks like previously the code returns values, and now it will return keys, please recheck. - PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]
On Wed, 17 Mar 2021 00:57:24 GMT, Henry Jen wrote: >> This patch ensure launcher won't crash JVM for the new static Methods from >> local/anonymous class on MacOS. >> >> As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug >> when the launcher trying to grab class name to be displayed as the >> Application name on the menu. >> >> The fix is to not setting name, test shows that GUI java application shows >> 'bin' as the application name. It's possible for us to set the name to >> something more friendly, for example, "Java", but I am not sure that should >> be launcher's responsibility to choose such a default name. It seems to me >> the consumer of the JAVA_MAIN_CLASS_%d environment variable should be >> responsible to pick such name in case the environment variable is not set. > > Henry Jen has updated the pull request incrementally with one additional > commit since the last revision: > > Add copyright and another test case Looks fine from the client point of view. - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v2]
On Tue, 16 Mar 2021 17:39:34 GMT, Henry Jen wrote: >> test/jdk/tools/launcher/8261785/CrashTheJVM.java line 1: >> >>> 1: import java.io.IOException; >> >> Copyright? > > This file is mostly based on the bug report as I just adjust static keyword > to make sure we cover different cases, thus I am not sure whether to add > copyright or not. you need to clarify this, if we cannot add copyright here, we should not use this file. - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Tue, 16 Mar 2021 01:55:49 GMT, Sergey Bylokhov wrote: >> This patch ensure launcher won't crash JVM for the new static Methods from >> local/anonymous class on MacOS. >> >> As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug >> when the launcher trying to grab class name to be displayed as the >> Application name on the menu. >> >> The fix is to not setting name, test shows that GUI java application shows >> 'bin' as the application name. It's possible for us to set the name to >> something more friendly, for example, "Java", but I am not sure that should >> be launcher's responsibility to choose such a default name. It seems to me >> the consumer of the JAVA_MAIN_CLASS_%d environment variable should be >> responsible to pick such name in case the environment variable is not set. > > This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and > the fix looks fine. > Maybe the AWT folk should decide what name should be displayed in this > case. The canonical name was chosen when all main classes had to have a > canonical name. So perhaps a simple name will suffice in the case where > there is no canonical name? This is not the last attempt to set the name, the JAVA_MAIN_CLASS_ variable is used in the middle of the name selection, there are some others. And the "bin" is selected by some of the next step, I agree it is not a friendly name that could be improved. - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen wrote: > This patch ensure launcher won't crash JVM for the new static Methods from > local/anonymous class on MacOS. > > As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug > when the launcher trying to grab class name to be displayed as the > Application name on the menu. > > The fix is to not setting name, test shows that GUI java application shows > 'bin' as the application name. It's possible for us to set the name to > something more friendly, for example, "Java", but I am not sure that should > be launcher's responsibility to choose such a default name. It seems to me > the consumer of the JAVA_MAIN_CLASS_%d environment variable should be > responsible to pick such name in case the environment variable is not set. test/jdk/tools/launcher/8261785/CrashTheJVM.java line 1: > 1: import java.io.IOException; Copyright? test/jdk/tools/launcher/8261785/Test8261785.java line 5: > 3: * COPYRIGHT NOTICES OR THIS FILE HEADER. > 4: * > 5: * This code is free software; you can redistribute it and/or modify it > under the terms of the GNU Looks like formatting much wider than usual - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen wrote: > This patch ensure launcher won't crash JVM for the new static Methods from > local/anonymous class on MacOS. > > As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug > when the launcher trying to grab class name to be displayed as the > Application name on the menu. > > The fix is to not setting name, test shows that GUI java application shows > 'bin' as the application name. It's possible for us to set the name to > something more friendly, for example, "Java", but I am not sure that should > be launcher's responsibility to choose such a default name. It seems to me > the consumer of the JAVA_MAIN_CLASS_%d environment variable should be > responsible to pick such name in case the environment variable is not set. This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and the fix looks fine. - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v6]
On Mon, 15 Mar 2021 18:04:26 GMT, Сергей Цыпанов wrote: >> In some cases wrapping of `InputStream` with `BufferedInputStream` is >> redundant, e.g. in case the wrapped one is `ByteArrayOutputStream` which >> does not require any buffer having one within. >> >> Other cases are related to reading either a byte or short `byte[]`: in both >> cases `BufferedInputStream.fill()` will be called resulting in load of much >> bigger amount of data (8192 by default) than required. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > Revert HttpClient Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v3]
On Sun, 14 Mar 2021 19:35:25 GMT, Сергей Цыпанов
wrote:
>> In some cases wrapping of `InputStream` with `BufferedInputStream` is
>> redundant, e.g. in case the wrapped one is `ByteArrayOutputStream` which
>> does not require any buffer having one within.
>>
>> Other cases are related to reading either a byte or short `byte[]`: in both
>> cases `BufferedInputStream.fill()` will be called resulting in load of much
>> bigger amount of data (8192 by default) than required.
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use InputStream.readNBytes() and fix JLinkNegativeTest
src/java.desktop/share/classes/sun/awt/image/ByteArrayImageSource.java line 54:
> 52:
> 53: protected ImageDecoder getDecoder() {
> 54: InputStream is = new ByteArrayInputStream(imagedata, imageoffset,
> imagelength);
This file use 80 chars per line code style.
-
PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Fri, 5 Mar 2021 18:53:29 GMT, Claes Redestad wrote: >> This patch refactors Locale.getDefault(Category) so that the volatile field >> holding the Locale is typically only read once. This has a small performance >> advantage, and might be more robust if initialization is racy. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Fix omitted synchronized Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Sat, 6 Mar 2021 13:34:14 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/util/Locale.java line 946:
>>
>>> 944: Locale loc = defaultDisplayLocale; // volatile read
>>> 945: if (loc == null) {
>>> 946: loc = getDisplayLocale();
>>
>> Just interesting how did you check that the performance difference is
>> because of volatile read, and not because of replacing of the switch by the
>> "if"?
>
> I started out with this variant, only removing the double volatile reads:
>
> public static Locale getDefault(Locale.Category category) {
> // do not synchronize this method - see 4071298
> Locale loc;
> switch (category) {
> case DISPLAY:
> loc = defaultDisplayLocale;
> if (loc == null) {
> synchronized(Locale.class) {
> loc = defaultDisplayLocale;
> if (loc == null) {
> loc = defaultDisplayLocale = initDefault(category);
> }
> }
> }
> return loc;
> case FORMAT:
> loc = defaultFormatLocale;
> if (loc == null) {
> synchronized(Locale.class) {
> loc = defaultFormatLocale;
> if (loc == null) {
> loc = defaultFormatLocale = initDefault(category);
> }
> }
> }
> return loc;
> default:
> assert false: "Unknown Category";
> }
> return getDefault();
> }
>
> Scores were the same:
> Benchmark Mode Cnt Score Error Units
> LocaleDefaults.getDefault avgt5 10.045 ± 0.032 ns/op
> LocaleDefaults.getDefaultDisplay avgt5 11.301 ± 0.053 ns/op
> LocaleDefaults.getDefaultFormat avgt5 11.303 ± 0.054 ns/op
>
> I then refactored and checked that the refactorings were performance neutral.
And it is faster than the final version?
-
PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Fri, 5 Mar 2021 18:53:29 GMT, Claes Redestad wrote:
>> This patch refactors Locale.getDefault(Category) so that the volatile field
>> holding the Locale is typically only read once. This has a small performance
>> advantage, and might be more robust if initialization is racy.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix omitted synchronized
src/java.base/share/classes/java/util/Locale.java line 946:
> 944: Locale loc = defaultDisplayLocale; // volatile read
> 945: if (loc == null) {
> 946: loc = getDisplayLocale();
Just interesting how did you check that the performance difference is because
of volatile read, and not because of replacing of the switch by the "if"?
-
PR: https://git.openjdk.java.net/jdk/pull/2845
Integrated: 8261010: Delete the Netbeans "default" license header
On Wed, 3 Feb 2021 04:01:51 GMT, Sergey Bylokhov wrote: > Trivial cleanup, the "default" license header is removed in a few components. This pull request has now been integrated. Changeset: f279ff9d Author:Sergey Bylokhov URL: https://git.openjdk.java.net/jdk/commit/f279ff9d Stats: 14 lines in 3 files changed: 0 ins; 14 del; 0 mod 8261010: Delete the Netbeans "default" license header Reviewed-by: iris, psadhukhan - PR: https://git.openjdk.java.net/jdk/pull/2368
RFR: 8261010: Delete the Netbeans "default" license header
Trivial cleanup, the "default" license header is removed in a few components. - Commit messages: - Initial fix Changes: https://git.openjdk.java.net/jdk/pull/2368/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2368=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261010 Stats: 14 lines in 3 files changed: 0 ins; 14 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2368.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2368/head:pull/2368 PR: https://git.openjdk.java.net/jdk/pull/2368
Re: RFR: 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo
On Sun, 20 Dec 2020 20:33:43 GMT, Phil Race wrote: >>> One or two of the sources changes should probably uses Files.copy, e.g. >>> ZipPath, sjavac/CopyFile.java. >> >> Good idea! Replaced in few places. But not in ZipPath: it's actually >> implementation of underlying call from Files.copy: >> `jdk.nio.zipfs.ZipFileSystemProvider#copy`. So, `Files.copy` call will be >> recursive. > > So these changes are all over the place which means no one person knows how > to test all of it. > Have you run the sound, swing tests, and the printing tests on unix and > windows ? > For printing for sure you'll need to do some manual testing. I already suggested this, but anyway please always extract the changes to the java.desktop module to the separate PR. - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 15:08:27 GMT, Alan Bateman wrote: >> This change terminally deprecates the following methods defined by >> java.lang.ThreadGroup >> >> - stop >> - destroy >> - isDestroyed >> - setDaemon >> - isDaemon >> >> The stop method has been deprecated since=1.2 because it is inherently >> unsafe. It is time to terminally deprecate this method so it can be removed >> in a future release. Thread.stop will be examined in a separate issue. >> >> The destroy, isDestroyed, setDaemon, isDaemon methods support the mechanism >> to explicitly or automatically destroy a thread group. As detailed in >> JDK-8252885, the mechanism to destroy thread groups is flawed and racy. >> Furthermore, this mechanism inhibits efforts to drop the reference from a >> thread group to its threads (so that thread creation, starting and >> termination do not need to coordinate with their thread group). These >> methods should be terminally deprecated so they can be degraded in a future >> release and eventually removed. >> >> CSR with more information: https://bugs.openjdk.java.net/browse/JDK-8256644 > > Alan Bateman 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: > > - Fixed typo in @deprecated text > - Merge > - Update jshell class > - 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, > setDaemon and isDaemon Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,…
On Thu, 19 Nov 2020 18:51:50 GMT, Sergey Bylokhov wrote: >> This change terminally deprecates the following methods defined by >> java.lang.ThreadGroup >> >> - stop >> - destroy >> - isDestroyed >> - setDaemon >> - isDaemon >> >> The stop method has been deprecated since=1.2 because it is inherently >> unsafe. It is time to terminally deprecate this method so it can be removed >> in a future release. Thread.stop will be examined in a separate issue. >> >> The destroy, isDestroyed, setDaemon, isDaemon methods support the mechanism >> to explicitly or automatically destroy a thread group. As detailed in >> JDK-8252885, the mechanism to destroy thread groups is flawed and racy. >> Furthermore, this mechanism inhibits efforts to drop the reference from a >> thread group to its threads (so that thread creation, starting and >> termination do not need to coordinate with their thread group). These >> methods should be terminally deprecated so they can be degraded in a future >> release and eventually removed. >> >> CSR with more information: https://bugs.openjdk.java.net/browse/JDK-8256644 > > Marked as reviewed by serb (Reviewer). The changes in the awt are trivial and look fine. - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,…
On Thu, 19 Nov 2020 14:24:18 GMT, Alan Bateman wrote: > This change terminally deprecates the following methods defined by > java.lang.ThreadGroup > > - stop > - destroy > - isDestroyed > - setDaemon > - isDaemon > > The stop method has been deprecated since=1.2 because it is inherently > unsafe. It is time to terminally deprecate this method so it can be removed > in a future release. Thread.stop will be examined in a separate issue. > > The destroy, isDestroyed, setDaemon, isDaemon methods support the mechanism > to explicitly or automatically destroy a thread group. As detailed in > JDK-8252885, the mechanism to destroy thread groups is flawed and racy. > Furthermore, this mechanism inhibits efforts to drop the reference from a > thread group to its threads (so that thread creation, starting and > termination do not need to coordinate with their thread group). These methods > should be terminally deprecated so they can be degraded in a future release > and eventually removed. > > CSR with more information: https://bugs.openjdk.java.net/browse/JDK-8256644 Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8246739: InputStream.skipNBytes could be implemented more efficiently
On Thu, 19 Nov 2020 19:29:43 GMT, Brian Burkhalter wrote:
> Please review this modification of `java.io.InputStream.skipNBytes(long)` to
> improve its performance when `skip(long)` skips fewer than the requested
> number of bytes. In the current implementation, `skip(long)` is invoked once
> and, if not enough bytes have been skipped, then `read()` is invoked for each
> of the remaining bytes to be skipped. The proposed implementation instead
> repeatedly invokes `skip(long)` until the requested number of bytes has been
> skipped, or an error condition is encountered. For cases where `skip(long)`
> skips fewer bytes than the number requested, the new version was measured to
> be up to more than one thousand times faster than the old version. When
> `skip(long)` actually skips the requested number of bytes, the performance
> difference is insignificant.
src/java.base/share/classes/java/io/InputStream.java line 596:
> 594: * @seejava.io.InputStream#skip(long)
> 595: */
> 596: public void skipNBytes(long n) throws IOException {
Not related to this change, but looks like `@since` is missing.
-
PR: https://git.openjdk.java.net/jdk/pull/1329
Re: RFR: JDK-8189198: Add "forRemoval = true" to Applet APIs [v3]
On Fri, 13 Nov 2020 18:20:37 GMT, Kevin Rushforth wrote: >> Andy Herrick has updated the pull request incrementally with one additional >> commit since the last revision: >> >> JDK-8189198: Add "forRemoval = true" to Applet APIs > > src/java.desktop/share/classes/java/applet/package-info.java line 40: > >> 38: * >> 39: * Deprecated. >> 40: * This package has been deprecated and may be removed in a future >> version of the Java Platform. > > That should be `@deprecated This package ...`. See > [java/rmi/activation/package-info.java#L41](https://github.com/openjdk/jdk/blob/master/src/java.rmi/share/classes/java/rmi/activation/package-info.java#L41). The deprecation description should point to the new API which might be used instead of the deprecated ones. So the text "deprecated without replacement" was intentionally added, it will be good to preserve it. - PR: https://git.openjdk.java.net/jdk/pull/1127
Re: RFR: 8255989: Remove explicitly unascribed authorship from Java source files
On Fri, 6 Nov 2020 20:11:24 GMT, Pavel Rappo wrote: > This PR proposes to remove > 1. JavaDoc `@author` tags with unclear semantics: `@author > unascribed|unattributed|unknown` > 2. A couple of astray Form Feed (a.k.a. FF, `\f`, `0xC`, or `^L`) characters Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1100
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v20]
On Mon, 2 Nov 2020 11:59:09 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation (see JEP 393 [1]). This >> iteration focus on improving the usability of the API in 3 main ways: >> >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee that the memory will be >> deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class has been added, which >> defines several useful dereference routines; these are really just thin >> wrappers around memory access var handles, but they make the barrier of >> entry for using this API somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not the same as it used to be; it >> used to be the case that a memory address could (sometimes, not always) have >> a back link to the memory segment which originated it; additionally, memory >> access var handles used `MemoryAddress` as a basic unit of dereference. >> >> This has all changed as per this API refresh; now a `MemoryAddress` is just >> a dumb carrier which wraps a pair of object/long addressing coordinates; >> `MemorySegment` has become the star of the show, as far as dereferencing >> memory is concerned. You cannot dereference memory if you don't have a >> segment. This improves usability in a number of ways - first, it is a lot >> easier to wrap native addresses (`long`, essentially) into a >> `MemoryAddress`; secondly, it is crystal clear what a client has to do in >> order to dereference memory: if a client has a segment, it can use that; >> otherwise, if the client only has an address, it will have to create a >> segment *unsafely* (this can be done by calling >> `MemoryAddress::asSegmentRestricted`). >> >> A list of the API, implementation and test changes is provided below. If >> you have any questions, or need more detailed explanations, I (and the rest >> of the Panama team) will be happy to point at existing discussions, and/or >> to provide the feedback required. >> >> A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without >> whom the work on shared memory segment would not have been possible; also >> I'd like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. >> >> Thanks >> Maurizio >> >> Javadoc: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a carrier is one of the usual >> suspect (a Java primitive, minus `boolean`); the access mode can be simple >> (e.g. access base address of given segment), or indexed, in which case the >> accessor takes a segment and either a low-level byte offset,or a high level >> logical index. The classification is reflected in the naming scheme (e.g. >> `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which it is easy to derive all >> the other handles using plain var handle combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both `MemoryAddress` and >>
Re: RFR: 8255299: Drop explicit zeroing at instantiation of Atomic* objects [v2]
On Wed, 28 Oct 2020 08:40:02 GMT, Сергей Цыпанов wrote: >> client changes are fine > > Rebased onto master to have the fix introduced in > https://github.com/openjdk/jdk/pull/778 FYI it is better to use merge, instead of rebase+force push. Rebase breaks history and all existed code comments. - PR: https://git.openjdk.java.net/jdk/pull/818
Re: RFR: 8255299: Drop explicit zeroing at instantiation of Atomic* objects
On Thu, 22 Oct 2020 20:46:15 GMT, Сергей Цыпанов
wrote:
> As discussed in https://github.com/openjdk/jdk/pull/510 there is never a
> reason to explicitly instantiate any instance of `Atomic*` class with its
> default value, i.e. `new AtomicInteger(0)` could be replaced with `new
> AtomicInteger()` which is faster:
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @BenchmarkMode(value = Mode.AverageTime)
> public class AtomicBenchmark {
> @Benchmark
> public Object defaultValue() {
> return new AtomicInteger();
> }
> @Benchmark
> public Object explicitValue() {
> return new AtomicInteger(0);
> }
> }
> THis benchmark demonstrates that `explicitValue()` is much slower:
> Benchmark Mode Cnt Score Error Units
> AtomicBenchmark.defaultValue avgt 30 4.778 ± 0.403 ns/op
> AtomicBenchmark.explicitValue avgt 30 11.846 ± 0.273 ns/op
> So meanwhile https://bugs.openjdk.java.net/browse/JDK-8145948 is still in
> progress we could trivially replace explicit zeroing with default
> constructors gaining some performance benefit with no risk.
>
> I've tested the changes locally, both tier1 and tier 2 are ok.
>
> Could one create an issue for tracking this?
The changes in src/java.desktop looks fine.
-
Marked as reviewed by serb (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/818
Integrated: 8255043: Incorrectly styled copyright text
On Tue, 20 Oct 2020 08:17:27 GMT, Sergey Bylokhov wrote: > In some files, the copyright text is styled as a JavaDoc comment. > Most of the affected files are tests, only one product file is affected: > src/java.sql/share/classes/javax/sql/package-info.java > > The chenge is trivial: > - /** > + /* > * Copyright (c) This pull request has now been integrated. Changeset: 2e510e04 Author:Sergey Bylokhov URL: https://git.openjdk.java.net/jdk/commit/2e510e04 Stats: 49 lines in 49 files changed: 0 ins; 0 del; 49 mod 8255043: Incorrectly styled copyright text Reviewed-by: dholmes, trebari, jdv - PR: https://git.openjdk.java.net/jdk/pull/759
RFR: 8255043: Incorrectly styled copyright text
In some files, the copyright text is styled as a JavaDoc comment. Most of the affected files are tests, only one product file is affected: src/java.sql/share/classes/javax/sql/package-info.java The chenge is trivial: - /** + /* * Copyright (c) - Commit messages: - Initial fix Changes: https://git.openjdk.java.net/jdk/pull/759/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=759=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255043 Stats: 49 lines in 49 files changed: 0 ins; 0 del; 49 mod Patch: https://git.openjdk.java.net/jdk/pull/759.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/759/head:pull/759 PR: https://git.openjdk.java.net/jdk/pull/759
RFR: 8253606: Need to add missed constructor to the SwingEventMonitor
The javadoc for this class was added, but the constructor itsef is missed.. - Commit messages: - Update SwingEventMonitor.java Changes: https://git.openjdk.java.net/jdk/pull/340/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=340=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253606 Stats: 9 lines in 1 file changed: 5 ins; 4 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/340.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/340/head:pull/340 PR: https://git.openjdk.java.net/jdk/pull/340
Re: RFR: 8252999: replace all String.equals("") usages with String.isEmpty()
On Sun, 6 Sep 2020 13:57:19 GMT, Dmitriy Dumanskiy
wrote:
> **isEmpty** is faster + has less byte code + easier to read. Benchmarks could
> be found
>
> [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416).
src/demo/share/java2d/J2DBench/src/j2dbench/tests/iio/InputImageTests.java line
187:
> 185: String format = spi.getFormatNames()[0].toLowerCase();
> 186: String suffix = spi.getFileSuffixes()[0].toLowerCase();
> 187: if (suffix == null || suffix.equals("")) {
This file intentionally maintains compatibility to jdk1.4, so equals("") should
be used.
src/demo/share/java2d/J2DBench/src/j2dbench/tests/iio/OutputImageTests.java
line 146:
> 144: String format = spi.getFormatNames()[0].toLowerCase();
> 145: String suffix = spi.getFileSuffixes()[0].toLowerCase();
> 146: if (suffix == null || suffix.equals("")) {
This file intentionally maintains compatibility to jdk1.4, so equals("") should
be used.
-
PR: https://git.openjdk.java.net/jdk/pull/29
Re: [14] Review Request: 8233827 Enable screenshots in the enhanced failure handler on Linux/macOS
I there are no objections I'll push the fix to JDK/client. On 12/11/19 7:29 pm, Igor Ignatyev wrote: Hi Sergey, overall looks good to me. a question about linux, is there an alternative to gnome-screenshot for DEs other than GNOME? -- Igor On Dec 11, 2019, at 1:00 AM, Sergey Bylokhov wrote: Hello. Please review the fix for JDK 14. Bug: https://bugs.openjdk.java.net/browse/JDK-8233827 Fix: http://cr.openjdk.java.net/~serb/8233827/webrev.01 This change adds the "screen capture on the test failure" feature on macOS and Linux. - On Linux, it is implemented by the "gnome-screenshot" command(in case of multiscreen+xineramathe whole big screen will be saved to the "screen.png" file). - On macOS it is implemented by the "screencapture" command, note that I have used 1 file per screen, if the number of screens less than 5, other files will be ignored. -- Best regards, Sergey. -- Best regards, Sergey.
Re: RFR: 8236075: Minor bootstrap improvements
Hi Claes, How did you measure this performance improvement? On 1/20/20 7:54 am, Claes Redestad wrote: Hi, some minor cleanups and enhancements in and around java.lang.ClassLoader which add up to a small startup improvement: - Remove use of Vector/Hashtable from ClassLoader, along with a few other improvements/modernizations. - Refactor ClassLoader::sys_paths/user_paths so that they're initialized lazily but also published safely Webrev: http://cr.openjdk.java.net/~redestad/8236075/open.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8236075 Testing: tier1-3 Thanks! /Claes -- Best regards, Sergey.
Re: [14] Review Request: 8233827 Enable screenshots in the enhanced failure handler on Linux/macOS
On 1/6/20 8:41 pm, Philip Race wrote: How is this useful given that we disable jtreg failure handlers for the headful tests ? It is disabled only in mach5 for headful nightly and CI builds, but it is enabled for other builds, also it is enabled if the headful tests are executed standalone via "make test", and it could easily be enabled in mach5 for personal jobs(I do this time to time) -phil. On 12/30/19, 11:33 AM, Sergey Bylokhov wrote: On 12/23/19 9:15 pm, Phil Race wrote: I am not sure what the right mailing list(s) are for this change. It definitely isn't a core-libs change. I think build-dev may be better. Previous changes to these configs were discussed here, so I have send it here as well. I am also unclear when this failure handler is invoked and how all this machinery works. It is only useful for headful tests and so I'd only want it enabled in such a case. And we disable the failure handlers in the headful test jobs anyway because they seem focused on taking pointless core dumps ...> So we need something that can be used with headful tests only and doesn't involve re-enabling the other handlers. It could be useful for other tests as well and may be able to identify problems such as: - Suggestions "to open under debugger" from the native asserts - Various error dialogs from the OS And it does not spend much resources compared to current handlers. Also why exclude Windows ? No easy way to get the screenshot ? There is no command-line program that can take a screenshot on windows by default -phil. On 12/11/19 1:00 AM, Sergey Bylokhov wrote: Hello. Please review the fix for JDK 14. Bug: https://bugs.openjdk.java.net/browse/JDK-8233827 Fix: http://cr.openjdk.java.net/~serb/8233827/webrev.01 This change adds the "screen capture on the test failure" feature on macOS and Linux. - On Linux, it is implemented by the "gnome-screenshot" command(in case of multiscreen+xinerama the whole big screen will be saved to the "screen.png" file). - On macOS it is implemented by the "screencapture" command, note that I have used 1 file per screen, if the number of screens less than 5, other files will be ignored. -- Best regards, Sergey.
Re: [15] Review Request: 8235975 Update copyright year to match last edit in jdk repository for 2014/15/16/17/18
I guess it is too late to fix it, will need to update the files at the end of 2020. On 1/2/20 10:57 am, Erik Joelsson wrote: Build files look good. /Erik On 2019-12-24 19:22, Sergey Bylokhov wrote: Hello. Here is an updated version: Bug: https://bugs.openjdk.java.net/browse/JDK-8235975 Patch (2 Mb): http://cr.openjdk.java.net/~serb/8235975/webrev.03/open.patch Fix: http://cr.openjdk.java.net/~serb/8235975/webrev.03/ - "jdk.internal.vm.compiler" is removed from the patch. - "Aes128CtsHmacSha2EType.java" is updated to "Copyright (c) 2018" On 12/22/19 11:24 pm, Sergey Bylokhov wrote: Hello. Please review the fix for JDK 15. Bug: https://bugs.openjdk.java.net/browse/JDK-8235975 Patch (2 Mb): http://cr.openjdk.java.net/~serb/8235975/webrev.02/open.patch Fix: http://cr.openjdk.java.net/~serb/8235975/webrev.02 I have updated the source code copyrights by the "update_copyright_year.sh" script for 2014/15/16/18/19 years, unfortunately, cannot run it for 2017 because of: "JDK-8187443: Forest Consolidation: Move files to unified layout" which touched all files. -- Best regards, Sergey.
Re: [14] Review Request: 8233827 Enable screenshots in the enhanced failure handler on Linux/macOS
On 12/23/19 9:15 pm, Phil Race wrote: I am not sure what the right mailing list(s) are for this change. It definitely isn't a core-libs change. I think build-dev may be better. Previous changes to these configs were discussed here, so I have send it here as well. I am also unclear when this failure handler is invoked and how all this machinery works. It is only useful for headful tests and so I'd only want it enabled in such a case. And we disable the failure handlers in the headful test jobs anyway because they seem focused on taking pointless core dumps ...> So we need something that can be used with headful tests only and doesn't involve re-enabling the other handlers. It could be useful for other tests as well and may be able to identify problems such as: - Suggestions "to open under debugger" from the native asserts - Various error dialogs from the OS And it does not spend much resources compared to current handlers. Also why exclude Windows ? No easy way to get the screenshot ? There is no command-line program that can take a screenshot on windows by default -phil. On 12/11/19 1:00 AM, Sergey Bylokhov wrote: Hello. Please review the fix for JDK 14. Bug: https://bugs.openjdk.java.net/browse/JDK-8233827 Fix: http://cr.openjdk.java.net/~serb/8233827/webrev.01 This change adds the "screen capture on the test failure" feature on macOS and Linux. - On Linux, it is implemented by the "gnome-screenshot" command(in case of multiscreen+xinerama the whole big screen will be saved to the "screen.png" file). - On macOS it is implemented by the "screencapture" command, note that I have used 1 file per screen, if the number of screens less than 5, other files will be ignored. -- Best regards, Sergey.
