Re: [jdk18] RFR: 8278574: update --help-extra message to include default value of --finalization option
On Thu, 16 Dec 2021 06:33:24 GMT, Stuart Marks wrote: > A small modification to the Launcher's help text. LGTM - Marked as reviewed by sundar (Reviewer). PR: https://git.openjdk.java.net/jdk18/pull/34
[jdk18] RFR: 8278574: update --help-extra message to include default value of --finalization option
A small modification to the Launcher's help text. - Commit messages: - Update launcher usage message to mention finalization default Changes: https://git.openjdk.java.net/jdk18/pull/34/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk18=34=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278574 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk18/pull/34.diff Fetch: git fetch https://git.openjdk.java.net/jdk18 pull/34/head:pull/34 PR: https://git.openjdk.java.net/jdk18/pull/34
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v7]
> BigInteger currently uses three different algorithms for multiply. The simple > quadratic algorithm, then the slightly better Karatsuba if we exceed a bit > count and then Toom Cook 3 once we go into the several thousands of bits. > Since Toom Cook 3 is a recursive algorithm, it is trivial to parallelize it. > I have demonstrated this several times in conference talks. In order to be > consistent with other classes such as Arrays and Collection, I have added a > parallelMultiply() method. Internally we have added a parameter to the > private multiply method to indicate whether the calculation should be done in > parallel. > > The performance improvements are as should be expected. Fibonacci of 100 > million (using a single-threaded Dijkstra's sum of squares version) completes > in 9.2 seconds with the parallelMultiply() vs 25.3 seconds with the > sequential multiply() method. This is on my 1-8-2 laptop. The final > multiplications are with very large numbers, which then benefit from the > parallelization of Toom-Cook 3. Fibonacci 100 million is a 347084 bit number. > > We have also parallelized the private square() method. Internally, the > square() method defaults to be sequential. > > Some benchmark results, run on my 1-6-2 server: > > > Benchmark (n) Mode Cnt Score > Error Units > BigIntegerParallelMultiply.multiply100ss4 51.707 > ± 11.194 ms/op > BigIntegerParallelMultiply.multiply 1000ss4988.302 > ± 235.977 ms/op > BigIntegerParallelMultiply.multiply 1ss4 24662.063 > ± 1123.329 ms/op > BigIntegerParallelMultiply.parallelMultiply100ss4 49.337 > ± 26.611 ms/op > BigIntegerParallelMultiply.parallelMultiply 1000ss4527.560 > ± 268.903 ms/op > BigIntegerParallelMultiply.parallelMultiply 1ss4 9076.551 > ± 1899.444 ms/op > > > We can see that for larger calculations (fib 100m), the execution is 2.7x > faster in parallel. For medium size (fib 10m) it is 1.873x faster. And for > small (fib 1m) it is roughly the same. Considering that the fibonacci > algorithm that we used was in itself sequential, and that the last 3 > calculations would dominate, 2.7x faster should probably be considered quite > good on a 1-6-2 machine. kabutz has updated the pull request incrementally with one additional commit since the last revision: Changed depth type to byte to save 8 bytes on each RecursiveSquare instance - Changes: - all: https://git.openjdk.java.net/jdk/pull/6409/files - new: https://git.openjdk.java.net/jdk/pull/6409/files/3cd16443..94c2d665 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6409=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6409=05-06 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6409.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6409/head:pull/6409 PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Thu, 16 Dec 2021 01:01:33 GMT, Paul Sandoz wrote: >> Inside the constructor would not work, since we do not construct RecursiveOp >> for all the tasks. However, I have incremented the parameter depth. I don't >> like changing parameters inside methods, but since I'm doing it where it is >> being used, I feel that the code is now better than before. Thanks for the >> suggestion. > > I am confused by "we do not construct RecursiveOp for all the tasks", since > each call to `RecursiveOp.multiply/square` constructs a new object that is an > instance of `RecursiveOp`. > > Your approach looks good. var v0_task = RecursiveOp.multiply(a0, b0, parallel, depth); // Here we make a new RecursiveOp for the multiply da1 = a2.add(a0); db1 = b2.add(b0); var vm1_task = RecursiveOp.multiply(da1.subtract(a1), db1.subtract(b1), parallel, depth); // Here also da1 = da1.add(a1); db1 = db1.add(b1); var v1_task = RecursiveOp.multiply(da1, db1, parallel, depth); // And here v2 = da1.add(a2).shiftLeft(1).subtract(a0).multiply( // Here we call multiply() directly, without the RecursiveOp db1.add(b2).shiftLeft(1).subtract(b0), true, parallel, depth); vinf = a2.multiply(b2, true, parallel, depth); // Again, we call multiply() directly v0 = v0_task.join(); vm1 = vm1_task.join(); v1 = v1_task.join(); - PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v6]
On Thu, 16 Dec 2021 01:33:46 GMT, Paul Sandoz wrote: > This is looking good. I will create the CSR and propose it. Since the holiday > season is imminent it's likely no approval of the CSR will happen on until > the new year. Thank you so much Paul. - PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v5]
On Mon, 15 Nov 2021 19:31:09 GMT, Raffaello Giulietti wrote: >> Hello, >> >> here's a PR for a patch submitted on March 2020 >> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was >> a thing. >> >> The patch has been edited to adhere to OpenJDK code conventions about >> multi-line (block) comments. Nothing in the code proper has changed, except >> for the addition of redundant but clarifying parentheses in some expressions. >> >> >> Greetings >> Raffaello > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 4511638: Double.toString(double) sometimes produces incorrect results > > Enhanced intervals in MathUtils. > Updated references to Schubfach v4. Has the paper been published? What is holding this back from merging? - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v6]
On Wed, 15 Dec 2021 20:18:39 GMT, kabutz wrote:
>> BigInteger currently uses three different algorithms for multiply. The
>> simple quadratic algorithm, then the slightly better Karatsuba if we exceed
>> a bit count and then Toom Cook 3 once we go into the several thousands of
>> bits. Since Toom Cook 3 is a recursive algorithm, it is trivial to
>> parallelize it. I have demonstrated this several times in conference talks.
>> In order to be consistent with other classes such as Arrays and Collection,
>> I have added a parallelMultiply() method. Internally we have added a
>> parameter to the private multiply method to indicate whether the calculation
>> should be done in parallel.
>>
>> The performance improvements are as should be expected. Fibonacci of 100
>> million (using a single-threaded Dijkstra's sum of squares version)
>> completes in 9.2 seconds with the parallelMultiply() vs 25.3 seconds with
>> the sequential multiply() method. This is on my 1-8-2 laptop. The final
>> multiplications are with very large numbers, which then benefit from the
>> parallelization of Toom-Cook 3. Fibonacci 100 million is a 347084 bit number.
>>
>> We have also parallelized the private square() method. Internally, the
>> square() method defaults to be sequential.
>>
>> Some benchmark results, run on my 1-6-2 server:
>>
>>
>> Benchmark (n) Mode Cnt Score
>> Error Units
>> BigIntegerParallelMultiply.multiply100ss4 51.707
>> ± 11.194 ms/op
>> BigIntegerParallelMultiply.multiply 1000ss4988.302
>> ± 235.977 ms/op
>> BigIntegerParallelMultiply.multiply 1ss4 24662.063
>> ± 1123.329 ms/op
>> BigIntegerParallelMultiply.parallelMultiply100ss4 49.337
>> ± 26.611 ms/op
>> BigIntegerParallelMultiply.parallelMultiply 1000ss4527.560
>> ± 268.903 ms/op
>> BigIntegerParallelMultiply.parallelMultiply 1ss4 9076.551
>> ± 1899.444 ms/op
>>
>>
>> We can see that for larger calculations (fib 100m), the execution is 2.7x
>> faster in parallel. For medium size (fib 10m) it is 1.873x faster. And for
>> small (fib 1m) it is roughly the same. Considering that the fibonacci
>> algorithm that we used was in itself sequential, and that the last 3
>> calculations would dominate, 2.7x faster should probably be considered quite
>> good on a 1-6-2 machine.
>
> kabutz has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - Made RecursiveOp fields package-private so that we do not need super.
> qualifiers in subclasses
> - Incremented depth once instead of on every call
> - Simplified depth calculation to rely on common pool parallelism or the
> current fork join pool executing the code.
> - Removed serialVersionUID and annotated class with
> @SuppressWarning("serial") instead
This is looking good. I will create the CSR and propose it. Since the holiday
season is imminent it's likely no approval of the CSR will happen on until the
new year.
-
PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 17 Nov 2021 20:04:50 GMT, Martin Balao wrote: >> Hi Martin, >> >> The change looks reasonable to me. >> I would suggest having a CSR logged for this change due to the following >> [behavioral >> incompatibility](https://wiki.openjdk.java.net/display/csr/Kinds+of+Compatibility): >> Before the change - all available endpoints/URLs are tried to create an LDAP >> context. >> With the proposed change - incorrect credentials will prevent other >> endpoints to be exercised to create an LDAP context. >> >> Having a CSR will also help to document difference in handling >> `AuthenticationException` and `NamingException` during construction of an >> LDAP context from the list of endpoints acquired from a LDAP DNS provider. > > Hi @AlekseiEfimov > > Can you please review the CSR [1]? > > Thanks, > Martin.- > > -- > [1] - https://bugs.openjdk.java.net/browse/JDK-8276959 > @martinuy This pull request has been inactive for more than 4 weeks and will > be automatically closed if another 4 weeks passes without any activity. To > avoid this, simply add a new comment to the pull request. Feel free to ask > for assistance if you need help with progressing this pull request towards > integration! Please do not close, waiting for CSR approval. - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Wed, 15 Dec 2021 14:31:26 GMT, kabutz wrote: >> src/java.base/share/classes/java/math/BigInteger.java line 2000: >> >>> 1998: da1 = a2.add(a0); >>> 1999: db1 = b2.add(b0); >>> 2000: var vm1_task = RecursiveOp.multiply(da1.subtract(a1), >>> db1.subtract(b1), parallel, depth + 1); >> >> I recommend incrementing the depth in the `RecursiveOp` constructor, thereby >> reducing the repetition. > > Inside the constructor would not work, since we do not construct RecursiveOp > for all the tasks. However, I have incremented the parameter depth. I don't > like changing parameters inside methods, but since I'm doing it where it is > being used, I feel that the code is now better than before. Thanks for the > suggestion. I am confused by "we do not construct RecursiveOp for all the tasks", since each call to `RecursiveOp.multiply/square` constructs a new object that is an instance of `RecursiveOp`. Your approach looks good. - PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Wed, 15 Dec 2021 14:40:34 GMT, kabutz wrote:
>> src/java.base/share/classes/java/math/BigInteger.java line 1909:
>>
>>> 1907: @Override
>>> 1908: public BigInteger compute() {
>>> 1909: return a.multiply(b, true, super.parallel,
>>> super.depth);
>>
>> Using `super.` is a little unusual, it's not wrong :-) but not really
>> idiomatic in the source code base.
>
> I cannot remember ever using `super.` like this before either :-) I have a
> static inner class, which in turn has two static inner classes, which are its
> subclasses. The inner inner classes want to access the fields of their outer
> static super class. Since the fields are private, we need to use `super`
> otherwise we get this compiler error:
>
> error: non-static variable parallel cannot be referenced from a static context
> return a.multiply(b, true, parallel, depth);
>
> And if we move the inner inner class up one level, then we also get an error:
>
> error: parallel has private access in RecursiveOp
> return a.multiply(b, true, parallel, super.depth);
>
> A solution of course is to make the fields non-private. Then the compiler is
> happy and we can get rid of the `super`. Since the classes are private
> anyway, making the fields private does not increase the encapsulation. I will
> do that to get rid of the `super` FWIW.
Oh, i did not realize that was a forced move.
-
PR: https://git.openjdk.java.net/jdk/pull/6409
Integrated: Merge jdk18
On Wed, 15 Dec 2021 23:18:45 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 18 -> JDK 19 This pull request has now been integrated. Changeset: e6b28e05 Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/e6b28e05c6f7698f230b04199932d4fc81f41a89 Stats: 341 lines in 27 files changed: 269 ins; 12 del; 60 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/6856
Re: RFR: Merge jdk18 [v2]
> Forwardport JDK 18 -> JDK 19 Jesper Wilhelmsson 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 53 additional commits since the last revision: - Merge - 8278020: ~13% variation in Renaissance-Scrabble Reviewed-by: dholmes, stuefe, kvn - 8274898: Cleanup usages of StringBuffer in jdk tools modules Reviewed-by: sspitsyn, lmesnik - 8269838: BasicTypeDataBase.findDynamicTypeForAddress(addr, basetype) can be simplified Reviewed-by: kevinw, sspitsyn - 8278186: org.jcp.xml.dsig.internal.dom.Utils.parseIdFromSameDocumentURI throws StringIndexOutOfBoundsException when calling substring method Reviewed-by: mullan - 8278241: Implement JVM SpinPause on linux-aarch64 Reviewed-by: aph, phh - 8278842: Parallel: Remove unused VerifyObjectStartArrayClosure::_old_gen Reviewed-by: tschatzl - 8278548: G1: Remove unnecessary check in forward_to_block_containing_addr Reviewed-by: tschatzl, mli, sjohanss - 8202579: Revisit VM_Version and VM_Version_ext for overlap and consolidation Reviewed-by: dholmes, hseigel - 8278351: Add function to retrieve worker_id from any context Reviewed-by: eosterlund, kbarrett, ayang - ... and 43 more: https://git.openjdk.java.net/jdk/compare/6d63c6dd...fa3d80e6 - Changes: - all: https://git.openjdk.java.net/jdk/pull/6856/files - new: https://git.openjdk.java.net/jdk/pull/6856/files/fa3d80e6..fa3d80e6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6856=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6856=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6856.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6856/head:pull/6856 PR: https://git.openjdk.java.net/jdk/pull/6856
RFR: Merge jdk18
Forwardport JDK 18 -> JDK 19 - Commit messages: - Merge - 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation - 8272064: test/jdk/jdk/jfr/api/consumer/TestHiddenMethod.java needs update for JEP 416 - 8278607: Misc issues in foreign API javadoc - 8278233: [macos] tools/jpackage tests timeout due to /usr/bin/osascript - 8278758: runtime/BootstrapMethod/BSMCalledTwice.java fails with release VMs after JDK-8262134 - 8278744: KeyStore:getAttributes() not returning unmodifiable Set - 8277919: OldObjectSample event causing bloat in the class constant pool in JFR recording - 8262134: compiler/uncommontrap/TestDeoptOOM.java failed with "guarantee(false) failed: wrong number of expression stack elements during deopt" The merge commit only contains trivial merges, so no merge-specific webrevs have been generated. Changes: https://git.openjdk.java.net/jdk/pull/6856/files Stats: 341 lines in 27 files changed: 269 ins; 12 del; 60 mod Patch: https://git.openjdk.java.net/jdk/pull/6856.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6856/head:pull/6856 PR: https://git.openjdk.java.net/jdk/pull/6856
Re: RFR: 8278642: Refactor java.util.Formatter
On Wed, 15 Dec 2021 20:09:22 GMT, Roger Riggs wrote: >> A few refactorings to how `java.util.Formatter` sets up `FormatString`s, >> aligning the implementation with changes explored by the TemplatedStrings >> JEP and ever so slightly improving performance: >> >> - turn `Flags` into an `int` (fewer allocations in the complex case) >> - remove some superfluous varargs: `checkBadFlags(Flags.PARENTHESES, ...` -> >> `checkBadFlags(Flags.Parentheses | ...` - again less allocations in the >> complex cases since these varargs arrays were being allocated. Also improves >> error messages since all bad flags will be listed in the exception message. >> - make `FormatSpecifier` and `FixedString` static, reducing size of these >> objects slightly. >> >> Baseline: >> >> Benchmark Mode >> Cnt Score Error Units >> StringFormat.complexFormat avgt >> 25 8977.043 ± 246.810 ns/op >> StringFormat.complexFormat:·gc.alloc.rate.norm avgt >> 25 2144.170 ± 0.012B/op >> StringFormat.stringFormat avgt >> 25 252.109 ± 2.732 ns/op >> StringFormat.stringFormat:·gc.alloc.rate.norm avgt >> 25 256.019 ± 0.001B/op >> StringFormat.stringIntFormatavgt >> 25 576.423 ± 4.596 ns/op >> StringFormat.stringIntFormat:·gc.alloc.rate.normavgt >> 25 432.034 ± 0.002B/op >> StringFormat.widthStringFormat avgt >> 25 999.835 ± 20.127 ns/op >> StringFormat.widthStringFormat:·gc.alloc.rate.norm avgt >> 25 525.509 ± 14.742B/op >> StringFormat.widthStringIntFormat avgt >> 25 1332.163 ± 30.901 ns/op >> StringFormat.widthStringIntFormat:·gc.alloc.rate.norm avgt >> 25 720.715 ± 8.798B/op >> >> >> Patch: >> >> Benchmark Mode >> Cnt ScoreError Units >> StringFormat.complexFormat avgt >> 25 8840.089 ± 51.222 ns/op >> StringFormat.complexFormat:·gc.alloc.rate.norm avgt >> 25 1736.151 ± 0.009B/op >> StringFormat.stringFormat avgt >> 25 247.310 ± 2.091 ns/op >> StringFormat.stringFormat:·gc.alloc.rate.norm avgt >> 25 248.018 ± 0.001B/op >> StringFormat.stringIntFormatavgt >> 25 565.487 ± 6.572 ns/op >> StringFormat.stringIntFormat:·gc.alloc.rate.normavgt >> 25 408.032 ± 0.002B >> StringFormat.widthStringFormat avgt >> 25 970.015 ± 32.915 ns/op >> StringFormat.widthStringFormat:·gc.alloc.rate.norm avgt >> 25 449.991 ± 25.716B/op >> StringFormat.widthStringIntFormat avgt >> 25 1284.572 ± 28.829 ns/op >> StringFormat.widthStringIntFormat:·gc.alloc.rate.norm avgt >> 25 636.872 ± 7.331B/op > > Looks good to me. Not a big improvement, but as much as it gets used. Thanks for reviews, @RogerRiggs and @naotoj ! Yes, the speed-up is a bit underwhelming. I had to dial back some more promising optimizations I was exploring (#6792) after realizing they'd be breaking semantics in case of an illegal format string. These are the cleanups and pile-on optimizations I could salvage from that experiment, along with some harmonization with changes @JimLaskey is doing for TemplatedStrings. - PR: https://git.openjdk.java.net/jdk/pull/6821
[jdk18] Integrated: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation
On Wed, 15 Dec 2021 05:59:45 GMT, Vladimir Kozlov wrote: > A proper fix for this is to use the catchException combination. However, that > introduces significant cold startup performance regression. JDK-8278447 > tracks the work to address the performance regression using catchException > and asSpreader combinator. It may require significant work and refactoring > which is risky for JDK 18. > > It is proposed to implement a workaround in C2 to white list the relevant > methods (all methods in sun.invoke.util.ValueConversions class) not to omit > stack trace when exception is thrown in them. > > Added new regression test. Tested tier1-3. This pull request has now been integrated. Changeset: d3408a46 Author:Vladimir Kozlov URL: https://git.openjdk.java.net/jdk18/commit/d3408a46b7c8c2f8b5e41f3e286a497064a2c104 Stats: 149 lines in 7 files changed: 147 ins; 1 del; 1 mod 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation Reviewed-by: dlong, mchung, dholmes - PR: https://git.openjdk.java.net/jdk18/pull/27
Re: [jdk18] RFR: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation [v2]
On Wed, 15 Dec 2021 17:56:23 GMT, Vladimir Kozlov wrote: >> A proper fix for this is to use the catchException combination. However, >> that introduces significant cold startup performance regression. JDK-8278447 >> tracks the work to address the performance regression using catchException >> and asSpreader combinator. It may require significant work and refactoring >> which is risky for JDK 18. >> >> It is proposed to implement a workaround in C2 to white list the relevant >> methods (all methods in sun.invoke.util.ValueConversions class) not to omit >> stack trace when exception is thrown in them. >> >> Added new regression test. Tested tier1-3. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Removed boot classloader check Thank you, David, Dean and Mandy for reviews. - PR: https://git.openjdk.java.net/jdk18/pull/27
Re: RFR: 8278642: Refactor java.util.Formatter
On Tue, 14 Dec 2021 00:14:32 GMT, Claes Redestad wrote: > A few refactorings to how `java.util.Formatter` sets up `FormatString`s, > aligning the implementation with changes explored by the TemplatedStrings JEP > and ever so slightly improving performance: > > - turn `Flags` into an `int` (fewer allocations in the complex case) > - remove some superfluous varargs: `checkBadFlags(Flags.PARENTHESES, ...` -> > `checkBadFlags(Flags.Parentheses | ...` - again less allocations in the > complex cases since these varargs arrays were being allocated. Also improves > error messages since all bad flags will be listed in the exception message. > - make `FormatSpecifier` and `FixedString` static, reducing size of these > objects slightly. > > Baseline: > > Benchmark Mode Cnt > Score Error Units > StringFormat.complexFormat avgt 25 > 8977.043 ± 246.810 ns/op > StringFormat.complexFormat:·gc.alloc.rate.norm avgt 25 > 2144.170 ± 0.012B/op > StringFormat.stringFormat avgt 25 > 252.109 ± 2.732 ns/op > StringFormat.stringFormat:·gc.alloc.rate.norm avgt 25 > 256.019 ± 0.001B/op > StringFormat.stringIntFormatavgt 25 > 576.423 ± 4.596 ns/op > StringFormat.stringIntFormat:·gc.alloc.rate.normavgt 25 > 432.034 ± 0.002B/op > StringFormat.widthStringFormat avgt 25 > 999.835 ± 20.127 ns/op > StringFormat.widthStringFormat:·gc.alloc.rate.norm avgt 25 > 525.509 ± 14.742B/op > StringFormat.widthStringIntFormat avgt 25 > 1332.163 ± 30.901 ns/op > StringFormat.widthStringIntFormat:·gc.alloc.rate.norm avgt 25 > 720.715 ± 8.798B/op > > > Patch: > > Benchmark Mode Cnt > ScoreError Units > StringFormat.complexFormat avgt 25 > 8840.089 ± 51.222 ns/op > StringFormat.complexFormat:·gc.alloc.rate.norm avgt 25 > 1736.151 ± 0.009B/op > StringFormat.stringFormat avgt 25 > 247.310 ± 2.091 ns/op > StringFormat.stringFormat:·gc.alloc.rate.norm avgt 25 > 248.018 ± 0.001B/op > StringFormat.stringIntFormatavgt 25 > 565.487 ± 6.572 ns/op > StringFormat.stringIntFormat:·gc.alloc.rate.normavgt 25 > 408.032 ± 0.002B > StringFormat.widthStringFormat avgt 25 > 970.015 ± 32.915 ns/op > StringFormat.widthStringFormat:·gc.alloc.rate.norm avgt 25 > 449.991 ± 25.716B/op > StringFormat.widthStringIntFormat avgt 25 > 1284.572 ± 28.829 ns/op > StringFormat.widthStringIntFormat:·gc.alloc.rate.norm avgt 25 > 636.872 ± 7.331B/op LGTM. I'd be tempted to replace those `instanceof`s with pattern-match to get rid of castings, but that's not inherent to this issue. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6821
Re: [jdk18] RFR: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation [v2]
On Wed, 15 Dec 2021 17:56:23 GMT, Vladimir Kozlov wrote: >> A proper fix for this is to use the catchException combination. However, >> that introduces significant cold startup performance regression. JDK-8278447 >> tracks the work to address the performance regression using catchException >> and asSpreader combinator. It may require significant work and refactoring >> which is risky for JDK 18. >> >> It is proposed to implement a workaround in C2 to white list the relevant >> methods (all methods in sun.invoke.util.ValueConversions class) not to omit >> stack trace when exception is thrown in them. >> >> Added new regression test. Tested tier1-3. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Removed boot classloader check Looks good. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk18/pull/27
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v6]
> BigInteger currently uses three different algorithms for multiply. The simple
> quadratic algorithm, then the slightly better Karatsuba if we exceed a bit
> count and then Toom Cook 3 once we go into the several thousands of bits.
> Since Toom Cook 3 is a recursive algorithm, it is trivial to parallelize it.
> I have demonstrated this several times in conference talks. In order to be
> consistent with other classes such as Arrays and Collection, I have added a
> parallelMultiply() method. Internally we have added a parameter to the
> private multiply method to indicate whether the calculation should be done in
> parallel.
>
> The performance improvements are as should be expected. Fibonacci of 100
> million (using a single-threaded Dijkstra's sum of squares version) completes
> in 9.2 seconds with the parallelMultiply() vs 25.3 seconds with the
> sequential multiply() method. This is on my 1-8-2 laptop. The final
> multiplications are with very large numbers, which then benefit from the
> parallelization of Toom-Cook 3. Fibonacci 100 million is a 347084 bit number.
>
> We have also parallelized the private square() method. Internally, the
> square() method defaults to be sequential.
>
> Some benchmark results, run on my 1-6-2 server:
>
>
> Benchmark (n) Mode Cnt Score
> Error Units
> BigIntegerParallelMultiply.multiply100ss4 51.707
> ± 11.194 ms/op
> BigIntegerParallelMultiply.multiply 1000ss4988.302
> ± 235.977 ms/op
> BigIntegerParallelMultiply.multiply 1ss4 24662.063
> ± 1123.329 ms/op
> BigIntegerParallelMultiply.parallelMultiply100ss4 49.337
> ± 26.611 ms/op
> BigIntegerParallelMultiply.parallelMultiply 1000ss4527.560
> ± 268.903 ms/op
> BigIntegerParallelMultiply.parallelMultiply 1ss4 9076.551
> ± 1899.444 ms/op
>
>
> We can see that for larger calculations (fib 100m), the execution is 2.7x
> faster in parallel. For medium size (fib 10m) it is 1.873x faster. And for
> small (fib 1m) it is roughly the same. Considering that the fibonacci
> algorithm that we used was in itself sequential, and that the last 3
> calculations would dominate, 2.7x faster should probably be considered quite
> good on a 1-6-2 machine.
kabutz has updated the pull request incrementally with four additional commits
since the last revision:
- Made RecursiveOp fields package-private so that we do not need super.
qualifiers in subclasses
- Incremented depth once instead of on every call
- Simplified depth calculation to rely on common pool parallelism or the
current fork join pool executing the code.
- Removed serialVersionUID and annotated class with @SuppressWarning("serial")
instead
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/6409/files
- new: https://git.openjdk.java.net/jdk/pull/6409/files/59de5298..3cd16443
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=6409=05
- incr: https://webrevs.openjdk.java.net/?repo=jdk=6409=04-05
Stats: 54 lines in 1 file changed: 17 ins; 16 del; 21 mod
Patch: https://git.openjdk.java.net/jdk/pull/6409.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6409/head:pull/6409
PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Wed, 24 Nov 2021 15:20:42 GMT, kabutz wrote:
>> BigInteger currently uses three different algorithms for multiply. The
>> simple quadratic algorithm, then the slightly better Karatsuba if we exceed
>> a bit count and then Toom Cook 3 once we go into the several thousands of
>> bits. Since Toom Cook 3 is a recursive algorithm, it is trivial to
>> parallelize it. I have demonstrated this several times in conference talks.
>> In order to be consistent with other classes such as Arrays and Collection,
>> I have added a parallelMultiply() method. Internally we have added a
>> parameter to the private multiply method to indicate whether the calculation
>> should be done in parallel.
>>
>> The performance improvements are as should be expected. Fibonacci of 100
>> million (using a single-threaded Dijkstra's sum of squares version)
>> completes in 9.2 seconds with the parallelMultiply() vs 25.3 seconds with
>> the sequential multiply() method. This is on my 1-8-2 laptop. The final
>> multiplications are with very large numbers, which then benefit from the
>> parallelization of Toom-Cook 3. Fibonacci 100 million is a 347084 bit number.
>>
>> We have also parallelized the private square() method. Internally, the
>> square() method defaults to be sequential.
>>
>> Some benchmark results, run on my 1-6-2 server:
>>
>>
>> Benchmark (n) Mode Cnt Score
>> Error Units
>> BigIntegerParallelMultiply.multiply100ss4 51.707
>> ± 11.194 ms/op
>> BigIntegerParallelMultiply.multiply 1000ss4988.302
>> ± 235.977 ms/op
>> BigIntegerParallelMultiply.multiply 1ss4 24662.063
>> ± 1123.329 ms/op
>> BigIntegerParallelMultiply.parallelMultiply100ss4 49.337
>> ± 26.611 ms/op
>> BigIntegerParallelMultiply.parallelMultiply 1000ss4527.560
>> ± 268.903 ms/op
>> BigIntegerParallelMultiply.parallelMultiply 1ss4 9076.551
>> ± 1899.444 ms/op
>>
>>
>> We can see that for larger calculations (fib 100m), the execution is 2.7x
>> faster in parallel. For medium size (fib 10m) it is 1.873x faster. And for
>> small (fib 1m) it is roughly the same. Considering that the fibonacci
>> algorithm that we used was in itself sequential, and that the last 3
>> calculations would dominate, 2.7x faster should probably be considered quite
>> good on a 1-6-2 machine.
>
> kabutz has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Made forkOrInvoke() method protected to avoid strange compiler error
> IIRC you are not overly concerned about the additional object creation of
> `RecursiveOp` instances?
>
> If that changes the operation method could return `Object` and choose to
> perform the operation directly returning the `BigInteger` result or returning
> the particular `RecursiveOp`.
>
> ```java
> private static Object multiply(BigInteger a, BigInteger b, boolean parallel,
> int depth) {
> if (isParallel(parallel, depth)) {
> return new RecursiveMultiply(a, b, parallel, depth).fork();
> } else {
> // Also called by RecursiveMultiply.compute()
> return RecursiveMultiply.compute(a, b, false, depth);
> }
> }
> ```
>
> Then we could have another method on `RecursiveOp` that pattern matches e.g.:
>
> ```java
> static BigInteger join(Object o) {
> // replace with pattern match switch when it exits preview
> if (o instanceof BigInteger b) {
> return b;
> } else if (o instanceof RecursiveOp r) {
> return r.join();
> } else {
> throw new InternalError("Cannot reach here);
> }
> }
> ```
>
> That seems simple enough it might be worth doing anyway.
If we need a demonstration as to why pattern matching switch is necessary, then
I guess we could do this. Each RecursiveOp instance is 40 bytes. To calculate
Fibonacci (1_000_000_000) we create 7_324_218 tasks, thus we are allocating an
additional 292_968_720 bytes of memory. In addition, I believe that calling
invoke() allocates some more bytes. According to the GC logs, we allocate an
additional 2.33 GB of memory. That might sound like a lot, but it takes 2.25 TB
to calculate Fibonacci of 1 billion using our algorithm. The additional memory
allocated is thus roughly 0.1%. The performance of the old sequential multiply
and the new one, with the additional object creation, seems equivalent.
I would thus recommend that we keep it the way it is at the moment, with the
new RecursiveOp task creation. Considering the volume of objects that we will
be allocating once we get to Toom Cook 3, a 0.1% reduction in object allocation
will not be noticed.
Old BigInteger#multiply()
Fibonacci memory bytes
100 11.5KB 11808
1k 119.0KB 121856
10k1.2MB1238552
100k 13.0MB 13634608
Re: RFR: 8278642: Refactor java.util.Formatter
On Tue, 14 Dec 2021 00:14:32 GMT, Claes Redestad wrote: > A few refactorings to how `java.util.Formatter` sets up `FormatString`s, > aligning the implementation with changes explored by the TemplatedStrings JEP > and ever so slightly improving performance: > > - turn `Flags` into an `int` (fewer allocations in the complex case) > - remove some superfluous varargs: `checkBadFlags(Flags.PARENTHESES, ...` -> > `checkBadFlags(Flags.Parentheses | ...` - again less allocations in the > complex cases since these varargs arrays were being allocated. Also improves > error messages since all bad flags will be listed in the exception message. > - make `FormatSpecifier` and `FixedString` static, reducing size of these > objects slightly. > > Baseline: > > Benchmark Mode Cnt > Score Error Units > StringFormat.complexFormat avgt 25 > 8977.043 ± 246.810 ns/op > StringFormat.complexFormat:·gc.alloc.rate.norm avgt 25 > 2144.170 ± 0.012B/op > StringFormat.stringFormat avgt 25 > 252.109 ± 2.732 ns/op > StringFormat.stringFormat:·gc.alloc.rate.norm avgt 25 > 256.019 ± 0.001B/op > StringFormat.stringIntFormatavgt 25 > 576.423 ± 4.596 ns/op > StringFormat.stringIntFormat:·gc.alloc.rate.normavgt 25 > 432.034 ± 0.002B/op > StringFormat.widthStringFormat avgt 25 > 999.835 ± 20.127 ns/op > StringFormat.widthStringFormat:·gc.alloc.rate.norm avgt 25 > 525.509 ± 14.742B/op > StringFormat.widthStringIntFormat avgt 25 > 1332.163 ± 30.901 ns/op > StringFormat.widthStringIntFormat:·gc.alloc.rate.norm avgt 25 > 720.715 ± 8.798B/op > > > Patch: > > Benchmark Mode Cnt > ScoreError Units > StringFormat.complexFormat avgt 25 > 8840.089 ± 51.222 ns/op > StringFormat.complexFormat:·gc.alloc.rate.norm avgt 25 > 1736.151 ± 0.009B/op > StringFormat.stringFormat avgt 25 > 247.310 ± 2.091 ns/op > StringFormat.stringFormat:·gc.alloc.rate.norm avgt 25 > 248.018 ± 0.001B/op > StringFormat.stringIntFormatavgt 25 > 565.487 ± 6.572 ns/op > StringFormat.stringIntFormat:·gc.alloc.rate.normavgt 25 > 408.032 ± 0.002B > StringFormat.widthStringFormat avgt 25 > 970.015 ± 32.915 ns/op > StringFormat.widthStringFormat:·gc.alloc.rate.norm avgt 25 > 449.991 ± 25.716B/op > StringFormat.widthStringIntFormat avgt 25 > 1284.572 ± 28.829 ns/op > StringFormat.widthStringIntFormat:·gc.alloc.rate.norm avgt 25 > 636.872 ± 7.331B/op Looks good to me. Not a big improvement, but as much as it gets used. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6821
Re: [jdk18] RFR: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation [v2]
On Wed, 15 Dec 2021 17:56:23 GMT, Vladimir Kozlov wrote: >> A proper fix for this is to use the catchException combination. However, >> that introduces significant cold startup performance regression. JDK-8278447 >> tracks the work to address the performance regression using catchException >> and asSpreader combinator. It may require significant work and refactoring >> which is risky for JDK 18. >> >> It is proposed to implement a workaround in C2 to white list the relevant >> methods (all methods in sun.invoke.util.ValueConversions class) not to omit >> stack trace when exception is thrown in them. >> >> Added new regression test. Tested tier1-3. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Removed boot classloader check Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk18/pull/27
Re: [jdk18] RFR: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation [v2]
> A proper fix for this is to use the catchException combination. However, that > introduces significant cold startup performance regression. JDK-8278447 > tracks the work to address the performance regression using catchException > and asSpreader combinator. It may require significant work and refactoring > which is risky for JDK 18. > > It is proposed to implement a workaround in C2 to white list the relevant > methods (all methods in sun.invoke.util.ValueConversions class) not to omit > stack trace when exception is thrown in them. > > Added new regression test. Tested tier1-3. Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision: Removed boot classloader check - Changes: - all: https://git.openjdk.java.net/jdk18/pull/27/files - new: https://git.openjdk.java.net/jdk18/pull/27/files/3c23350d..5b6805b6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk18=27=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk18=27=00-01 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk18/pull/27.diff Fetch: git fetch https://git.openjdk.java.net/jdk18 pull/27/head:pull/27 PR: https://git.openjdk.java.net/jdk18/pull/27
Re: RFR: 8278518: String(byte[], int, int, Charset) constructor and String.translateEscapes() miss bounds check elimination
On Wed, 15 Dec 2021 08:29:20 GMT, Сергей Цыпанов wrote: >>> @AlanBateman the benchmark is mine along with the changes for >>> `translateEscapes` and `newStringUTF8NoRepl`, the change for constructor is >>> from SO. >> >> I don't know how we can progress this PR if the patch includes code copied >> from SO. Maybe the PR should be closed, the JBS issue unassigned, and leave >> it to someone else to start again? Maybe you could get Amit to sign the OCA >> and you co-contribute/author the PR? I can't look at the patch so I don't >> know how significant the changes, maybe there are other options. > > @AlanBateman I suggest to decide first whether this should be fixed on > HotSpot level (which is the best option to me) or on Java level. > > If we choose HotSpot then the issue should be reassigned, because I cannot do > the fix myself. > > If we choose Java then I would revert the change for String constructor and > Amir would commit it into this branch having OCA signed. > > What do you think? @stsypanov I just looked at the generated code and make a guess, which may be not entirely true though. I think you could ask hotspot compiler mailing list for more profound analysis. - PR: https://git.openjdk.java.net/jdk/pull/6812
Re: [jdk18] RFR: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation
On Wed, 15 Dec 2021 17:19:25 GMT, Mandy Chung wrote: >> @dholmes-ora, thank you for looking on it. >> I discussed it with Mandy and agreed that we need to narrow down this >> workaround as much as possible. That is why it is done only for system class >> loaded by null loader. > > David has a good observation. There will be no split package for modules. > So sun.invoke.util classes will only be loaded from java.base. The boot > loader is not needed. Okay, I will remove it. - PR: https://git.openjdk.java.net/jdk18/pull/27
Re: [jdk18] RFR: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation
On Wed, 15 Dec 2021 17:15:46 GMT, Vladimir Kozlov wrote:
>> src/hotspot/share/oops/method.cpp line 827:
>>
>>> 825: */
>>> 826: bool Method::can_omit_stack_trace() {
>>> 827: if
>>> (method_holder()->class_loader_data()->is_boot_class_loader_data()) {
>>
>> Do you actually need to check this?
>
> @dholmes-ora, thank you for looking on it.
> I discussed it with Mandy and agreed that we need to narrow down this
> workaround as much as possible. That is why it is done only for system class
> loaded by null loader.
David has a good observation. There will be no split package for modules. So
sun.invoke.util classes will only be loaded from java.base. The boot loader is
not needed.
-
PR: https://git.openjdk.java.net/jdk18/pull/27
Re: [jdk18] RFR: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation
On Wed, 15 Dec 2021 07:00:17 GMT, David Holmes wrote:
>> A proper fix for this is to use the catchException combination. However,
>> that introduces significant cold startup performance regression. JDK-8278447
>> tracks the work to address the performance regression using catchException
>> and asSpreader combinator. It may require significant work and refactoring
>> which is risky for JDK 18.
>>
>> It is proposed to implement a workaround in C2 to white list the relevant
>> methods (all methods in sun.invoke.util.ValueConversions class) not to omit
>> stack trace when exception is thrown in them.
>>
>> Added new regression test. Tested tier1-3.
>
> src/hotspot/share/oops/method.cpp line 827:
>
>> 825: */
>> 826: bool Method::can_omit_stack_trace() {
>> 827: if
>> (method_holder()->class_loader_data()->is_boot_class_loader_data()) {
>
> Do you actually need to check this?
@dholmes-ora, thank you for looking on it.
I discussed it with Mandy and agreed that we need to narrow down this
workaround as much as possible. That is why it is done only for system class
loaded by null loader.
-
PR: https://git.openjdk.java.net/jdk18/pull/27
Re: [jdk18] RFR: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation
On Wed, 15 Dec 2021 05:59:45 GMT, Vladimir Kozlov wrote: > A proper fix for this is to use the catchException combination. However, that > introduces significant cold startup performance regression. JDK-8278447 > tracks the work to address the performance regression using catchException > and asSpreader combinator. It may require significant work and refactoring > which is risky for JDK 18. > > It is proposed to implement a workaround in C2 to white list the relevant > methods (all methods in sun.invoke.util.ValueConversions class) not to omit > stack trace when exception is thrown in them. > > Added new regression test. Tested tier1-3. Looks good. Thanks for taking this on. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk18/pull/27
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Tue, 30 Nov 2021 23:18:21 GMT, Paul Sandoz wrote:
>> kabutz has updated the pull request incrementally with one additional commit
>> since the last revision:
>>
>> Made forkOrInvoke() method protected to avoid strange compiler error
>
> src/java.base/share/classes/java/math/BigInteger.java line 1909:
>
>> 1907: @Override
>> 1908: public BigInteger compute() {
>> 1909: return a.multiply(b, true, super.parallel,
>> super.depth);
>
> Using `super.` is a little unusual, it's not wrong :-) but not really
> idiomatic in the source code base.
I cannot remember ever using `super.` like this before either :-) I have a
static inner class, which in turn has two static inner classes, which are its
subclasses. The inner inner classes want to access the fields of their outer
static super class. Since the fields are private, we need to use `super`
otherwise we get this compiler error:
error: non-static variable parallel cannot be referenced from a static context
return a.multiply(b, true, parallel, depth);
And if we move the inner inner class up one level, then we also get an error:
error: parallel has private access in RecursiveOp
return a.multiply(b, true, parallel, super.depth);
A solution of course is to make the fields non-private. Then the compiler is
happy and we can get rid of the `super`. Since the classes are private anyway,
making the fields private does not increase the encapsulation. I will do that
to get rid of the `super` FWIW.
-
PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Tue, 30 Nov 2021 23:19:37 GMT, Paul Sandoz wrote: >> kabutz has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Made forkOrInvoke() method protected to avoid strange compiler error > > src/java.base/share/classes/java/math/BigInteger.java line 2000: > >> 1998: da1 = a2.add(a0); >> 1999: db1 = b2.add(b0); >> 2000: var vm1_task = RecursiveOp.multiply(da1.subtract(a1), >> db1.subtract(b1), parallel, depth + 1); > > I recommend incrementing the depth in the `RecursiveOp` constructor, thereby > reducing the repetition. Inside the constructor would not work, since we do not construct RecursiveOp for all the tasks. However, I have incremented the parameter depth. I don't like changing parameters inside methods, but since I'm doing it where it is being used, I feel that the code is now better than before. Thanks for the suggestion. - PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Tue, 30 Nov 2021 23:45:02 GMT, Paul Sandoz wrote:
>> kabutz has updated the pull request incrementally with one additional commit
>> since the last revision:
>>
>> Made forkOrInvoke() method protected to avoid strange compiler error
>
> src/java.base/share/classes/java/math/BigInteger.java line 1930:
>
>> 1928: }
>> 1929:
>> 1930: private static RecursiveTask exec(RecursiveOp op) {
>
> Unused.
Well spotted, thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/6409
[jdk18] Integrated: 8278607: Misc issues in foreign API javadoc
On Mon, 13 Dec 2021 21:13:20 GMT, Maurizio Cimadamore wrote: > This patch fixes a number of issues and typos in the foreign API javadoc > following another internal round of reviews. This pull request has now been integrated. Changeset: d6b5544e Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk18/commit/d6b5544e74d46c1ca464a1994e73ddd323ef5c2b Stats: 56 lines in 7 files changed: 16 ins; 1 del; 39 mod 8278607: Misc issues in foreign API javadoc Reviewed-by: sundar - PR: https://git.openjdk.java.net/jdk18/pull/17
Re: [jdk18] RFR: 8278607: Misc issues in foreign API javadoc [v4]
On Tue, 14 Dec 2021 10:24:07 GMT, Maurizio Cimadamore wrote: >> This patch fixes a number of issues and typos in the foreign API javadoc >> following another internal round of reviews. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typo as per review comment. Improve javadoc for VaList. LGTM - Marked as reviewed by sundar (Reviewer). PR: https://git.openjdk.java.net/jdk18/pull/17
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Tue, 30 Nov 2021 23:16:20 GMT, Paul Sandoz wrote: >> kabutz has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Made forkOrInvoke() method protected to avoid strange compiler error > > src/java.base/share/classes/java/math/BigInteger.java line 1875: > >> 1873: private static final int PARALLEL_FORK_THRESHOLD = >> Integer.getInteger( >> 1874: "java.math.BigInteger.parallelForkThreshold", >> 1875: (int) >> Math.ceil(Math.log(Runtime.getRuntime().availableProcessors()) / >> Math.log(2))); > > We can simplify to `32 - > Integer.numberOfLeadingZeros(ForkJoinPool.getCommonPoolParallelism() - 1))`. > > `ForkJoinPool.getCommonPoolParallelism()` is guaranteed to return a value `> > 0` `ForkJoinPool.getCommonPoolParallelism() ` cannot distinguish between 0, 1, and 2 active processors. In the `java.util.stream.AbstractTask` class they calculate the number of leaves based on this with << 2. However, there is always the possibility that someone launches the parallel stream inside another FJP, so they check for that as well. It does complicate things a bit, but at the same time, means we have one less environment variable. And if it becomes an issue (which I doubt), then we can always add it later. Will work on this a bit. - PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Tue, 30 Nov 2021 23:10:39 GMT, Paul Sandoz wrote: >> kabutz has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Made forkOrInvoke() method protected to avoid strange compiler error > > src/java.base/share/classes/java/math/BigInteger.java line 1874: > >> 1872: */ >> 1873: private static final int PARALLEL_FORK_THRESHOLD = >> Integer.getInteger( >> 1874: "java.math.BigInteger.parallelForkThreshold", > > I suspect we don't need this system property, there is no such property for > streams. We use `ForkJoinPool.getCommonPoolParallelism()`, which is > configurable as an escape hatch. Agreed. It is easier to add something in if we need it, than to take it out once we have offered it. - PR: https://git.openjdk.java.net/jdk/pull/6409
Re: RFR: 8278518: String(byte[], int, int, Charset) constructor and String.translateEscapes() miss bounds check elimination
On Tue, 14 Dec 2021 13:20:46 GMT, Alan Bateman wrote: >>> Originally this was spotted by by Amir Hadadi in >>> https://stackoverflow.com/questions/70272651/missing-bounds-checking-elimination-in-string-constructor >> >> Before anyone looks at this, can you confirm that the patch does not include >> any code or tests/benchmarks that were taken from SO? > >> @AlanBateman the benchmark is mine along with the changes for >> `translateEscapes` and `newStringUTF8NoRepl`, the change for constructor is >> from SO. > > I don't know how we can progress this PR if the patch includes code copied > from SO. Maybe the PR should be closed, the JBS issue unassigned, and leave > it to someone else to start again? Maybe you could get Amit to sign the OCA > and you co-contribute/author the PR? I can't look at the patch so I don't > know how significant the changes, maybe there are other options. @AlanBateman I suggest to decide first whether this should be fixed on HotSpot level (which is the best option to me) or on Java level. If we choose HotSpot then the issue should be reassigned, because I cannot do the fix myself. If we choose Java then I would revert the change for String constructor and Amir would commit it into this branch having OCA signed. What do you think? - PR: https://git.openjdk.java.net/jdk/pull/6812
Re: [jdk18] RFR: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation
On Wed, 15 Dec 2021 05:59:45 GMT, Vladimir Kozlov wrote: > A proper fix for this is to use the catchException combination. However, that > introduces significant cold startup performance regression. JDK-8278447 > tracks the work to address the performance regression using catchException > and asSpreader combinator. It may require significant work and refactoring > which is risky for JDK 18. > > It is proposed to implement a workaround in C2 to white list the relevant > methods (all methods in sun.invoke.util.ValueConversions class) not to omit > stack trace when exception is thrown in them. > > Added new regression test. Tested tier1-3. Marked as reviewed by dlong (Reviewer). - PR: https://git.openjdk.java.net/jdk18/pull/27
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v5]
On Tue, 30 Nov 2021 23:03:48 GMT, Paul Sandoz wrote:
>> kabutz has updated the pull request incrementally with one additional commit
>> since the last revision:
>>
>> Made forkOrInvoke() method protected to avoid strange compiler error
>
> src/java.base/share/classes/java/math/BigInteger.java line 1878:
>
>> 1876:
>> 1877: @Serial
>> 1878: private static final long serialVersionUID = 0L;
>
> I don't think you need to declare these, the instances are never intended to
> support serialization e.g. in the stream implementation for `AbstractTask`
> that extends `CountedCompleter` we state:
>
> * Serialization is not supported as there is no intention to serialize
> * tasks managed by stream ops.
Thanks, I see now that it is common for the ForkJoinTasks to be annotated with
`@SuppressWarnings("serial")`
Changed
-
PR: https://git.openjdk.java.net/jdk/pull/6409
