Re: RFR: 8077587: BigInteger Roots [v18]
On Sun, 20 Apr 2025 05:12:19 GMT, Chen Liang wrote:
>> fabioromano1 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 21 additional
>> commits since the last revision:
>>
>> - Merge branch 'openjdk:master' into BigInteger-nth-root
>> - Format code
>> - Format code
>> - An optimization
>> - An optimization
>> - Extend use cases of MutableBigInteger.valueOf(double)
>> - BigIntegers nth root's initial estimate optimization
>> - An optimization
>> - Memory usage optimization
>> - Correct left shift if shift is zero
>> - ... and 11 more: https://git.openjdk.org/jdk/compare/d7448f7e...524f195e
>
> src/java.base/share/classes/java/math/BigInteger.java line 2669:
>
>> 2667: // Perform exponentiation using repeated squaring trick
>> 2668: for (int expLen = Integer.SIZE - expZeros; expLen > 0;
>> expLen--) {
>> 2669: answer = answer.multiply(answer);
>
> Is `answer.multiply(answer)` faster than `answer.square()`?
It should be, because `multiply(BigInteger)` checks the size of the argument in
order to call `square()` when it is more convenient.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24690#discussion_r2052102817
Re: RFR: 8354724: BufferedReader readAllLines and readString methods [v6]
On Fri, 18 Apr 2025 20:10:32 GMT, Brian Burkhalter wrote:
>> Implement the requested methods and add a test thereof.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8354724: Remove unused import
test/jdk/java/io/BufferedReader/ReadAll.java line 70:
> 68: int size = rnd.nextInt(2, 16386);
> 69:
> 70: try (FileChannel fc = FileChannel.open(path, CREATE, WRITE);) {
Suggestion:
try (FileChannel fc = FileChannel.open(path, CREATE, WRITE)) {
test/jdk/java/io/BufferedReader/ReadAll.java line 104:
> 102: List lines;
> 103: try (FileReader fr = new FileReader(file);
> 104: BufferedReader br = new BufferedReader(fr);) {
Suggestion:
BufferedReader br = new BufferedReader(fr)) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24728#discussion_r2052118847
PR Review Comment: https://git.openjdk.org/jdk/pull/24728#discussion_r2052119014
Re: RFR: 8077587: BigInteger Roots [v19]
On Sun, 20 Apr 2025 16:07:56 GMT, fabioromano1 wrote: >> This PR implements nth root computation for `BigInteger`s using Newton >> method and optimizes `BigInteger.pow(int)` method. >> [Here is a proof of convergence of the recurrence >> used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) > > fabioromano1 has updated the pull request incrementally with one additional > commit since the last revision: > > Code simplification src/java.base/share/classes/java/math/MutableBigInteger.java line 2002: > 2000: // Don't use conditional-or to ensure to do both divisions > 2001: if (rToN.divideOneWord(n, q1) != 0 > 2002: | !q1.divide(new MutableBigInteger(rToN1.mag), > delta).isZero()) Suggestion: // Don't use conditional-or to ensure to do both divisions [because ...] if ((rToN.divideOneWord(n, q1) != 0) | (!q1.divide(new MutableBigInteger(rToN1.mag), delta).isZero())) ... add parens for clarity. - PR Review Comment: https://git.openjdk.org/jdk/pull/24690#discussion_r2052135457
Re: RFR: 8354774: DocumentBuilderFactory getAttribute throws NPE [v2]
On Fri, 18 Apr 2025 21:26:23 GMT, Joe Wang wrote:
>> Fix a NPE on calling DocumentBuilderFactory::getAttribute, refer to the bug
>> report.
>>
>> Also in this patch: consolidates get and set properties to use the same Util
>> methods to reduce potential errors when code changes.
>>
>> Test:
>> Tier1 - 3 passed
>> JCK test passed
>
> Joe Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove unused variable pName; remove unused imports
src/java.xml/share/classes/jdk/xml/internal/JdkXmlUtils.java line 133:
> 131: String property) {
> 132: String value = null;
> 133: if (xsm != null && (value = xsm.getLimitAsString(property)) !=
> null) {
Suggestion:
if (xsm != null && (value = xsm.getLimitAsString(property)) != null) {
test/jaxp/javax/xml/jaxp/unittest/common/PropertiesTest.java line 128:
> 126: case DOM:
> 127: DocumentBuilderFactory dbf =
> DocumentBuilderFactory.newDefaultInstance();
> 128: if (apiValue != null) dbf.setAttribute(apiProperty,
> apiValue);
Suggestion:
if (apiValue != null) dbf.setAttribute(apiProperty, apiValue);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24732#discussion_r2052113086
PR Review Comment: https://git.openjdk.org/jdk/pull/24732#discussion_r2052112697
RFR: 8355240: Remove unused Import in StringUTF16
This PR is to remove unused imports after PR #16425 - Commit messages: - remove unused import Changes: https://git.openjdk.org/jdk/pull/24788/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24788&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8355240 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24788.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24788/head:pull/24788 PR: https://git.openjdk.org/jdk/pull/24788
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v6]
On Mon, 21 Apr 2025 17:42:33 GMT, Chen Liang wrote: >> When jdeps was migrated from old classfile to ClassFile API, the parsing >> semantic changed - error checks are now made lazily, and nested crashes from >> malformed signature or other problems is now latent, after a `ClassModel` >> instance is available. (The old error check existed only for constructing a >> `ClassModel`) >> >> To address this issue, I have updated the way of iterating class files to be >> handler/consumer based like in the ClassFile API. This has the advantage >> that when one invocation of the handler fails of a `ClassFileError`, other >> invocations for other class files can proceed, and the exception handler has >> sufficient information to report a useful message indicating the source of >> error. >> >> For the particular example of examining a proguard processed >> `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: >> >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 59, expected an identifier: >> Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: >> scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 49, expected an identifier: >> Lscala/collection/parallel/mutable/ParArray.;: >> scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) >> >> >> Now, jdeps shows the bad class files. Inspection into the files reveal that >> proguard incorrectly deleted the simple class names with trailing `$`, for >> example, the original signature of the broken ParArray was >> `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so >> the `ParArrayIterator$` part was incorrectly dropped by proguard. >> >> Testing: langtools/tools/jdeps. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Use simple file name due to windows directory separators Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24604#pullrequestreview-2782534687
Re: RFR: 8351623: VectorAPI: Refactor subword gather load and add SVE implementation
On Sun, 20 Apr 2025 03:28:48 GMT, SendaoYan wrote: >> ### Summary: >> [JDK-8318650](http://java-service.client.nvidia.com/?q=8318650) added the >> hotspot intrinsifying of subword gather load APIs for X86 platforms [1]. >> This patch aims at implementing the equivalent functionality for AArch64 SVE >> platform. In addition to the AArch64 backend support, this patch also >> refactors the API implementation in Java side and the compiler mid-end part >> to make the operations more efficient and maintainable across different >> architectures. >> >> ### Background: >> Vector gather load APIs load values from memory addresses calculated by >> adding a base pointer to integer indices stored in an int array. SVE >> provides native vector gather load instructions for byte/short types using >> an int vector saving indices (see [2][3]). >> >> The number of loaded elements must match the index vector's element count. >> Since int elements are 4/2 times larger than byte/short elements, and given >> `MaxVectorSize` constraints, the operation may need to be splitted into >> multiple parts. >> >> Using a 128-bit byte vector gather load as an example, there are four >> scenarios with different `MaxVectorSize`: >> >> 1. `MaxVectorSize = 16, byte_vector_size = 16`: >>- Can load 4 indices per vector register >>- So can finish 4 bytes per gather-load operation >>- Requires 4 times of gather-loads and final merge >>Example: >>``` >>byte[] arr = [a, b, c, d, e, f, g, h, i, g, k, l, m, n, o, p, ...] >>int[] idx = [3, 2, 4, 1, 5, 7, 5, 2, 0, 6, 7, 1, 15, 10, 11, 9] >> >>4 gather-load: >>idx_v1 = [1 4 2 3]gather_v1 = [ becd] >>idx_v2 = [2 5 7 5]gather_v2 = [ cfhf] >>idx_v3 = [1 7 6 0]gather_v3 = [ bhga] >>idx_v4 = [9 11 10 15] gather_v4 = [ jlkp] >>merge: v = [jlkp bhga cfhf becd] >>``` >> >> 2. `MaxVectorSize = 32, byte_vector_size = MaxVectorSize / 2`: >>- Can load 8 indices per vector register >>- So can finish 8 bytes per gather-load operation >>- Requires 2 times of gather-loads and merge >>Example: >>``` >>byte[] arr = [a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, ...] >>int[] index = [3, 2, 4, 1, 5, 7, 5, 2, 0, 6, 7, 1, 15, 10, 11, 9] >> >>2 gather-load: >>idx_v1 = [2 5 7 5 1 4 2 3] >>idx_v2 = [9 11 10 15 1 7 6 0] >>gather_v1 = [ cfhf becd] >>gather_v2 = [ jlkp bhga] >>merge: v = [ jlkp bhga cfhf becd] >>``` >> >> 3. `MaxVectorSize = 64, byte_v... > > test/hotspot/jtreg/compiler/vectorapi/VectorGatherSubwordTest.java line 39: > >> 37: * @modules jdk.incubator.vector >> 38: * >> 39: * @run driver compiler.vectorapi.VectorGatherSubwordTest > > Should we use `@run main` instead of `@run driver` Thanks for taking a look at this PR! I think it's fine using `@run main` instead. - PR Review Comment: https://git.openjdk.org/jdk/pull/24679#discussion_r2053187161
Re: RFR: 8352003: Support --add-opens with -XX:+AOTClassLinking [v5]
> This RFE allows --add-opens to be specified for AOT cache creation. AOT cache > can be used during production run with --add-opens option as long as the same > set of options is used during assembly phase. > > Passed tiers 1 - 4 testing. Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision: trailing whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/24695/files - new: https://git.openjdk.org/jdk/pull/24695/files/95f2e9c6..465261f7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24695&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24695&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24695.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24695/head:pull/24695 PR: https://git.openjdk.org/jdk/pull/24695
Re: RFR: 8352003: Support --add-opens with -XX:+AOTClassLinking [v5]
On Tue, 22 Apr 2025 05:31:01 GMT, Calvin Cheung wrote: >> This RFE allows --add-opens to be specified for AOT cache creation. AOT >> cache can be used during production run with --add-opens option as long as >> the same set of options is used during assembly phase. >> >> Passed tiers 1 - 4 testing. > > Calvin Cheung has updated the pull request incrementally with one additional > commit since the last revision: > > trailing whitespace LGTM - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24695#pullrequestreview-2782806700
Re: RFR: 8352003: Support --add-opens with -XX:+AOTClassLinking [v3]
On Mon, 21 Apr 2025 22:54:37 GMT, Calvin Cheung wrote: >> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line >> 165: >> >>> 163: BootLoader.getUnnamedModule(); // trigger of >>> BootLoader. >>> 164: >>> CDS.defineArchivedModules(ClassLoaders.platformClassLoader(), >>> ClassLoaders.appClassLoader()); >>> 165: boolean extraExportsOrOpens = >>> addExtraExportsAndOpens(bootLayer); >> >> (1) The returned value of `addExtraExportsAndOpens()` is not used. So I >> think this function can be changed to `void`, and all occurrences of the >> local variable `extraExportsOrOpens` can be removed. >> >> (2) I traced the code paths that depend on the effects of `--add-opens` and >> `--add-exports`. It Looks like some of the effects are recorded in the >> `java.lang.Module$ReflectionData::export` table: >> >> https://github.com/openjdk/jdk/blob/684d3b336e9cb31707d35e75f9b785e04e1fdbee/src/java.base/share/classes/java/lang/Module.java#L398C2-L412 >> >> >> /** >> * A module (1st key) exports or opens a package to another module >> * (2nd key). The map value is a map of package name to a boolean >> * that indicates if the package is opened. >> */ >> static final WeakPairMap> >> exports = >> new WeakPairMap<>(); >> >> >> This table is *not* stored as part of the `ArchivedBootLayer`, so we must >> re-initialize this table in the production run. @AlanBateman could you >> confirm that this is correct. >> >> Eventually, we should enhance the `ArchivedBootLayer` to also include the >> tables in `Module$ReflectionData`. That will obviate the call to >> `addExtraExportsAndOpens()` and save a few bytecodes during start-up (but >> the overall impact would be small, so it's not critical in the current PR). >> Because these tables use WeakReference, we need to wait for >> [JDK-8354897](https://bugs.openjdk.org/browse/JDK-8354897). > > Fixed (1) above. > (2) I traced the code paths that depend on the effects of `--add-opens` and > `--add-exports`. It Looks like some of the effects are recorded in the > `java.lang.Module$ReflectionData::export` table: > > https://github.com/openjdk/jdk/blob/684d3b336e9cb31707d35e75f9b785e04e1fdbee/src/java.base/share/classes/java/lang/Module.java#L398C2-L412 > > ``` > /** > * A module (1st key) exports or opens a package to another module > * (2nd key). The map value is a map of package name to a boolean > * that indicates if the package is opened. > */ > static final WeakPairMap> > exports = > new WeakPairMap<>(); > ``` > > This table is _not_ stored as part of the `ArchivedBootLayer`, so we must > re-initialize this table in the production run. @AlanBateman could you > confirm that this is correct. In the changes for "JEP draft: Prepare final means final", this is changed significantly so that reflectively exporting or opening a package during startup will add to openPackages/exportedPackages. Once the VM is fully initialized then reflective change via the addExports/addOpens API make use of ReflectionData. So no ReflectionData or weak refs setup for --add-exports/--add-opens. We could potential separate this out in advance so that ReflectionData doesn't need to be re-initialized. - PR Review Comment: https://git.openjdk.org/jdk/pull/24695#discussion_r2053392827
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v6]
On Mon, 21 Apr 2025 17:42:33 GMT, Chen Liang wrote: >> When jdeps was migrated from old classfile to ClassFile API, the parsing >> semantic changed - error checks are now made lazily, and nested crashes from >> malformed signature or other problems is now latent, after a `ClassModel` >> instance is available. (The old error check existed only for constructing a >> `ClassModel`) >> >> To address this issue, I have updated the way of iterating class files to be >> handler/consumer based like in the ClassFile API. This has the advantage >> that when one invocation of the handler fails of a `ClassFileError`, other >> invocations for other class files can proceed, and the exception handler has >> sufficient information to report a useful message indicating the source of >> error. >> >> For the particular example of examining a proguard processed >> `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: >> >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 59, expected an identifier: >> Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: >> scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 49, expected an identifier: >> Lscala/collection/parallel/mutable/ParArray.;: >> scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) >> >> >> Now, jdeps shows the bad class files. Inspection into the files reveal that >> proguard incorrectly deleted the simple class names with trailing `$`, for >> example, the original signature of the broken ParArray was >> `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so >> the `ParArrayIterator$` part was incorrectly dropped by proguard. >> >> Testing: langtools/tools/jdeps. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Use simple file name due to windows directory separators Thanks for the reviews! Reran tier 1 tests which includes this jdeps test and now everything passes. No related failure in tier2. - PR Comment: https://git.openjdk.org/jdk/pull/24604#issuecomment-2819970755
Integrated: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine
On Fri, 11 Apr 2025 22:45:55 GMT, Chen Liang wrote: > When jdeps was migrated from old classfile to ClassFile API, the parsing > semantic changed - error checks are now made lazily, and nested crashes from > malformed signature or other problems is now latent, after a `ClassModel` > instance is available. (The old error check existed only for constructing a > `ClassModel`) > > To address this issue, I have updated the way of iterating class files to be > handler/consumer based like in the ClassFile API. This has the advantage that > when one invocation of the handler fails of a `ClassFileError`, other > invocations for other class files can proceed, and the exception handler has > sufficient information to report a useful message indicating the source of > error. > > For the particular example of examining a proguard processed > `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: > > Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected > character ; at position 59, expected an identifier: > Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: > scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) > Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected > character ; at position 49, expected an identifier: > Lscala/collection/parallel/mutable/ParArray.;: > scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) > > > Now, jdeps shows the bad class files. Inspection into the files reveal that > proguard incorrectly deleted the simple class names with trailing `$`, for > example, the original signature of the broken ParArray was > `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so the > `ParArrayIterator$` part was incorrectly dropped by proguard. > > Testing: langtools/tools/jdeps. This pull request has now been integrated. Changeset: 0be3f163 Author:Chen Liang URL: https://git.openjdk.org/jdk/commit/0be3f163ed12db305673928d97f975d6f6bb6b1c Stats: 357 lines in 8 files changed: 201 ins; 116 del; 40 mod 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine Reviewed-by: jpai, henryjen - PR: https://git.openjdk.org/jdk/pull/24604
Re: RFR: 8354774: DocumentBuilderFactory getAttribute throws NPE [v3]
On Mon, 21 Apr 2025 16:19:30 GMT, Joe Wang wrote: >> Fix a NPE on calling DocumentBuilderFactory::getAttribute, refer to the bug >> report. >> >> Also in this patch: consolidates get and set properties to use the same Util >> methods to reduce potential errors when code changes. >> >> Test: >> Tier1 - 3 passed >> JCK test passed > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > adjust spaces Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24732#pullrequestreview-2781828554
Re: RFR: 8354774: DocumentBuilderFactory getAttribute throws NPE [v3]
On Mon, 21 Apr 2025 16:19:30 GMT, Joe Wang wrote: >> Fix a NPE on calling DocumentBuilderFactory::getAttribute, refer to the bug >> report. >> >> Also in this patch: consolidates get and set properties to use the same Util >> methods to reduce potential errors when code changes. >> >> Test: >> Tier1 - 3 passed >> JCK test passed > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > adjust spaces Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24732#pullrequestreview-2781842304
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v3]
On Mon, 21 Apr 2025 14:59:59 GMT, Chen Liang wrote: >> When jdeps was migrated from old classfile to ClassFile API, the parsing >> semantic changed - error checks are now made lazily, and nested crashes from >> malformed signature or other problems is now latent, after a `ClassModel` >> instance is available. (The old error check existed only for constructing a >> `ClassModel`) >> >> To address this issue, I have updated the way of iterating class files to be >> handler/consumer based like in the ClassFile API. This has the advantage >> that when one invocation of the handler fails of a `ClassFileError`, other >> invocations for other class files can proceed, and the exception handler has >> sufficient information to report a useful message indicating the source of >> error. >> >> For the particular example of examining a proguard processed >> `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: >> >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 59, expected an identifier: >> Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: >> scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 49, expected an identifier: >> Lscala/collection/parallel/mutable/ParArray.;: >> scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) >> >> >> Now, jdeps shows the bad class files. Inspection into the files reveal that >> proguard incorrectly deleted the simple class names with trailing `$`, for >> example, the original signature of the broken ParArray was >> `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so >> the `ParArrayIterator$` part was incorrectly dropped by proguard. >> >> Testing: langtools/tools/jdeps. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Add a test Thank you Chen for adding the test. The test looks good to me. I verified locally that it does indeed reproduce the original issue without the fix. I've just one final request - since the `DirectoryIterator` has been updated in this PR, I think it would be good to include in your test, a `jdeps` invocation against a directory just like against you do it for the JAR file. - PR Review: https://git.openjdk.org/jdk/pull/24604#pullrequestreview-2781628423
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v3]
On Mon, 21 Apr 2025 14:59:59 GMT, Chen Liang wrote: >> When jdeps was migrated from old classfile to ClassFile API, the parsing >> semantic changed - error checks are now made lazily, and nested crashes from >> malformed signature or other problems is now latent, after a `ClassModel` >> instance is available. (The old error check existed only for constructing a >> `ClassModel`) >> >> To address this issue, I have updated the way of iterating class files to be >> handler/consumer based like in the ClassFile API. This has the advantage >> that when one invocation of the handler fails of a `ClassFileError`, other >> invocations for other class files can proceed, and the exception handler has >> sufficient information to report a useful message indicating the source of >> error. >> >> For the particular example of examining a proguard processed >> `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: >> >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 59, expected an identifier: >> Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: >> scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 49, expected an identifier: >> Lscala/collection/parallel/mutable/ParArray.;: >> scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) >> >> >> Now, jdeps shows the bad class files. Inspection into the files reveal that >> proguard incorrectly deleted the simple class names with trailing `$`, for >> example, the original signature of the broken ParArray was >> `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so >> the `ParArrayIterator$` part was incorrectly dropped by proguard. >> >> Testing: langtools/tools/jdeps. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Add a test test/langtools/tools/jdeps/MalformedClassesTest.java line 28: > 26: * @bug 8341608 > 27: * @summary Tests for jdeps tool with jar files with malformed classes > 28: * @library lib /test/lib jdk.jdeps I am not too familiar with the `jdk.jdeps` style usage of `@library` here. Does the test still function without that `jdk.jdeps` entry? I briefly looked up the documentation of `@library` https://openjdk.org/jtreg/tag-spec.html#tags and it isn't clear to me what jtreg does with such a declaration. - PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2052629495
Re: RFR: 8355177: Speed up StringBuilder::append(char[]) via UTF16::compress & Unsafe::copyMemory [v2]
> In BufferedReader.readLine and other similar scenarios, we need to use > StringBuilder.append(char[]) to build the string. > > For these scenarios, we can use the intrinsic method StringUTF16.compress and > Unsafe.copyMemory instead of the character copy of the char-by-char loop to > improve the speed. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: putCharsUnchecked - Changes: - all: https://git.openjdk.org/jdk/pull/24773/files - new: https://git.openjdk.org/jdk/pull/24773/files/ee356d3a..ec6482b0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24773&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24773&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/24773.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24773/head:pull/24773 PR: https://git.openjdk.org/jdk/pull/24773
Re: RFR: 8355177: Speed up StringBuilder::append(char[]) via UTF16::compress & Unsafe::copyMemory [v2]
On Mon, 21 Apr 2025 15:25:34 GMT, Chen Liang wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> putCharsUnchecked > > src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1773: > >> 1771: int compressed = StringUTF16.compress(s, off, val, count, >> end - off); >> 1772: count += compressed; >> 1773: off += compressed; > > Should we update `this.count` eagerly after compression? In the StringLatin1.canEncode(c) branch of the original code, this.count is not updated either, and the behavior is the same as before. - PR Review Comment: https://git.openjdk.org/jdk/pull/24773#discussion_r2052637495
RFR: 8355177: Speed up StringBuilder::append(char[]) via UTF16::compress & Unsafe::copyMemory
In BufferedReader.readLine and other similar scenarios, we need to use StringBuilder.append(char[]) to build the string. For these scenarios, we can use the intrinsic method StringUTF16.compress and Unsafe.copyMemory instead of the character copy of the char-by-char loop to improve the speed. - Commit messages: - copyright - Using StringUTF16.compress to speed up LATIN1 StringBuilder append(char[]) - Using Unsafe.copyMemory to speed up UTF16 StringBuilder append(char[]) - add append(char[]) benchmark Changes: https://git.openjdk.org/jdk/pull/24773/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24773&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8355177 Stats: 46 lines in 3 files changed: 40 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/24773.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24773/head:pull/24773 PR: https://git.openjdk.org/jdk/pull/24773
Re: RFR: 8355177: Speed up StringBuilder::append(char[]) via UTF16::compress & Unsafe::copyMemory
On Mon, 21 Apr 2025 07:00:36 GMT, Shaojin Wen wrote: > In BufferedReader.readLine and other similar scenarios, we need to use > StringBuilder.append(char[]) to build the string. > > For these scenarios, we can use the intrinsic method StringUTF16.compress and > Unsafe.copyMemory instead of the character copy of the char-by-char loop to > improve the speed. Below are the performance numbers on a MacBookPro M1 Max, showing a 112% speed increase when coder = LATIN1 and a 44.19% speed increase when coder = UTF16. # shell git remote add wenshao g...@github.com:wenshao/jdk.git git fetch wenshao # basline git clone ee356d3af177877e2702db08a3b55d170a7e454c make test TEST="micro:java.lang.StringBuilders.appendWithCharArray" # current git clone cd5137097b4a7be370cf60df9aa5000203ea99c0 make test TEST="micro:java.lang.StringBuilders.appendWithCharArray" # Performance Numbrers -Benchmark Mode Cnt Score Error Units (baseline cd5137097b4) -StringBuilders.appendWithCharArrayLatin1 avgt 15 33.039 ± 0.059 ns/op -StringBuilders.appendWithCharArrayUTF16 avgt 15 19.977 ± 0.054 ns/op +Benchmark Mode Cnt Score Error Units (current ee356d3af17) +StringBuilders.appendWithCharArrayLatin1 avgt 15 15.533 ± 0.039 ns/op +112.70% +StringBuilders.appendWithCharArrayUTF16 avgt 15 13.868 ± 0.053 ns/op +44.19% - PR Comment: https://git.openjdk.org/jdk/pull/24773#issuecomment-2817873466
Re: RFR: 8077587: BigInteger Roots [v19]
On Mon, 21 Apr 2025 10:05:21 GMT, Andrew Haley wrote: >> fabioromano1 has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Code simplification > > src/java.base/share/classes/java/math/MutableBigInteger.java line 1924: > >> 1922: * @implNote The implementation is based on the material in Henry >> S. Warren, >> 1923: * Jr., Hacker's Delight (2nd ed.) (Addison Wesley, 2013), >> 279-282. >> 1924: * > > * @implNote The implementation is based on the material in Henry S. Warren, > * Jr., Hacker's Delight (2nd ed.) (Addison Wesley, 2013), 279-282. > * > > I'm looking at this reference, and I only see integer square root here. But > this is n >= 3. > > I do see an explanation at [nth > root](https://en.wikipedia.org/wiki/Nth_root#Computing_principal_roots) I have put a proof of the recurrence in the description of the PR. - PR Review Comment: https://git.openjdk.org/jdk/pull/24690#discussion_r2052298673
Re: RFR: 8077587: BigInteger Roots [v19]
On Mon, 21 Apr 2025 10:14:05 GMT, Andrew Haley wrote: > That's very nice. It would be even nicer if this was a permalink into the JDK > repo, and a reference in the source code. @theRealAph Ok. It would be useful to have a link to an explanation on how this can be done, if there is one. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/24690#issuecomment-2818254412
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v6]
> When jdeps was migrated from old classfile to ClassFile API, the parsing > semantic changed - error checks are now made lazily, and nested crashes from > malformed signature or other problems is now latent, after a `ClassModel` > instance is available. (The old error check existed only for constructing a > `ClassModel`) > > To address this issue, I have updated the way of iterating class files to be > handler/consumer based like in the ClassFile API. This has the advantage that > when one invocation of the handler fails of a `ClassFileError`, other > invocations for other class files can proceed, and the exception handler has > sufficient information to report a useful message indicating the source of > error. > > For the particular example of examining a proguard processed > `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: > > Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected > character ; at position 59, expected an identifier: > Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: > scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) > Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected > character ; at position 49, expected an identifier: > Lscala/collection/parallel/mutable/ParArray.;: > scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) > > > Now, jdeps shows the bad class files. Inspection into the files reveal that > proguard incorrectly deleted the simple class names with trailing `$`, for > example, the original signature of the broken ParArray was > `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so the > `ParArrayIterator$` part was incorrectly dropped by proguard. > > Testing: langtools/tools/jdeps. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Use simple file name due to windows directory separators - Changes: - all: https://git.openjdk.org/jdk/pull/24604/files - new: https://git.openjdk.org/jdk/pull/24604/files/32bc0a32..a2b1dc70 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24604&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24604&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24604.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24604/head:pull/24604 PR: https://git.openjdk.org/jdk/pull/24604
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v6]
On Mon, 21 Apr 2025 17:42:33 GMT, Chen Liang wrote: >> When jdeps was migrated from old classfile to ClassFile API, the parsing >> semantic changed - error checks are now made lazily, and nested crashes from >> malformed signature or other problems is now latent, after a `ClassModel` >> instance is available. (The old error check existed only for constructing a >> `ClassModel`) >> >> To address this issue, I have updated the way of iterating class files to be >> handler/consumer based like in the ClassFile API. This has the advantage >> that when one invocation of the handler fails of a `ClassFileError`, other >> invocations for other class files can proceed, and the exception handler has >> sufficient information to report a useful message indicating the source of >> error. >> >> For the particular example of examining a proguard processed >> `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: >> >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 59, expected an identifier: >> Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: >> scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 49, expected an identifier: >> Lscala/collection/parallel/mutable/ParArray.;: >> scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) >> >> >> Now, jdeps shows the bad class files. Inspection into the files reveal that >> proguard incorrectly deleted the simple class names with trailing `$`, for >> example, the original signature of the broken ParArray was >> `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so >> the `ParArrayIterator$` part was incorrectly dropped by proguard. >> >> Testing: langtools/tools/jdeps. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Use simple file name due to windows directory separators The improper path separator in the new test is causing test failure in windows. Updated and fixed it. - PR Comment: https://git.openjdk.org/jdk/pull/24604#issuecomment-2819120641
Integrated: 8354344: Test behavior after cut-over for future ISO 4217 currency
On Wed, 16 Apr 2025 23:06:19 GMT, Justin Lu wrote: > Please review this PR which improves the _ValidateISO4217_ Currency test by > adding testing of future currencies after the transition date. > > This is done by creating a patched version of Currency that replaces > `System.currentTimeMillis()` calls with a mocked value equivalent to > `Long.MAX_VALUE`. A module patch is then applied to supply the new Currency > class files. > > The mocked time behavior is tested by using the `currency.properties` > override in a separate invocation. This pull request has now been integrated. Changeset: 1526dd81 Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/1526dd81d9b5bf4abaac1546c370cf7a056d01dc Stats: 133 lines in 2 files changed: 128 ins; 1 del; 4 mod 8354344: Test behavior after cut-over for future ISO 4217 currency Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/24701
Re: RFR: 8354724: BufferedReader readAllLines and readString methods [v8]
> Implement the requested methods and add a test thereof. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8354724: Change "lines" to "characters" in spec of Reader.readAllChars, where appropriate - Changes: - all: https://git.openjdk.org/jdk/pull/24728/files - new: https://git.openjdk.org/jdk/pull/24728/files/b2c3481a..d35d9ebb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24728&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24728&range=06-07 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/24728.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24728/head:pull/24728 PR: https://git.openjdk.org/jdk/pull/24728
Re: RFR: 8355223: Improve documentation on @IntrinsicCandidate [v2]
> In offline discussion, we noted that the documentation on this annotation > does not recommend minimizing the intrinsified section and moving whatever > can be done in Java to Java; thus I prepared this documentation update, to > shrink a "TLDR" essay to something concise for readers, such as pointing to > that list at `vmIntrinsics.hpp` instead of "a list". Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Refine validation and defensive copying - Changes: - all: https://git.openjdk.org/jdk/pull/24777/files - new: https://git.openjdk.org/jdk/pull/24777/files/6e8b3254..e8adab3c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24777&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24777&range=00-01 Stats: 9 lines in 1 file changed: 1 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/24777.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24777/head:pull/24777 PR: https://git.openjdk.org/jdk/pull/24777
RFR: 8355215: Add @spec tags to Emoji related methods
Adding @spec tags to Emoji related methods in the Character class. A CSR has also been drafted. - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/24779/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24779&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8355215 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24779.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24779/head:pull/24779 PR: https://git.openjdk.org/jdk/pull/24779
Re: RFR: 8354724: BufferedReader readAllLines and readString methods [v5]
On Fri, 18 Apr 2025 15:10:02 GMT, Chen Liang wrote: >> Maybe it can be implemented by referring to `InputStream::readNBytes(int)` >> (The default implementation of `InputStream::readAllBytes()` is based on it): >> >> https://github.com/openjdk/jdk/blob/22e8a97a1ce4e1c781fbc6f1e271c477fe95f069/src/java.base/share/classes/java/io/InputStream.java#L396-L458 > > If accumulation of array is necessary, I think the "variable sized array" in > #24232 may help/ https://github.com/openjdk/jdk/pull/24728#issuecomment-2815687444 - PR Review Comment: https://git.openjdk.org/jdk/pull/24728#discussion_r2052949330
Re: RFR: 8355215: Add @spec tags to Emoji related methods
On Mon, 21 Apr 2025 20:11:47 GMT, Naoto Sato wrote: > Adding @spec tags to Emoji related methods in the Character class. A CSR has > also been drafted. Marked as reviewed by joehw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24779#pullrequestreview-2782236067
Re: RFR: 8355215: Add @spec tags to Emoji related methods
On Mon, 21 Apr 2025 20:11:47 GMT, Naoto Sato wrote: > Adding @spec tags to Emoji related methods in the Character class. A CSR has > also been drafted. CSR also Reviewed. - Marked as reviewed by iris (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24779#pullrequestreview-2782238754
Re: RFR: 8077587: BigInteger Roots [v19]
On Sun, 20 Apr 2025 16:07:56 GMT, fabioromano1 wrote: >> This PR implements nth root computation for `BigInteger`s using Newton >> method and optimizes `BigInteger.pow(int)` method. >> [Here is a proof of convergence of the recurrence >> used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) > > fabioromano1 has updated the pull request incrementally with one additional > commit since the last revision: > > Code simplification src/java.base/share/classes/java/math/MutableBigInteger.java line 1924: > 1922: * @implNote The implementation is based on the material in Henry > S. Warren, > 1923: * Jr., Hacker's Delight (2nd ed.) (Addison Wesley, 2013), > 279-282. > 1924: * * @implNote The implementation is based on the material in Henry S. Warren, * Jr., Hacker's Delight (2nd ed.) (Addison Wesley, 2013), 279-282. * I'm looking at this reference, and I only see integer square root here. But this is n >= 3. I do see an explanation at [nth root](https://en.wikipedia.org/wiki/Nth_root#Computing_principal_roots) - PR Review Comment: https://git.openjdk.org/jdk/pull/24690#discussion_r2052217018
Re: RFR: 8343110: Add getChars(int, int, char[], int) to CharSequence and CharBuffer [v7]
On Sun, 23 Mar 2025 10:33:42 GMT, Markus KARG wrote: >> src/java.base/share/classes/java/nio/X-Buffer.java.template line 2356: >> >>> 2354: #end[streamableType] >>> 2355: >>> 2356: #if[char] >> >> Can we merge this with `// -- Other char stuff --` on line 1895? >> >> On a side note, we can optimize a lot of Appendable operations that transfer >> from CharSequence on CharBuffer; don't know if you wish to have it in this >> RFE or another. > > Fixed in a3c2add9c16e4c7331c5a7c2848f27b6c0330a17. > > Let's finish this PR first. After that I would be happy to author another PR > with all the optimizations you tell me. 😃 > On a side note, we can optimize a lot of Appendable operations that transfer > from CharSequence on CharBuffer; don't know if you wish to have it in this > RFE or another. @liach FYI, I have just startet work on an experimental impementation for `Appendable`s utilizing `CharSequence.getChars`. So far, it really looks rather primising, as it gets rid of lots of potental copies in favor of absolute bulk get. My target is to turn it into a separate RFE eventually, once 8343110 is finally merged. 😃 - PR Review Comment: https://git.openjdk.org/jdk/pull/21730#discussion_r2052366020
Re: RFR: 8355177: Speed up StringBuilder::append(char[]) via UTF16::compress & Unsafe::copyMemory
On Mon, 21 Apr 2025 07:00:36 GMT, Shaojin Wen wrote: > In BufferedReader.readLine and other similar scenarios, we need to use > StringBuilder.append(char[]) to build the string. > > For these scenarios, we can use the intrinsic method StringUTF16.compress and > Unsafe.copyMemory instead of the character copy of the char-by-char loop to > improve the speed. This might be helpful combined with #21730. - PR Comment: https://git.openjdk.org/jdk/pull/24773#issuecomment-2818748083
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v4]
> When jdeps was migrated from old classfile to ClassFile API, the parsing > semantic changed - error checks are now made lazily, and nested crashes from > malformed signature or other problems is now latent, after a `ClassModel` > instance is available. (The old error check existed only for constructing a > `ClassModel`) > > To address this issue, I have updated the way of iterating class files to be > handler/consumer based like in the ClassFile API. This has the advantage that > when one invocation of the handler fails of a `ClassFileError`, other > invocations for other class files can proceed, and the exception handler has > sufficient information to report a useful message indicating the source of > error. > > For the particular example of examining a proguard processed > `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: > > Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected > character ; at position 59, expected an identifier: > Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: > scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) > Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected > character ; at position 49, expected an identifier: > Lscala/collection/parallel/mutable/ParArray.;: > scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) > > > Now, jdeps shows the bad class files. Inspection into the files reveal that > proguard incorrectly deleted the simple class names with trailing `$`, for > example, the original signature of the broken ParArray was > `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so the > `ParArrayIterator$` part was incorrectly dropped by proguard. > > Testing: langtools/tools/jdeps. Chen Liang has updated the pull request incrementally with two additional commits since the last revision: - Redundant library entry - Use test case to switch between flat dir and jar - Changes: - all: https://git.openjdk.org/jdk/pull/24604/files - new: https://git.openjdk.org/jdk/pull/24604/files/f61e9201..6a7cef62 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24604&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24604&range=02-03 Stats: 28 lines in 1 file changed: 21 ins; 1 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/24604.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24604/head:pull/24604 PR: https://git.openjdk.org/jdk/pull/24604
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v3]
On Mon, 21 Apr 2025 15:44:20 GMT, Jaikiran Pai wrote: >> Chen Liang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add a test > > test/langtools/tools/jdeps/MalformedClassesTest.java line 28: > >> 26: * @bug 8341608 >> 27: * @summary Tests for jdeps tool with jar files with malformed classes >> 28: * @library lib /test/lib jdk.jdeps > > I am not too familiar with the `jdk.jdeps` style usage of `@library` here. > Does the test still function without that `jdk.jdeps` entry? I briefly looked > up the documentation of `@library` > https://openjdk.org/jtreg/tag-spec.html#tags and it isn't clear to me what > jtreg does with such a declaration. Turns out this is not necessary when I have `@build jdk.jdeps/com.sun.tools.jdeps.*`. Thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2052655127
Re: RFR: 8354990: Improve negative tests coverage for jpackage signing [v2]
> Add tests for the following test cases: > - Expired certificate specified for signing; > - Multiple certificates with the same name in one keychain. > > Adding the new tests revealed an issue with MacCertificate - > [JDK-8354989](https://bugs.openjdk.org/browse/JDK-8354989). This issue is > also addressed in this PR. > > Additionally: > - Moved code to verify signatures in MacSignVerify class and reworked > SigningBase verify functions to use MacSignVerify API. Alexey Semenyuk has updated the pull request incrementally with one additional commit since the last revision: Fix compilation error - Changes: - all: https://git.openjdk.org/jdk/pull/24762/files - new: https://git.openjdk.org/jdk/pull/24762/files/f60e1da8..95a45098 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24762&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24762&range=00-01 Stats: 6 lines in 1 file changed: 0 ins; 3 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/24762.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24762/head:pull/24762 PR: https://git.openjdk.org/jdk/pull/24762
Re: RFR: 8354990: Improve negative tests coverage for jpackage signing
On Fri, 18 Apr 2025 21:11:35 GMT, Alexey Semenyuk wrote: > Add tests for the following test cases: > - Expired certificate specified for signing; > - Multiple certificates with the same name in one keychain. > > Adding the new tests revealed an issue with MacCertificate - > [JDK-8354989](https://bugs.openjdk.org/browse/JDK-8354989). This issue is > also addressed in this PR. > > Additionally: > - Moved code to verify signatures in MacSignVerify class and reworked > SigningBase verify functions to use MacSignVerify API. @sashamatveev PTAL - PR Comment: https://git.openjdk.org/jdk/pull/24762#issuecomment-2819546107
Re: RFR: 8355215: Add @spec tags to Emoji related methods
On Mon, 21 Apr 2025 20:11:47 GMT, Naoto Sato wrote: > Adding @spec tags to Emoji related methods in the Character class. A CSR has > also been drafted. Marked as reviewed by jlu (Committer). - PR Review: https://git.openjdk.org/jdk/pull/24779#pullrequestreview-2782310737
RFR: 8354343: Hardening of Currency tests for not yet defined future ISO 4217 currency
Please review this PR which improves future currency checking for ISO 4217 currencies. Checking for a currency that should not yet exist in the set of available currencies is already done. It should also be explicitly checked that such a currency can not be instantiated as well via the String getter. - Commit messages: - init Changes: https://git.openjdk.org/jdk/pull/24782/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24782&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8354343 Stats: 35 lines in 1 file changed: 34 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24782.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24782/head:pull/24782 PR: https://git.openjdk.org/jdk/pull/24782
Re: RFR: 8077587: BigInteger Roots [v19]
On Sun, 20 Apr 2025 16:07:56 GMT, fabioromano1 wrote: >> This PR implements nth root computation for `BigInteger`s using Newton >> method and optimizes `BigInteger.pow(int)` method. >> [Here is a proof of convergence of the recurrence >> used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) > > fabioromano1 has updated the pull request incrementally with one additional > commit since the last revision: > > Code simplification > [Here is a proof of convergence of the recurrence > used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) That's very nice. It would be even nicer if this was a permalink into the JDK repo, and a reference in the source code. - PR Comment: https://git.openjdk.org/jdk/pull/24690#issuecomment-2818103511
Re: RFR: 8077587: BigInteger Roots [v20]
> This PR implements nth root computation for `BigInteger`s using Newton method > and optimizes `BigInteger.pow(int)` method. > [Here is a proof of convergence of the recurrence > used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) fabioromano1 has updated the pull request incrementally with one additional commit since the last revision: Added reference for proof of convergence in the comment - Changes: - all: https://git.openjdk.org/jdk/pull/24690/files - new: https://git.openjdk.org/jdk/pull/24690/files/21fbf278..e11b32f5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=19 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=18-19 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/24690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24690/head:pull/24690 PR: https://git.openjdk.org/jdk/pull/24690
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v2]
On Mon, 21 Apr 2025 05:10:48 GMT, Jaikiran Pai wrote: >> Chen Liang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Review remarks > > Hello Chen, > >> Unfortunately, I don't think there is a convenient way to make a jar with >> jtreg tests. > > There are several tests in the JDK repo which create a JAR file of their > choice within the test code and then run some tests against the JAR file. In > fact, existing tests for `jdeps` tool already has similar tests. For example, > there's a `JdepsUtil` test library which has a `createJar(...)` utility > method which some of these tests use. This new test can use that utility if > necessary. > > In addition to testing this change against a JAR file, I think the test could > also include running `jdeps` against a directory which contains more than one > class file with one of the class file exhibiting the reported issue. That > will then be able to reproduce the issue and verify the fix when running > `jdeps` against a directory. @jaikiran Done. Please review again. - PR Comment: https://git.openjdk.org/jdk/pull/24604#issuecomment-2818674103
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v4]
On Mon, 21 Apr 2025 16:06:25 GMT, Chen Liang wrote: >> When jdeps was migrated from old classfile to ClassFile API, the parsing >> semantic changed - error checks are now made lazily, and nested crashes from >> malformed signature or other problems is now latent, after a `ClassModel` >> instance is available. (The old error check existed only for constructing a >> `ClassModel`) >> >> To address this issue, I have updated the way of iterating class files to be >> handler/consumer based like in the ClassFile API. This has the advantage >> that when one invocation of the handler fails of a `ClassFileError`, other >> invocations for other class files can proceed, and the exception handler has >> sufficient information to report a useful message indicating the source of >> error. >> >> For the particular example of examining a proguard processed >> `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: >> >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 59, expected an identifier: >> Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: >> scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 49, expected an identifier: >> Lscala/collection/parallel/mutable/ParArray.;: >> scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) >> >> >> Now, jdeps shows the bad class files. Inspection into the files reveal that >> proguard incorrectly deleted the simple class names with trailing `$`, for >> example, the original signature of the broken ParArray was >> `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so >> the `ParArrayIterator$` part was incorrectly dropped by proguard. >> >> Testing: langtools/tools/jdeps. > > Chen Liang has updated the pull request incrementally with two additional > commits since the last revision: > > - Redundant library entry > - Use test case to switch between flat dir and jar Thank you for adding this test. The updated test looks good to me and I believe it add the necessary coverage for this fix. Since this is a freshly added test, it would be good to run the relevant tier in our CI before integrating. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24604#pullrequestreview-2781701303
Re: RFR: 8354774: DocumentBuilderFactory getAttribute throws NPE [v2]
On Mon, 21 Apr 2025 08:11:33 GMT, Andrey Turbanov wrote:
>> Joe Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove unused variable pName; remove unused imports
>
> src/java.xml/share/classes/jdk/xml/internal/JdkXmlUtils.java line 133:
>
>> 131: String property) {
>> 132: String value = null;
>> 133: if (xsm != null && (value = xsm.getLimitAsString(property)) !=
>> null) {
>
> Suggestion:
>
> if (xsm != null && (value = xsm.getLimitAsString(property)) != null) {
Thanks. Spaces adjusted.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24732#discussion_r2052669398
Re: RFR: 8354774: DocumentBuilderFactory getAttribute throws NPE [v3]
> Fix a NPE on calling DocumentBuilderFactory::getAttribute, refer to the bug > report. > > Also in this patch: consolidates get and set properties to use the same Util > methods to reduce potential errors when code changes. > > Test: > Tier1 - 3 passed > JCK test passed Joe Wang has updated the pull request incrementally with one additional commit since the last revision: adjust spaces - Changes: - all: https://git.openjdk.org/jdk/pull/24732/files - new: https://git.openjdk.org/jdk/pull/24732/files/1b8ecd32..022611dd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24732&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24732&range=01-02 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/24732.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24732/head:pull/24732 PR: https://git.openjdk.org/jdk/pull/24732
Re: RFR: 8077587: BigInteger Roots [v21]
> This PR implements nth root computation for `BigInteger`s using Newton method > and optimizes `BigInteger.pow(int)` method. > [Here is a proof of convergence of the recurrence > used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) fabioromano1 has updated the pull request incrementally with two additional commits since the last revision: - Merge branch 'BigInteger-nth-root' of https://github.com/fabioromano1/jdk into BigInteger-nth-root - Revert format code changes - Changes: - all: https://git.openjdk.org/jdk/pull/24690/files - new: https://git.openjdk.org/jdk/pull/24690/files/e11b32f5..8de6b825 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=20 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=19-20 Stats: 9 lines in 1 file changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/24690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24690/head:pull/24690 PR: https://git.openjdk.org/jdk/pull/24690
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v4]
On Mon, 21 Apr 2025 16:06:25 GMT, Chen Liang wrote: >> When jdeps was migrated from old classfile to ClassFile API, the parsing >> semantic changed - error checks are now made lazily, and nested crashes from >> malformed signature or other problems is now latent, after a `ClassModel` >> instance is available. (The old error check existed only for constructing a >> `ClassModel`) >> >> To address this issue, I have updated the way of iterating class files to be >> handler/consumer based like in the ClassFile API. This has the advantage >> that when one invocation of the handler fails of a `ClassFileError`, other >> invocations for other class files can proceed, and the exception handler has >> sufficient information to report a useful message indicating the source of >> error. >> >> For the particular example of examining a proguard processed >> `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: >> >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 59, expected an identifier: >> Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: >> scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 49, expected an identifier: >> Lscala/collection/parallel/mutable/ParArray.;: >> scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) >> >> >> Now, jdeps shows the bad class files. Inspection into the files reveal that >> proguard incorrectly deleted the simple class names with trailing `$`, for >> example, the original signature of the broken ParArray was >> `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so >> the `ParArrayIterator$` part was incorrectly dropped by proguard. >> >> Testing: langtools/tools/jdeps. > > Chen Liang has updated the pull request incrementally with two additional > commits since the last revision: > > - Redundant library entry > - Use test case to switch between flat dir and jar Indeed, I will run tier 1 and 2 which seem to include all langtools tests after merging latest master (not merging mainline before integration is usually the cause of critical build or test failures) - PR Comment: https://git.openjdk.org/jdk/pull/24604#issuecomment-2818940119
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v5]
> When jdeps was migrated from old classfile to ClassFile API, the parsing > semantic changed - error checks are now made lazily, and nested crashes from > malformed signature or other problems is now latent, after a `ClassModel` > instance is available. (The old error check existed only for constructing a > `ClassModel`) > > To address this issue, I have updated the way of iterating class files to be > handler/consumer based like in the ClassFile API. This has the advantage that > when one invocation of the handler fails of a `ClassFileError`, other > invocations for other class files can proceed, and the exception handler has > sufficient information to report a useful message indicating the source of > error. > > For the particular example of examining a proguard processed > `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: > > Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected > character ; at position 59, expected an identifier: > Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: > scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) > Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected > character ; at position 49, expected an identifier: > Lscala/collection/parallel/mutable/ParArray.;: > scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) > > > Now, jdeps shows the bad class files. Inspection into the files reveal that > proguard incorrectly deleted the simple class names with trailing `$`, for > example, the original signature of the broken ParArray was > `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so the > `ParArrayIterator$` part was incorrectly dropped by proguard. > > Testing: langtools/tools/jdeps. Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into fix/jdeps-bad-class-skip - Redundant library entry - Use test case to switch between flat dir and jar - Add a test - Review remarks - 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine - Changes: - all: https://git.openjdk.org/jdk/pull/24604/files - new: https://git.openjdk.org/jdk/pull/24604/files/6a7cef62..32bc0a32 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24604&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24604&range=03-04 Stats: 190967 lines in 560 files changed: 20599 ins; 168910 del; 1458 mod Patch: https://git.openjdk.org/jdk/pull/24604.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24604/head:pull/24604 PR: https://git.openjdk.org/jdk/pull/24604
Re: RFR: 8354053: Remove unused JavaIOFilePermissionAccess [v2]
On Mon, 14 Apr 2025 16:14:03 GMT, Roger Riggs wrote: >> The JavaIOFilePermissionAccess interface is removed from SharedSecrets and >> its implementation (FilePermCompat.java) used by the test is moved to >> java.io FilePermission where cross package access is not needed. >> The test FilePermissionCollectionMerge is updated to access the internal >> implementation in FilePermission. >> Modernized the initialization from the system property >> `jdk.io.permissionsUseCanonicalPath`. >> The remaining support will be removed when FilePermission is removed. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify as suggested in review comments > Convert static methods to instance methods and invoke on the existing > FilePermission. LGTM. In fact, there is no need to keep or test on `newPermPlusAltPath` and `newPermUsingAltPath`. Those were only used in access control checks with the "new" behavior. - Marked as reviewed by weijun (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24603#pullrequestreview-2781713842
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v5]
On Mon, 21 Apr 2025 16:27:03 GMT, Chen Liang wrote: >> When jdeps was migrated from old classfile to ClassFile API, the parsing >> semantic changed - error checks are now made lazily, and nested crashes from >> malformed signature or other problems is now latent, after a `ClassModel` >> instance is available. (The old error check existed only for constructing a >> `ClassModel`) >> >> To address this issue, I have updated the way of iterating class files to be >> handler/consumer based like in the ClassFile API. This has the advantage >> that when one invocation of the handler fails of a `ClassFileError`, other >> invocations for other class files can proceed, and the exception handler has >> sufficient information to report a useful message indicating the source of >> error. >> >> For the particular example of examining a proguard processed >> `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: >> >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 59, expected an identifier: >> Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: >> scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) >> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected >> character ; at position 49, expected an identifier: >> Lscala/collection/parallel/mutable/ParArray.;: >> scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) >> >> >> Now, jdeps shows the bad class files. Inspection into the files reveal that >> proguard incorrectly deleted the simple class names with trailing `$`, for >> example, the original signature of the broken ParArray was >> `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so >> the `ParArrayIterator$` part was incorrectly dropped by proguard. >> >> Testing: langtools/tools/jdeps. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/jdeps-bad-class-skip > - Redundant library entry > - Use test case to switch between flat dir and jar > - Add a test > - Review remarks > - 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in > JDK 22 works fine Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24604#pullrequestreview-2781747490
RE: JDK-8352891 Performance improvements to ByteArrayOutputStream
Following up on MemoryOutputStream, etc. – the conversation has considered a few alternatives: 1. Hide the new class behind ByteArrayOutputStream.unsynchronized() 2. Create a new public class providing views to OutputStream, Channel, InputStream 3. Something to do with an Object[] variant I would personally reject any code review that used “unsynchronized()” to describe this class, since that is only a minor property of the change. I could live with “scalable” or something along those lines. A new public class provides more capabilities than I initially proposed, but of course requires a new public class. 😊 The Object[] variant is interesting but I have no pressing need for it. This probably gets a pin in it unless someone provides a use case. Thoughts appreciated. Thanks! John
Integrated: 8354774: DocumentBuilderFactory getAttribute throws NPE
On Thu, 17 Apr 2025 17:41:56 GMT, Joe Wang wrote: > Fix a NPE on calling DocumentBuilderFactory::getAttribute, refer to the bug > report. > > Also in this patch: consolidates get and set properties to use the same Util > methods to reduce potential errors when code changes. > > Test: > Tier1 - 3 passed > JCK test passed This pull request has now been integrated. Changeset: 684d3b33 Author:Joe Wang URL: https://git.openjdk.org/jdk/commit/684d3b336e9cb31707d35e75f9b785e04e1fdbee Stats: 339 lines in 11 files changed: 219 ins; 71 del; 49 mod 8354774: DocumentBuilderFactory getAttribute throws NPE Reviewed-by: naoto, lancea - PR: https://git.openjdk.org/jdk/pull/24732
Re: RFR: 8352003: Support --add-opens with -XX:+AOTClassLinking [v3]
On Mon, 21 Apr 2025 06:20:46 GMT, Calvin Cheung wrote: >> This RFE allows --add-opens to be specified for AOT cache creation. AOT >> cache can be used during production run with --add-opens option as long as >> the same set of options is used during assembly phase. >> >> Passed tiers 1 - 4 testing. > > Calvin Cheung has updated the pull request incrementally with one additional > commit since the last revision: > > @iklam comment test/hotspot/jtreg/runtime/cds/appcds/jigsaw/modulepath/AddOpens.java line 109: > 107: "--add-opens", addOpensArg, > 108: "--module-path", moduleDir.toString(), > 109: "-m", TEST_MODULE1, "with_add_opens") Could you add a test case for `=ALL-UNNAMED` as well? That is frequently used by applications, and it takes a different code path inside the implementation. I think this can be done by running `com.simple.Main` in the classpath rather than module path. - PR Review Comment: https://git.openjdk.org/jdk/pull/24695#discussion_r205284
Re: RFR: 8352003: Support --add-opens with -XX:+AOTClassLinking [v3]
On Mon, 21 Apr 2025 06:20:46 GMT, Calvin Cheung wrote: >> This RFE allows --add-opens to be specified for AOT cache creation. AOT >> cache can be used during production run with --add-opens option as long as >> the same set of options is used during assembly phase. >> >> Passed tiers 1 - 4 testing. > > Calvin Cheung has updated the pull request incrementally with one additional > commit since the last revision: > > @iklam comment src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 165: > 163: BootLoader.getUnnamedModule(); // trigger of > BootLoader. > 164: > CDS.defineArchivedModules(ClassLoaders.platformClassLoader(), > ClassLoaders.appClassLoader()); > 165: boolean extraExportsOrOpens = > addExtraExportsAndOpens(bootLayer); (1) The returned value of `addExtraExportsAndOpens()` is not used. So I think this function can be changed to `void`, and all occurrences of the local variable `extraExportsOrOpens` can be removed. (2) I traced the code paths that depend on the effects of `--add-opens` and `--add-exports`. It Looks like some of the effects are recorded in the `java.lang.Module$ReflectionData::export` table: https://github.com/openjdk/jdk/blob/684d3b336e9cb31707d35e75f9b785e04e1fdbee/src/java.base/share/classes/java/lang/Module.java#L398C2-L412 /** * A module (1st key) exports or opens a package to another module * (2nd key). The map value is a map of package name to a boolean * that indicates if the package is opened. */ static final WeakPairMap> exports = new WeakPairMap<>(); This table is *not* stored as part of the `ArchivedBootLayer`, so we must re-initialize this table in the production run. @AlanBateman could you confirm that this is correct. Eventually, we should enhance the `ArchivedBootLayer` to also include the tables in `Module$ReflectionData`. That will obviate the call to `addExtraExportsAndOpens()` and save a few bytecodes during start-up (but the overall impact would be small, so it's not critical in the current PR). Because these tables use WeakReference, we need to wait for [JDK-8354897](https://bugs.openjdk.org/browse/JDK-8354897). - PR Review Comment: https://git.openjdk.org/jdk/pull/24695#discussion_r2052840896
Re: RFR: 8344706: Compiler Implementation of Compact Source Files and Instance Main Methods [v4]
On Thu, 10 Apr 2025 01:03:01 GMT, Jan Lahoda wrote: >> This is a PR that implements JEP: Compact Source Files and Instance Main >> Methods. Changes include: >> - `java.io.IO` moved to `java.lang.IO`, and no longer uses >> `System.console()` to implement the methods (thanks to @stuart-marks) >> - `java. ... .IO` is no longer automatically imported in any compilation unit >> - the feature is finalized (i.e. no longer requires `--enable-preview`) > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Add clause about handling of malformed/unmappable bytes. lgtm - Marked as reviewed by vromero (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24438#pullrequestreview-2782056620
Re: RFR: 8354424: java/util/logging/LoggingDeadlock5.java fails intermittently in tier6 [v2]
On Wed, 16 Apr 2025 17:37:07 GMT, David Beaumont wrote: >> Increasing timeout for deadlock detection and adjusting for the timeout >> factor in higher tiers. > > David Beaumont has updated the pull request incrementally with one additional > commit since the last revision: > > Removing test from the problem list. test/jdk/java/util/logging/LoggingDeadlock5.java line 127: > 125: // in higher tiers, so it's necessary to be a bit pessimistic > here. > 126: private final static Duration JOIN_WAIT = > 127: Duration.ofMillis(Utils.adjustTimeout(2000)); Just checking, adjustTimeout does scaling based on the timeout factor given to jtreg? What happens to the expected test running time to the test in lower tiers without as high loads? - PR Review Comment: https://git.openjdk.org/jdk/pull/24687#discussion_r2052873175
RFR: 8355223: Improve documentation on @IntrinsicCandidate
In offline discussion, we noted that the documentation on this annotation does not recommend minimizing the intrinsified section and moving whatever can be done in Java to Java; thus I prepared this documentation update, to shrink a "TLDR" essay to something concise for readers, such as pointing to that list at `vmIntrinsics.hpp` instead of "a list". - Commit messages: - 8355223: Improve documentation on @IntrinsicCandidate Changes: https://git.openjdk.org/jdk/pull/24777/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24777&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8355223 Stats: 91 lines in 1 file changed: 4 ins; 65 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/24777.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24777/head:pull/24777 PR: https://git.openjdk.org/jdk/pull/24777
Re: RFR: 8354724: BufferedReader readAllLines and readString methods [v6]
On Mon, 21 Apr 2025 08:17:30 GMT, Andrey Turbanov wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8354724: Remove unused import
>
> test/jdk/java/io/BufferedReader/ReadAll.java line 70:
>
>> 68: int size = rnd.nextInt(2, 16386);
>> 69:
>> 70: try (FileChannel fc = FileChannel.open(path, CREATE, WRITE);) {
>
> Suggestion:
>
> try (FileChannel fc = FileChannel.open(path, CREATE, WRITE)) {
Fixed in b2c3481.
> test/jdk/java/io/BufferedReader/ReadAll.java line 104:
>
>> 102: List lines;
>> 103: try (FileReader fr = new FileReader(file);
>> 104: BufferedReader br = new BufferedReader(fr);) {
>
> Suggestion:
>
> BufferedReader br = new BufferedReader(fr)) {
Fixed in b2c3481.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24728#discussion_r2052920558
PR Review Comment: https://git.openjdk.org/jdk/pull/24728#discussion_r2052920752
Re: RFR: 8354724: BufferedReader readAllLines and readString methods [v7]
> Implement the requested methods and add a test thereof. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8354724: readString -> readAllChars; update test - Changes: - all: https://git.openjdk.org/jdk/pull/24728/files - new: https://git.openjdk.org/jdk/pull/24728/files/b9424bb4..b2c3481a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24728&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24728&range=05-06 Stats: 20 lines in 2 files changed: 11 ins; 3 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/24728.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24728/head:pull/24728 PR: https://git.openjdk.org/jdk/pull/24728
Re: RFR: 8354343: Hardening of Currency tests for not yet defined future ISO 4217 currency
On Mon, 21 Apr 2025 21:51:35 GMT, Justin Lu wrote: > Please review this PR which improves future currency checking for ISO 4217 > currencies. > > Checking for a currency that should not yet exist in the set of available > currencies is already done. > It should also be explicitly checked that such a currency can not be > instantiated as well via the String getter. LGTM. I think this JIRA issue and the previous test improvement one can be linke to [JDK-8321480](https://bugs.openjdk.org/browse/JDK-8321480), and both have `iso4217` lables. test/jdk/java/util/Currency/ValidateISO4217.java line 183: > 181: setUpPatchedClasses(); > 182: setUpTestingData(); > 183: setUpNotYetDefined(); It may be clearer to move this inside `setUpTestingData()`, and modify the comment there - PR Review: https://git.openjdk.org/jdk/pull/24782#pullrequestreview-2782347271 PR Review Comment: https://git.openjdk.org/jdk/pull/24782#discussion_r2053062123
Re: RFR: 8352003: Support --add-opens with -XX:+AOTClassLinking [v3]
On Mon, 21 Apr 2025 18:39:39 GMT, Ioi Lam wrote: >> Calvin Cheung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> @iklam comment > > src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 165: > >> 163: BootLoader.getUnnamedModule(); // trigger of >> BootLoader. >> 164: >> CDS.defineArchivedModules(ClassLoaders.platformClassLoader(), >> ClassLoaders.appClassLoader()); >> 165: boolean extraExportsOrOpens = >> addExtraExportsAndOpens(bootLayer); > > (1) The returned value of `addExtraExportsAndOpens()` is not used. So I think > this function can be changed to `void`, and all occurrences of the local > variable `extraExportsOrOpens` can be removed. > > (2) I traced the code paths that depend on the effects of `--add-opens` and > `--add-exports`. It Looks like some of the effects are recorded in the > `java.lang.Module$ReflectionData::export` table: > > https://github.com/openjdk/jdk/blob/684d3b336e9cb31707d35e75f9b785e04e1fdbee/src/java.base/share/classes/java/lang/Module.java#L398C2-L412 > > > /** > * A module (1st key) exports or opens a package to another module > * (2nd key). The map value is a map of package name to a boolean > * that indicates if the package is opened. > */ > static final WeakPairMap> > exports = > new WeakPairMap<>(); > > > This table is *not* stored as part of the `ArchivedBootLayer`, so we must > re-initialize this table in the production run. @AlanBateman could you > confirm that this is correct. > > Eventually, we should enhance the `ArchivedBootLayer` to also include the > tables in `Module$ReflectionData`. That will obviate the call to > `addExtraExportsAndOpens()` and save a few bytecodes during start-up (but the > overall impact would be small, so it's not critical in the current PR). > Because these tables use WeakReference, we need to wait for > [JDK-8354897](https://bugs.openjdk.org/browse/JDK-8354897). Fixed (1) above. > test/hotspot/jtreg/runtime/cds/appcds/jigsaw/modulepath/AddOpens.java line > 109: > >> 107: "--add-opens", addOpensArg, >> 108: "--module-path", moduleDir.toString(), >> 109: "-m", TEST_MODULE1, "with_add_opens") > > Could you add a test case for `=ALL-UNNAMED` as well? That is frequently used > by applications, and it takes a different code path inside the implementation. > > I think this can be done by running `com.simple.Main` in the classpath rather > than module path. Added the test case. - PR Review Comment: https://git.openjdk.org/jdk/pull/24695#discussion_r2053082635 PR Review Comment: https://git.openjdk.org/jdk/pull/24695#discussion_r2053082907
Re: RFR: 8352003: Support --add-opens with -XX:+AOTClassLinking [v4]
> This RFE allows --add-opens to be specified for AOT cache creation. AOT cache > can be used during production run with --add-opens option as long as the same > set of options is used during assembly phase. > > Passed tiers 1 - 4 testing. Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision: @iklam comment - Changes: - all: https://git.openjdk.org/jdk/pull/24695/files - new: https://git.openjdk.org/jdk/pull/24695/files/2ae685f8..95f2e9c6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24695&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24695&range=02-03 Stats: 35 lines in 2 files changed: 18 ins; 6 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/24695.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24695/head:pull/24695 PR: https://git.openjdk.org/jdk/pull/24695
Re: RFR: 8354343: Hardening of Currency tests for not yet defined future ISO 4217 currency [v2]
> Please review this PR which improves future currency checking for ISO 4217 > currencies. > > Checking for a currency that should not yet exist in the set of available > currencies is already done. > It should also be explicitly checked that such a currency can not be > instantiated as well via the String getter. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: move future currencies set up under setUpTestingData() - Changes: - all: https://git.openjdk.org/jdk/pull/24782/files - new: https://git.openjdk.org/jdk/pull/24782/files/7424e964..6c4cbe94 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24782&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24782&range=00-01 Stats: 3 lines in 1 file changed: 1 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24782.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24782/head:pull/24782 PR: https://git.openjdk.org/jdk/pull/24782
Re: RFR: 8354343: Hardening of Currency tests for not yet defined future ISO 4217 currency [v2]
On Mon, 21 Apr 2025 22:25:14 GMT, Naoto Sato wrote: >> Justin Lu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> move future currencies set up under setUpTestingData() > > test/jdk/java/util/Currency/ValidateISO4217.java line 183: > >> 181: setUpPatchedClasses(); >> 182: setUpTestingData(); >> 183: setUpNotYetDefined(); > > It may be clearer to move this inside `setUpTestingData()`, and modify the > comment there Right, makes more sense that way. Also updated the JBS issues as you requested. - PR Review Comment: https://git.openjdk.org/jdk/pull/24782#discussion_r2053093655
Re: RFR: 8354990: Improve negative tests coverage for jpackage signing [v2]
On Mon, 21 Apr 2025 21:34:59 GMT, Alexey Semenyuk wrote:
>> Add tests for the following test cases:
>> - Expired certificate specified for signing;
>> - Multiple certificates with the same name in one keychain.
>>
>> Adding the new tests revealed an issue with MacCertificate -
>> [JDK-8354989](https://bugs.openjdk.org/browse/JDK-8354989). This issue is
>> also addressed in this PR.
>>
>> Additionally:
>> - Moved code to verify signatures in MacSignVerify class and reworked
>> SigningBase verify functions to use MacSignVerify API.
>
> Alexey Semenyuk has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix compilation error
Looks good with minor comments.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 410:
> 408: continue;
> 409: }
> 410: certs.add(cert);
Maybe:
try {
final X509Certificate cert =
(X509Certificate)CERT_FACTORY.generateCertificate(in);
certs.add(cert);
} catch (Exception ex) {
TKit.trace("Failed to parse certificate data: " + ex);
}
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 691:
> 689: }
> 690:
> 691: public String value( ) {
Extra space between `()`.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 705:
> 703: }
> 704:
> 705: public record CertificateRequest(String name, CertificateType type,
> int days, boolean expired, boolean trusted) implements
> Comparable{
Space before `{`.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 733:
> 731: } else if (expired) {
> 732: return VerifyStatus.VERIFY_EXPIRED;
> 733: }else {
Formating.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 823:
> 821: }
> 822:
> 823: private record InstalledCertificate(String name, CertificateType
> type, int days, boolean expired) implements Comparable{
Space before `{`.
-
PR Review: https://git.openjdk.org/jdk/pull/24762#pullrequestreview-2782360440
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053070383
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053087453
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053087987
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053088479
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053089845
Re: RFR: 8354990: Improve negative tests coverage for jpackage signing [v2]
On Mon, 21 Apr 2025 23:02:33 GMT, Alexander Matveev
wrote:
>> Alexey Semenyuk has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix compilation error
>
> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 691:
>
>> 689: }
>> 690:
>> 691: public String value( ) {
>
> Extra space between `()`.
Fixed
> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 705:
>
>> 703: }
>> 704:
>> 705: public record CertificateRequest(String name, CertificateType type,
>> int days, boolean expired, boolean trusted) implements
>> Comparable{
>
> Space before `{`.
Fixed
> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 733:
>
>> 731: } else if (expired) {
>> 732: return VerifyStatus.VERIFY_EXPIRED;
>> 733: }else {
>
> Formating.
Fixed
> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 823:
>
>> 821: }
>> 822:
>> 823: private record InstalledCertificate(String name, CertificateType
>> type, int days, boolean expired) implements Comparable{
>
> Space before `{`.
Fixed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053134171
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053134256
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053134094
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053134012
Re: RFR: 8354990: Improve negative tests coverage for jpackage signing [v3]
> Add tests for the following test cases: > - Expired certificate specified for signing; > - Multiple certificates with the same name in one keychain. > > Adding the new tests revealed an issue with MacCertificate - > [JDK-8354989](https://bugs.openjdk.org/browse/JDK-8354989). This issue is > also addressed in this PR. > > Additionally: > - Moved code to verify signatures in MacSignVerify class and reworked > SigningBase verify functions to use MacSignVerify API. Alexey Semenyuk has updated the pull request incrementally with two additional commits since the last revision: - Fix formatting - Fix formatting - Changes: - all: https://git.openjdk.org/jdk/pull/24762/files - new: https://git.openjdk.org/jdk/pull/24762/files/95a45098..8630b47c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24762&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24762&range=01-02 Stats: 11 lines in 1 file changed: 2 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/24762.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24762/head:pull/24762 PR: https://git.openjdk.org/jdk/pull/24762
Re: RFR: 8354990: Improve negative tests coverage for jpackage signing [v3]
On Tue, 22 Apr 2025 00:14:17 GMT, Alexey Semenyuk wrote: >> Add tests for the following test cases: >> - Expired certificate specified for signing; >> - Multiple certificates with the same name in one keychain. >> >> Adding the new tests revealed an issue with MacCertificate - >> [JDK-8354989](https://bugs.openjdk.org/browse/JDK-8354989). This issue is >> also addressed in this PR. >> >> Additionally: >> - Moved code to verify signatures in MacSignVerify class and reworked >> SigningBase verify functions to use MacSignVerify API. > > Alexey Semenyuk has updated the pull request incrementally with two > additional commits since the last revision: > > - Fix formatting > - Fix formatting Marked as reviewed by almatvee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24762#pullrequestreview-2782457307
Re: RFR: 8354990: Improve negative tests coverage for jpackage signing [v2]
On Mon, 21 Apr 2025 22:36:08 GMT, Alexander Matveev
wrote:
>> Alexey Semenyuk has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix compilation error
>
> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 410:
>
>> 408: continue;
>> 409: }
>> 410: certs.add(cert);
>
> Maybe:
>
> try {
> final X509Certificate cert =
> (X509Certificate)CERT_FACTORY.generateCertificate(in);
> certs.add(cert);
> } catch (Exception ex) {
> TKit.trace("Failed to parse certificate data: " +
> ex);
> }
This will combine certificate creation and adding it to the list of
certificates. If the latter fails, this is not a `Failed to parse certificate
data` error.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053129886
Re: RFR: 8354990: Improve negative tests coverage for jpackage signing [v2]
On Tue, 22 Apr 2025 00:03:01 GMT, Alexey Semenyuk wrote:
>> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MacSign.java line 410:
>>
>>> 408: continue;
>>> 409: }
>>> 410: certs.add(cert);
>>
>> Maybe:
>>
>> try {
>> final X509Certificate cert =
>> (X509Certificate)CERT_FACTORY.generateCertificate(in);
>> certs.add(cert);
>> } catch (Exception ex) {
>> TKit.trace("Failed to parse certificate data: " +
>> ex);
>> }
>
> This will combine certificate creation and adding it to the list of
> certificates. If the latter fails, this is not a `Failed to parse certificate
> data` error.
I see. Agree.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24762#discussion_r2053136973
Integrated: 8354990: Improve negative tests coverage for jpackage signing
On Fri, 18 Apr 2025 21:11:35 GMT, Alexey Semenyuk wrote: > Add tests for the following test cases: > - Expired certificate specified for signing; > - Multiple certificates with the same name in one keychain. > > Adding the new tests revealed an issue with MacCertificate - > [JDK-8354989](https://bugs.openjdk.org/browse/JDK-8354989). This issue is > also addressed in this PR. > > Additionally: > - Moved code to verify signatures in MacSignVerify class and reworked > SigningBase verify functions to use MacSignVerify API. This pull request has now been integrated. Changeset: 47f78a75 Author:Alexey Semenyuk URL: https://git.openjdk.org/jdk/commit/47f78a7529a2b290a07394e053bcfaff4907b7e5 Stats: 957 lines in 7 files changed: 716 ins; 134 del; 107 mod 8354990: Improve negative tests coverage for jpackage signing 8354989: Bug in MacCertificate class Reviewed-by: almatvee - PR: https://git.openjdk.org/jdk/pull/24762
Re: RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v3]
> When jdeps was migrated from old classfile to ClassFile API, the parsing > semantic changed - error checks are now made lazily, and nested crashes from > malformed signature or other problems is now latent, after a `ClassModel` > instance is available. (The old error check existed only for constructing a > `ClassModel`) > > To address this issue, I have updated the way of iterating class files to be > handler/consumer based like in the ClassFile API. This has the advantage that > when one invocation of the handler fails of a `ClassFileError`, other > invocations for other class files can proceed, and the exception handler has > sufficient information to report a useful message indicating the source of > error. > > For the particular example of examining a proguard processed > `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`: > > Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected > character ; at position 59, expected an identifier: > Lscala/collection/immutable/TreeMap$TreeMapBuilder.;: > scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar) > Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected > character ; at position 49, expected an identifier: > Lscala/collection/parallel/mutable/ParArray.;: > scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar) > > > Now, jdeps shows the bad class files. Inspection into the files reveal that > proguard incorrectly deleted the simple class names with trailing `$`, for > example, the original signature of the broken ParArray was > `Lscala/collection/parallel/mutable/ParArray.ParArrayIterator$;`, so the > `ParArrayIterator$` part was incorrectly dropped by proguard. > > Testing: langtools/tools/jdeps. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Add a test - Changes: - all: https://git.openjdk.org/jdk/pull/24604/files - new: https://git.openjdk.org/jdk/pull/24604/files/7d2d5d0c..f61e9201 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24604&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24604&range=01-02 Stats: 169 lines in 5 files changed: 165 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/24604.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24604/head:pull/24604 PR: https://git.openjdk.org/jdk/pull/24604
Re: RFR: 8355177: Speed up StringBuilder::append(char[]) via UTF16::compress & Unsafe::copyMemory
On Mon, 21 Apr 2025 07:00:36 GMT, Shaojin Wen wrote:
> In BufferedReader.readLine and other similar scenarios, we need to use
> StringBuilder.append(char[]) to build the string.
>
> For these scenarios, we can use the intrinsic method StringUTF16.compress and
> Unsafe.copyMemory instead of the character copy of the char-by-char loop to
> improve the speed.
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1773:
> 1771: int compressed = StringUTF16.compress(s, off, val, count,
> end - off);
> 1772: count += compressed;
> 1773: off += compressed;
Should we update `this.count` eagerly after compression?
src/java.base/share/classes/java/lang/StringUTF16.java line 1317:
> 1315: }
> 1316:
> 1317: private static void putChars(byte[] val, int index, char[] str, int
> off, int end) {
I recommend renaming this to `putCharsUnchecked` to indicate this has no bound
checks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24773#discussion_r2052591347
PR Review Comment: https://git.openjdk.org/jdk/pull/24773#discussion_r2052597288
Re: RFR: 8077587: BigInteger Roots [v22]
> This PR implements nth root computation for `BigInteger`s using Newton method > and optimizes `BigInteger.pow(int)` method. > [Here is a proof of convergence of the recurrence > used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) fabioromano1 has updated the pull request incrementally with two additional commits since the last revision: - Merge remote-tracking branch 'origin/BigInteger-nth-root' into BigInteger-nth-root - Merge branch 'BigInteger-nth-root' of https://github.com/fabioromano1/jdk into BigInteger-nth-root - Changes: - all: https://git.openjdk.org/jdk/pull/24690/files - new: https://git.openjdk.org/jdk/pull/24690/files/8de6b825..100d0e14 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=21 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=20-21 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/24690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24690/head:pull/24690 PR: https://git.openjdk.org/jdk/pull/24690
Re: RFR: 8077587: BigInteger Roots [v23]
> This PR implements nth root computation for `BigInteger`s using Newton method > and optimizes `BigInteger.pow(int)` method. > [Here is a proof of convergence of the recurrence > used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) fabioromano1 has updated the pull request incrementally with one additional commit since the last revision: Suggested change - Changes: - all: https://git.openjdk.org/jdk/pull/24690/files - new: https://git.openjdk.org/jdk/pull/24690/files/100d0e14..1942fd10 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=22 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=21-22 Stats: 6 lines in 2 files changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/24690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24690/head:pull/24690 PR: https://git.openjdk.org/jdk/pull/24690
