Re: RFR: 8286191: misc tests fail due to JDK-8285987 [v4]
> The isMusl method had to be handled in > test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java . > Additionally, the vm.musl predicate seem not to be available in the langtools > tests. Matthias Baesken 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 remote-tracking branch 'origin/master' into JDK-8286191 - restore year in ExternalEditorTest, remove test exclusion - Merge remote-tracking branch 'origin/master' into JDK-8286191 - remove from ProblemList - Merge remote-tracking branch 'origin/master' into JDK-8286191 - JDK-8286191 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8556/files - new: https://git.openjdk.java.net/jdk/pull/8556/files/2337301c..4c9e5b7e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8556&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8556&range=02-03 Stats: 104552 lines in 1272 files changed: 94955 ins; 4519 del; 5078 mod Patch: https://git.openjdk.java.net/jdk/pull/8556.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8556/head:pull/8556 PR: https://git.openjdk.java.net/jdk/pull/8556
Integrated: 8286191: misc tests fail due to JDK-8285987
On Thu, 5 May 2022 15:21:23 GMT, Matthias Baesken wrote: > The isMusl method had to be handled in > test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java . > Additionally, the vm.musl predicate seem not to be available in the langtools > tests. This pull request has now been integrated. Changeset: de8f4d01 Author:Matthias Baesken URL: https://git.openjdk.java.net/jdk/commit/de8f4d01b234f5224a687dae5db52ab31247c2da Stats: 6 lines in 4 files changed: 0 ins; 4 del; 2 mod 8286191: misc tests fail due to JDK-8285987 Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/8556
Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments
On Mon, 9 May 2022 15:56:35 GMT, Adam Sotona wrote: > Please review this patch adding new lint option, **lossy-conversions**, to > javac to warn about type casts in compound assignments with possible lossy > conversions. > > The new lint warning is shown if the type of the right-hand operand of a > compound assignment is not assignment compatible with the type of the > variable. > > The implementation of the warning is based on similar check performed to emit > "possible lossy conversion" compilation error for simple assignments. > > Proposed patch also include complex matrix-style test with positive and > negative test cases of lossy conversions in compound assignments. > > Proposed patch also disables this new lint option in all affected JDK modules > and libraries to allow smooth JDK build. Individual cases to address possibly > lossy conversions warnings in JDK are already addressed in a separate > umbrella issue and its sub-tasks. > > Thanks for your review, > Adam I agree with the priority to keep java.base and java.desktop clean from possibly lossy conversions, so the related issues should probably raise from P4 priority level. However this lint warning as a part of the javac is critical to confirm that the situations have been correctly addressed. If we want to avoid "blind" patching, we only two possible scenarios: 1. big homogenous patch including hundreds of fixed lines of code across many "moving-target" classes, together with lint warning implemented and enabled 2. javac lint patch (disabled for affected JDK modules build) goes first, so each case can be resolved, reviewed and validated in individual patch >From complexity and cost perspective I prefer the second scenario. - PR: https://git.openjdk.java.net/jdk/pull/8599
Re: RFR: 8282701: Use Class.getInterfaces(false) where possible to reduce allocation pressure [v2]
On Wed, 4 May 2022 09:46:00 GMT, Сергей Цыпанов wrote: >> `Class.getInterfaces(false)` does not clone underlying array and can be used >> in cases when the returned array is only read from. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8282701: Revert some irrelevant changes Marked as reviewed by redestad (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7714
Integrated: 8286163: micro-optimize Instant.plusSeconds
On Wed, 4 May 2022 20:27:04 GMT, lennartfricke wrote: > Provide micro-benchmark for comparison This pull request has now been integrated. Changeset: 34621909 Author:Lennart Fricke Committer: Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/3462190965befc07fc79202b67f7927fc856 Stats: 94 lines in 2 files changed: 93 ins; 0 del; 1 mod 8286163: micro-optimize Instant.plusSeconds Reviewed-by: scolebourne, redestad, naoto - PR: https://git.openjdk.java.net/jdk/pull/8542
Integrated: 8286298: Remove unused methods in sun.invoke.util.VerifyType
On Fri, 6 May 2022 11:32:25 GMT, Claes Redestad wrote: > A few untested and unused methods in `VerifyType` which can be removed. > (Possibly used by native JSR 292 implementations in JDK 7). This pull request has now been integrated. Changeset: 3fa1c404 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/3fa1c4043919943baf0a2cdfaf040ffdd844750c Stats: 91 lines in 2 files changed: 2 ins; 85 del; 4 mod 8286298: Remove unused methods in sun.invoke.util.VerifyType Reviewed-by: bpb, alanb, mchung - PR: https://git.openjdk.java.net/jdk/pull/8570
Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v2]
> Please review this patch adding new lint option, **lossy-conversions**, to > javac to warn about type casts in compound assignments with possible lossy > conversions. > > The new lint warning is shown if the type of the right-hand operand of a > compound assignment is not assignment compatible with the type of the > variable. > > The implementation of the warning is based on similar check performed to emit > "possible lossy conversion" compilation error for simple assignments. > > Proposed patch also include complex matrix-style test with positive and > negative test cases of lossy conversions in compound assignments. > > Proposed patch also disables this new lint option in all affected JDK modules > and libraries to allow smooth JDK build. Individual cases to address possibly > lossy conversions warnings in JDK are already addressed in a separate > umbrella issue and its sub-tasks. > > Thanks for your review, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: 8244681: Add a warning for possibly lossy conversion in compound assignments recommended correction of the warning wording fixed typo in test method name - Changes: - all: https://git.openjdk.java.net/jdk/pull/8599/files - new: https://git.openjdk.java.net/jdk/pull/8599/files/47779ba5..6b3942b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8599&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8599&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8599.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8599/head:pull/8599 PR: https://git.openjdk.java.net/jdk/pull/8599
Re: RFR: 8262889: Compiler implementation for Record Patterns [v4]
On Mon, 9 May 2022 20:52:15 GMT, Vicente Romero wrote:
> I've noticed that this code:
>
> ```
> class Test {
> String e(E e) {
> return switch (e) {
> case A -> "42";
> };
> }
>
> enum E {
> A, B;
> }
> }
> ```
>
> fails with:
>
> ```
> Test.java:3: error: the switch expression does not cover all possible input
> values
> return switch (e) {
>^
> 1 error
> ```
>
> before this change but gets accepted with no errors after the change in this
> PR
Oops, sorry, should be fixed now
([a0d0c78](https://github.com/openjdk/jdk/pull/8516/commits/a0d0c78bcbb5ecb010c2e9bd235b3ae89eb00823)).
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v5]
> 8262889: Compiler implementation for Record Patterns > > A first version of a patch that introduces record patterns into javac as a > preview feature. For the specification, please see: > http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html > > There are two notable tricky parts: > -in the parser, it was necessary to improve the `analyzePattern` method to > handle nested/record patterns, while still keeping error recovery reasonable > -in the `TransPatterns`, the desugaring of the record patterns is very > straightforward - effectivelly the record patterns are desugared into > guards/conditions. This will likely be improved in some future version/preview > > `MatchException` has been extended to cover additional cases related to > record patterns. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Fixing (non-)exhaustiveness for enum. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8516/files - new: https://git.openjdk.java.net/jdk/pull/8516/files/10cd9d0c..a0d0c78b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8516&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8516&range=03-04 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8516/head:pull/8516 PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]
On Thu, 31 Mar 2022 08:03:23 GMT, Andrey Turbanov wrote:
>> Method `Class.isAssignableFrom` is often used in form of:
>>
>> if (clazz.isAssignableFrom(obj.getClass())) {
>> Such condition could be simplified to more shorter and performarnt code
>>
>> if (clazz.isInstance(obj)) {
>>
>> Replacement is equivalent if it's known that `obj != null`.
>> In JDK codebase there are many such places.
>>
>> I tried to measure performance via JMH
>>
>> Class integerClass = Integer.class;
>> Class numberClass = Number.class;
>>
>> Object integerObject = 45666;
>> Object doubleObject = 8777d;
>>
>> @Benchmark
>> public boolean integerInteger_isInstance() {
>> return integerClass.isInstance(integerObject);
>> }
>>
>> @Benchmark
>> public boolean integerInteger_isAssignableFrom() {
>> return integerClass.isAssignableFrom(integerObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean integerDouble_isInstance() {
>> return integerClass.isInstance(doubleObject);
>> }
>>
>> @Benchmark
>> public boolean integerDouble_isAssignableFrom() {
>> return integerClass.isAssignableFrom(doubleObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean numberDouble_isInstance() {
>> return numberClass.isInstance(doubleObject);
>> }
>>
>> @Benchmark
>> public boolean numberDouble_isAssignableFrom() {
>> return numberClass.isAssignableFrom(doubleObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean numberInteger_isInstance() {
>> return numberClass.isInstance(integerObject);
>> }
>>
>> @Benchmark
>> public boolean numberInteger_isAssignableFrom() {
>> return numberClass.isAssignableFrom(integerObject.getClass());
>> }
>>
>> @Benchmark
>> public void numberIntegerDouble_isInstance(Blackhole bh) {
>> bh.consume(numberClass.isInstance(integerObject));
>> bh.consume(numberClass.isInstance(doubleObject));
>> }
>>
>> @Benchmark
>> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
>> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
>> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
>> }
>>
>> `isInstance` is a bit faster than `isAssignableFrom`
>>
>> Benchmark Mode Cnt Score Error Units
>> integerDouble_isAssignableFrom avgt5 1,173 ± 0,026 ns/op
>> integerDouble_isInstance avgt5 0,939 ± 0,038 ns/op
>> integerIntegerDouble_isAssignableFrom avgt5 2,106 ± 0,068 ns/op
>> numberIntegerDouble_isInstance avgt5 1,516 ± 0,046 ns/op
>> integerInteger_isAssignableFromavgt5 1,175 ± 0,029 ns/op
>> integerInteger_isInstance avgt5 0,886 ± 0,017 ns/op
>> numberDouble_isAssignableFrom avgt5 1,172 ± 0,007 ns/op
>> numberDouble_isInstanceavgt5 0,891 ± 0,030 ns/op
>> numberInteger_isAssignableFrom avgt5 1,169 ± 0,014 ns/op
>> numberInteger_isInstance avgt5 0,887 ± 0,016 ns/op
>
> Andrey Turbanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8280035: Use Class.isInstance instead of Class.isAssignableFrom where
> applicable
> apply suggestion to avoid second Method.invoke call
src/java.desktop/share/classes/javax/imageio/spi/ServiceRegistry.java line 230:
> 228: List l = new ArrayList<>();
> 229: for (Class c : categoryMap.keySet()) {
> 230: if (c.isInstance(provider)) {
Can this be reached if `provider` is null? If yes there could be a change of
behaviour as the previous code would have thrown NPE.
-
PR: https://git.openjdk.java.net/jdk/pull/7061
Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]
On Tue, 10 May 2022 11:10:50 GMT, Daniel Fuchs wrote:
>> Andrey Turbanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8280035: Use Class.isInstance instead of Class.isAssignableFrom where
>> applicable
>> apply suggestion to avoid second Method.invoke call
>
> src/java.desktop/share/classes/javax/imageio/spi/ServiceRegistry.java line
> 230:
>
>> 228: List l = new ArrayList<>();
>> 229: for (Class c : categoryMap.keySet()) {
>> 230: if (c.isInstance(provider)) {
>
> Can this be reached if `provider` is null? If yes there could be a change of
> behaviour as the previous code would have thrown NPE.
No. This method is called from 3 places, and there 3 null checks before the
method call.
-
PR: https://git.openjdk.java.net/jdk/pull/7061
Re: HttpClient has no explicit way of releasing threads
Hi Rafael, On 09/05/2022 22:43, Rafael Winterhalter wrote: Hello, looking at thread dumps, I realized that the HttpClient implementation does not offer an explicit way to release its threads. Currently, the client: 1. maintains a cached thread pool with a retention size of 60 seconds. If many such clients are created for short lived application, these threads pile up. 2. has a selector thread that only shuts down if the outer "facade" reference is collected via a weak reference. If an application is not running GC, this can take a while. This is not a big problem but I have seen peaks with suddenly many, many threads (in test code) where many HttpClients were created for single use and I was wondering if it was ever considered to add a method for disposing the threads explicitly? I would consider it bad practice to create an HttpClient instance for single usage; Ideally a client should be reused - provided that the security context is the same. Creating an HttpClient involves creating a thread pool, a selector, a selector manager thread, potentially initializing an SSL context etc... WRT to adding a method for disposing the HttpClient explicitly, then yes - that's something that we could consider for a major release. It would need to be carefully specified - especially WRT what would be the effect of calling this method if some operations are still in progress. Asynchronously closing objects that are still in use is a notoriously thorny subject. We might need something equivalent to what is defined for executor services - that is - one variant that waits for all ongoing operations to terminate before closing, and one that abruptly aborts any on-going operation. Alternatively, it might be an option to add a method like HttpClient.shared() which would return a singleton HttpClient (created on the first call, collected if no reference is kept anymore but reused in the meantime) to address such scenarios. I can of course add a singleton in my test project but I find this a bit awkward as it is something to remember and to repeat in all applications we maintain. Therefore, it might be convenient to add such methods for tests that usually aim to be decoupled. An HttpClient is a kind of capability object so I don't think we want to have a shared client in the Java API. That's something that an application can easily implement at the application level if it makes sense for the application. A possibility to work around the thread peak issue is also for an application to configure its own thread executor on the HttpClient. If that makes sense for the application and if it is safe to do so the executor can also be shared between several client. It is then the responsibility of the application to shutdown the executor when the clients are no longer in use. Thanks for your consideration, best regards, Rafael best regards, -- daniel
Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]
On Tue, 10 May 2022 11:31:16 GMT, Andrey Turbanov wrote:
>> src/java.desktop/share/classes/javax/imageio/spi/ServiceRegistry.java line
>> 230:
>>
>>> 228: List l = new ArrayList<>();
>>> 229: for (Class c : categoryMap.keySet()) {
>>> 230: if (c.isInstance(provider)) {
>>
>> Can this be reached if `provider` is null? If yes there could be a change of
>> behaviour as the previous code would have thrown NPE.
>
> No. This method is called from 3 places, and there 3 null checks before the
> method call.
Thanks for double checking! LGTM then.
-
PR: https://git.openjdk.java.net/jdk/pull/7061
Re: RFR: 8282664: Unroll by hand StringUTF16 and StringLatin1 polynomial hash loops [v10]
> Despite the hash value being cached for Strings, computing the hash still > represents a significant CPU usage for applications handling lots of text. > > Even though it would be generally better to do it through an enhancement to > the autovectorizer, the complexity of doing it by hand is trivial and the > gain is sizable (2x speedup) even without the Vector API. The algorithm has > been proposed by Richard Startin and Paul Sandoz [1]. > > Speedup are as follows on a `Intel(R) Xeon(R) E-2276G CPU @ 3.80GHz` > > > Benchmark(size) Mode Cnt Score > Error Units > StringHashCode.Algorithm.scalarLatin1 0 avgt 25 2.111 > ± 0.210 ns/op > StringHashCode.Algorithm.scalarLatin1 1 avgt 25 3.500 > ± 0.127 ns/op > StringHashCode.Algorithm.scalarLatin110 avgt 25 7.001 > ± 0.099 ns/op > StringHashCode.Algorithm.scalarLatin1 100 avgt 2561.285 > ± 0.444 ns/op > StringHashCode.Algorithm.scalarLatin1 1000 avgt 25 628.995 > ± 0.846 ns/op > StringHashCode.Algorithm.scalarLatin1 1 avgt 25 6307.990 > ± 4.071 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 0 avgt 25 2.358 > ± 0.092 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3.631 > ± 0.159 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 10 avgt 25 7.049 > ± 0.019 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 100 avgt 2533.626 > ± 1.218 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled161000 avgt 25 317.811 > ± 1.225 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3212.333 > ± 14.621 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled80 avgt 25 2.356 > ± 0.097 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3.630 > ± 0.158 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 10 avgt 25 8.724 > ± 0.065 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 100 avgt 2532.402 > ± 0.019 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 1000 avgt 25 321.949 > ± 0.251 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3202.083 > ± 1.667 ns/op > StringHashCode.Algorithm.scalarUTF16 0 avgt 25 2.135 > ± 0.191 ns/op > StringHashCode.Algorithm.scalarUTF16 1 avgt 25 5.202 > ± 0.362 ns/op > StringHashCode.Algorithm.scalarUTF16 10 avgt 2511.105 > ± 0.112 ns/op > StringHashCode.Algorithm.scalarUTF16100 avgt 2575.974 > ± 0.702 ns/op > StringHashCode.Algorithm.scalarUTF16 1000 avgt 25 716.429 > ± 3.290 ns/op > StringHashCode.Algorithm.scalarUTF16 1 avgt 25 7095.459 > ± 43.847 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled160 avgt 25 2.381 > ± 0.038 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 5.268 > ± 0.422 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 10 avgt 2511.248 > ± 0.178 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 100 avgt 2552.966 > ± 0.089 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 1000 avgt 25 450.912 > ± 1.834 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 4403.988 > ± 2.927 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 0 avgt 25 2.401 > ± 0.032 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 5.091 > ± 0.396 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled810 avgt 2512.801 > ± 0.189 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 100 avgt 2552.068 > ± 0.032 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1000 avgt 25 453.270 > ± 0.340 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 4433.112 > ± 2.699 ns/op > > > At Datadog, we handle a great amount of text (through logs management for > example), and hashing String represents a large part of our CPU usage. It's > very unlikely that we are the only one as String.hashCode is such a core > feature of the JVM-based languages with its use in HashMap for example. > Having even only a 2x speedup would allow us to save thousands of CPU cores > per month and improve correspondingly the energy/carbon impact. > > [1] > https://static.rainfocus.com/oracle/oow18/sess/1525822677955001tLqU/PF/codeone18-vector-API-DEV5081_1540354883936001Q3Sv.pdf Ludovic Henry has updated the pull request incrementally with one additional commit since the last revision: Add missing check for AryHashCode node - Changes: - all: https://git.openjdk.java.net/jdk/pull/7700/files - new: https
RFR: 8286473: Drop --enable-preview from Record related tests
There are plenty of tests failing on many architectures due to `--enable-preview` VM code introduced by Loom. This improvements eliminates some of the redundant `--enable-preview` clauses from the Record tests, since Records have been graduated from preview in JDK 16. Additional testing: - [x] Linux x86_64 fastdebug, affected tests still pass - [x] Linux x86_32 fastdebug, affected tests start to pass - Commit messages: - Fix Changes: https://git.openjdk.java.net/jdk/pull/8626/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8626&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286473 Stats: 28 lines in 3 files changed: 0 ins; 24 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8626.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8626/head:pull/8626 PR: https://git.openjdk.java.net/jdk/pull/8626
Re: RFR: 8282664: Unroll by hand StringUTF16, StringLatin1, and Arrays polynomial hash loops [v11]
> Despite the hash value being cached for Strings, computing the hash still > represents a significant CPU usage for applications handling lots of text. > > Even though it would be generally better to do it through an enhancement to > the autovectorizer, the complexity of doing it by hand is trivial and the > gain is sizable (2x speedup) even without the Vector API. The algorithm has > been proposed by Richard Startin and Paul Sandoz [1]. > > Speedup are as follows on a `Intel(R) Xeon(R) E-2276G CPU @ 3.80GHz` > > > Benchmark(size) Mode Cnt Score > Error Units > StringHashCode.Algorithm.scalarLatin1 0 avgt 25 2.111 > ± 0.210 ns/op > StringHashCode.Algorithm.scalarLatin1 1 avgt 25 3.500 > ± 0.127 ns/op > StringHashCode.Algorithm.scalarLatin110 avgt 25 7.001 > ± 0.099 ns/op > StringHashCode.Algorithm.scalarLatin1 100 avgt 2561.285 > ± 0.444 ns/op > StringHashCode.Algorithm.scalarLatin1 1000 avgt 25 628.995 > ± 0.846 ns/op > StringHashCode.Algorithm.scalarLatin1 1 avgt 25 6307.990 > ± 4.071 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 0 avgt 25 2.358 > ± 0.092 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3.631 > ± 0.159 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 10 avgt 25 7.049 > ± 0.019 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 100 avgt 2533.626 > ± 1.218 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled161000 avgt 25 317.811 > ± 1.225 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3212.333 > ± 14.621 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled80 avgt 25 2.356 > ± 0.097 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3.630 > ± 0.158 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 10 avgt 25 8.724 > ± 0.065 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 100 avgt 2532.402 > ± 0.019 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 1000 avgt 25 321.949 > ± 0.251 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3202.083 > ± 1.667 ns/op > StringHashCode.Algorithm.scalarUTF16 0 avgt 25 2.135 > ± 0.191 ns/op > StringHashCode.Algorithm.scalarUTF16 1 avgt 25 5.202 > ± 0.362 ns/op > StringHashCode.Algorithm.scalarUTF16 10 avgt 2511.105 > ± 0.112 ns/op > StringHashCode.Algorithm.scalarUTF16100 avgt 2575.974 > ± 0.702 ns/op > StringHashCode.Algorithm.scalarUTF16 1000 avgt 25 716.429 > ± 3.290 ns/op > StringHashCode.Algorithm.scalarUTF16 1 avgt 25 7095.459 > ± 43.847 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled160 avgt 25 2.381 > ± 0.038 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 5.268 > ± 0.422 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 10 avgt 2511.248 > ± 0.178 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 100 avgt 2552.966 > ± 0.089 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 1000 avgt 25 450.912 > ± 1.834 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 4403.988 > ± 2.927 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 0 avgt 25 2.401 > ± 0.032 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 5.091 > ± 0.396 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled810 avgt 2512.801 > ± 0.189 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 100 avgt 2552.068 > ± 0.032 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1000 avgt 25 453.270 > ± 0.340 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 4433.112 > ± 2.699 ns/op > > > At Datadog, we handle a great amount of text (through logs management for > example), and hashing String represents a large part of our CPU usage. It's > very unlikely that we are the only one as String.hashCode is such a core > feature of the JVM-based languages with its use in HashMap for example. > Having even only a 2x speedup would allow us to save thousands of CPU cores > per month and improve correspondingly the energy/carbon impact. > > [1] > https://static.rainfocus.com/oracle/oow18/sess/1525822677955001tLqU/PF/codeone18-vector-API-DEV5081_1540354883936001Q3Sv.pdf Ludovic Henry has updated the pull request incrementally with one additional commit since the last revision: Fix h when vectorized for Arrays.hashCode - Changes: - all: https://git.openjdk.java.net/jdk/pull/7700/files - new: ht
RFR: 8286474: Drop --enable-preview from Sealed Classes related tests
There are plenty of tests failing on many architectures due to `--enable-preview` VM code introduced by Loom. This improvement eliminates some of the redundant `--enable-preview` clauses from the Sealed Classes tests, since Sealed Classes have been graduated from preview in JDK 17. Additional testing: - [x] Linux x86_64 fastdebug, affected tests still pass - [x] Linux x86_32 fastdebug, affected tests start to pass - Commit messages: - Some Changes: https://git.openjdk.java.net/jdk/pull/8627/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8627&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286474 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8627.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8627/head:pull/8627 PR: https://git.openjdk.java.net/jdk/pull/8627
Integrated: 8238373: Punctuation should be same in jlink help usage on Japanese language
On Wed, 27 Apr 2022 08:59:20 GMT, KIRIYAMA Takuya wrote: > When showing help for the jlink command in a Japanese locale, delimiters of > option's aliases are a mixture of "," and \u3001. Delimiter should be unified > to ",". > As the same, there is a inconsistency of delimiters in the jar command help > in a Japanese locale, and "," should be used. > Similarly, the javap command help uses space as delimiters other than the > module option in all locales. This inconsistency should also be corrected. > > Would you please review this fix? This pull request has now been integrated. Changeset: c4bd4499 Author:KIRIYAMA Takuya Committer: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/c4bd4499f1476dd300d967c556750cf8a5f1c5c7 Stats: 24 lines in 6 files changed: 0 ins; 0 del; 24 mod 8238373: Punctuation should be same in jlink help usage on Japanese language Reviewed-by: naoto - PR: https://git.openjdk.java.net/jdk/pull/8417
Re: RFR: 8286473: Drop --enable-preview from Record related tests
On Tue, 10 May 2022 12:03:09 GMT, Aleksey Shipilev wrote: > There are plenty of tests failing on many architectures due to > `--enable-preview` VM code introduced by Loom. This improvements eliminates > some of the redundant `--enable-preview` clauses from the Record tests, since > Records have been graduated from preview in JDK 16. > > Additional testing: > - [x] Linux x86_64 fastdebug, affected tests still pass > - [x] Linux x86_32 fastdebug, affected tests start to pass Hello Aleksey, the `unreflect` test package has just one test and that one uses the `record` feature. So removing the `TEST.properties` for that entire package looks fine. The changes to the other 2 tests look fine too, since like you note, they are just `record` related tests and don't have any other preview feature being tested. I think these two tests would need a copyright year update. - PR: https://git.openjdk.java.net/jdk/pull/8626
RFR: 8286212: Cgroup v1 initialization causes NPE on some systems
Please review this change to the cgroup v1 subsystem which makes it more resilient on some of the stranger systems. Unfortunately, I wasn't able to re-create a similar system as the reporter. The idea of using the longest substring match for the cgroupv1 file paths was based on the fact that on systemd systems processes run in separate scopes and the maven forked test runner might exhibit this property. For that it makes sense to use the common ancestor path. Nothing changes in the common cases where the `cgroup_path` matches `_root` and when the `_root` is `/` (container the former, host system the latter). In addition, the code paths which are susceptible to throw NPE have been hardened to catch those situations. Should it happen in the future it makes more sense (to me) to not have accurate container detection in favor of continuing to keep running. Finally, with the added unit-tests a bug was uncovered on the "substring" match case of cgroup paths in hotspot. `p` returned from `strstr` can never point to `_root` as it's used as the "needle" to find in "haystack" `cgroup_path` (not the other way round). Testing: - [x] Added unit tests - [x] GHA - [x] Container tests on cgroups v1 Linux. Continue to pass - Commit messages: - 8286212: Cgroup v1 initialization causes NPE on some systems Changes: https://git.openjdk.java.net/jdk/pull/8629/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8629&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286212 Stats: 267 lines in 7 files changed: 256 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8629.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8629/head:pull/8629 PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests
On Tue, 10 May 2022 12:07:31 GMT, Aleksey Shipilev wrote: > There are plenty of tests failing on many architectures due to > `--enable-preview` VM code introduced by Loom. This improvement eliminates > some of the redundant `--enable-preview` clauses from the Sealed Classes > tests, since Sealed Classes have been graduated from preview in JDK 17. > > Additional testing: > - [x] Linux x86_64 fastdebug, affected tests still pass > - [x] Linux x86_32 fastdebug, affected tests start to pass The changes look fine to me. Both these files will need a copyright year update, I think. - PR: https://git.openjdk.java.net/jdk/pull/8627
Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v3]
> Hi All, > > Patch adds the planned support for new vector operations and APIs targeted > for [JEP 426: Vector API (Fourth > Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173) > > Following is the brief summary of changes:- > > 1) Extends the scope of existing lanewise API for following new vector > operations. >- VectorOperations.BIT_COUNT: counts the number of one-bits >- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero > bits >- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing > zero bits >- VectorOperations.REVERSE: reversing the order of bits >- VectorOperations.REVERSE_BYTES: reversing the order of bytes >- compress and expand bits: Semantics are based on Hacker's Delight > section 7-4 Compress, or Generalized Extract. > > 2) Adds following new APIs to perform cross lane vector compress and > expansion operations under the influence of a mask. >- Vector.compress >- Vector.expand >- VectorMask.compress > > 3) Adds predicated and non-predicated versions of following new APIs to load > and store the contents of vector from foreign MemorySegments. > - Vector.fromMemorySegment > - Vector.intoMemorySegment > > 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support > for each newly added operation. > > > Patch has been regressed over AARCH64 and X86 targets different AVX levels. > > Kindly review and share your feedback. > > Best Regards, > Jatin Jatin Bhateja has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - 8284960: Correcting a typo. - 8284960: Integrating changes from panama-vector (Add @since 19 tags). - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - 8284960: AARCH64 backend changes. - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/3fa1c404...b021e082 - Changes: https://git.openjdk.java.net/jdk/pull/8425/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8425&range=02 Stats: 37901 lines in 214 files changed: 16527 ins; 16924 del; 4450 mod Patch: https://git.openjdk.java.net/jdk/pull/8425.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8425/head:pull/8425 PR: https://git.openjdk.java.net/jdk/pull/8425
Re: HttpClient has no explicit way of releasing threads
Thanks, I created a shared client for now which solves my issue. I only stumbled upon this as a build server increased its use of resources and the change was that somebody had removed a third-party (closeable) HTTP client to replace it with the JDK client. Unit tests often aim to be decoupled, therefore I assume this is an easy mistake to make. To be fair, while it might be inefficient to recreate the client for each test class, the value of a decoupled test suite might be higher, so I understand why the developer went for this solution. I'd appreciate a method for explicit disposal and possibly a clarification in the javadoc of HttpClient how resources are held. I had to dig through the sources of the client to understand what happened. Thanks, Rafael Daniel Fuchs schrieb am Di., 10. Mai 2022, 13:37: > Hi Rafael, > > On 09/05/2022 22:43, Rafael Winterhalter wrote: > > Hello, > > > > looking at thread dumps, I realized that the HttpClient implementation > does > > not offer an explicit way to release its threads. Currently, the client: > > > > 1. maintains a cached thread pool with a retention size of 60 seconds. If > > many such clients are created for short lived application, these threads > > pile up. > > 2. has a selector thread that only shuts down if the outer "facade" > > reference is collected via a weak reference. If an application is not > > running GC, this can take a while. > > > > This is not a big problem but I have seen peaks with suddenly many, many > > threads (in test code) where many HttpClients were created for single use > > and I was wondering if it was ever considered to add a method for > disposing > > the threads explicitly? > > I would consider it bad practice to create an HttpClient instance for > single usage; Ideally a client should be reused - provided that > the security context is the same. Creating an HttpClient involves > creating a thread pool, a selector, a selector manager thread, > potentially initializing an SSL context etc... > > WRT to adding a method for disposing the HttpClient explicitly, then > yes - that's something that we could consider for a major > release. It would need to be carefully specified - especially WRT what > would be the effect of calling this method if some operations are still > in progress. Asynchronously closing objects that are still in use is > a notoriously thorny subject. > We might need something equivalent to what is defined for executor > services - that is - one variant that waits for all ongoing operations > to terminate before closing, and one that abruptly aborts any > on-going operation. > > > Alternatively, it might be an option to add a method like > > HttpClient.shared() which would return a singleton HttpClient (created on > > the first call, collected if no reference is kept anymore but reused in > the > > meantime) to address such scenarios. I can of course add a singleton in > my > > test project but I find this a bit awkward as it is something to remember > > and to repeat in all applications we maintain. Therefore, it might be > > convenient to add such methods for tests that usually aim to be > decoupled. > > An HttpClient is a kind of capability object so I don't think we want > to have a shared client in the Java API. That's something that > an application can easily implement at the application level if it > makes sense for the application. > > A possibility to work around the thread peak issue is also for an > application to configure its own thread executor on the HttpClient. > If that makes sense for the application and if it is safe to do > so the executor can also be shared between several client. > It is then the responsibility of the application > to shutdown the executor when the clients are no longer in use. > > > > > Thanks for your consideration, > > best regards, Rafael > > best regards, > > -- daniel >
Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests
On Tue, 10 May 2022 12:07:31 GMT, Aleksey Shipilev wrote:
> There are plenty of tests failing on many architectures due to
> `--enable-preview` VM code introduced by Loom. This improvement eliminates
> some of the redundant `--enable-preview` clauses from the Sealed Classes
> tests, since Sealed Classes have been graduated from preview in JDK 17.
>
> Additional testing:
> - [x] Linux x86_64 fastdebug, affected tests still pass
> - [x] Linux x86_32 fastdebug, affected tests start to pass
test/jdk/java/lang/reflect/sealed_classes/SealedClassesReflectionTest.java line
29:
> 27: * @summary reflection test for sealed classes
> 28: * @compile -source ${jdk.version} SealedClassesReflectionTest.java
> 29: * @run testng/othervm SealedClassesReflectionTest
You should be able to drop` -source ${jdk.version}` too. It was required when
compiling with `--enable-preview`.
-
PR: https://git.openjdk.java.net/jdk/pull/8627
Re: RFR: 8279986: methods Math::asXExact for safely checked primitive casts [v2]
On Tue, 10 May 2022 04:42:19 GMT, Joe Darcy wrote:
>> Raffaello Giulietti has updated the pull request with a new target base due
>> to a merge or a rebase. The incremental webrev excludes the unrelated
>> changes brought in by the merge/rebase. The pull request contains four
>> additional commits since the last revision:
>>
>> - 8279986: methods Math::asXExact for safely checked primitive casts
>>
>>Merge branch 'master' into JDK-8279986
>> - 8279986: methods Math::asXExact for safely checked primitive casts
>>
>>Merge branch 'master' into JDK-8279986
>> - 8279986: methods Math::asXExact for safely checked primitive casts
>> - 8279986: methods Math::asXExact for safely checked primitive casts
>
> src/java.base/share/classes/java/lang/Math.java line 1578:
>
>> 1576: */
>> 1577: @ForceInline
>> 1578: public static long toUnsignedIntExact(long value) {
>
> Existing methods like Integer.parseUnsignedInt interpret the negative int
> values as positive values larger than MAX_INT. So if an int is not going to
> be returned here, I suggest a name like "toUnsignedIntRangeExact".
Returning a `long` is probably less error prone.
When the result is to be used in an `int` context, one simply has to add a
`(int)` cast, as mandated by language, compiler and IDE.
On the other hand, if this method were to return an `int`, when using the
result in a `long` context one has to remember masking it with `0x_L`.
AFAIK, there's no compiler or IDE support for this.
The name `toUnsignedIntRangeExact` is certainly better.
-
PR: https://git.openjdk.java.net/jdk/pull/8548
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected
On Mon, 9 May 2022 18:59:43 GMT, Naoto Sato wrote:
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support
> second-level resolution, enabling round trips with `java.time.ZoneOffset`s.
> Corresponding CSR is also being drafted.
src/java.base/share/classes/java/util/TimeZone.java line 109:
> 107: *
> 108: * NormalizedCustomID:
> 109: * {@code GMT} Sign TwoDigitHours {@code :}
> Minutes [Seconds]
Hello Naoto,
Should this instead be: `... Minutes [{@code :} Seconds]` - i.e.
should it have the `:` literal if seconds are present in the custom timezone id?
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: HttpClient has no explicit way of releasing threads
- Original Message - > From: "Rafael Winterhalter" > To: "core-libs-dev" > Sent: Monday, May 9, 2022 11:43:49 PM > Subject: HttpClient has no explicit way of releasing threads > Hello, Hello, > > looking at thread dumps, I realized that the HttpClient implementation does > not offer an explicit way to release its threads. Currently, the client: > > 1. maintains a cached thread pool with a retention size of 60 seconds. If > many such clients are created for short lived application, these threads > pile up. > 2. has a selector thread that only shuts down if the outer "facade" > reference is collected via a weak reference. If an application is not > running GC, this can take a while. > > This is not a big problem but I have seen peaks with suddenly many, many > threads (in test code) where many HttpClients were created for single use > and I was wondering if it was ever considered to add a method for disposing > the threads explicitly? You can change the Executor (it's one parameter of the builder) to use whatever executors you want so you can shutdown that executor as you want. This should fixed (1). Also once you update to Java 19/21, it seems a good scenario to test the executor that spawn virtual threads instead of platform threads. > > Alternatively, it might be an option to add a method like > HttpClient.shared() which would return a singleton HttpClient (created on > the first call, collected if no reference is kept anymore but reused in the > meantime) to address such scenarios. I can of course add a singleton in my > test project but I find this a bit awkward as it is something to remember > and to repeat in all applications we maintain. Therefore, it might be > convenient to add such methods for tests that usually aim to be decoupled. > > Thanks for your consideration, > best regards, Rafael regards, Rémi
Re: RFR: 8279986: methods Math::asXExact for safely checked primitive casts [v3]
> Add a family of "safe" cast methods. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 8279986: methods Math::asXExact for safely checked primitive casts - Changes: - all: https://git.openjdk.java.net/jdk/pull/8548/files - new: https://git.openjdk.java.net/jdk/pull/8548/files/7be0f9de..5f0ff527 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8548&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8548&range=01-02 Stats: 35 lines in 2 files changed: 0 ins; 0 del; 35 mod Patch: https://git.openjdk.java.net/jdk/pull/8548.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8548/head:pull/8548 PR: https://git.openjdk.java.net/jdk/pull/8548
Re: HttpClient has no explicit way of releasing threads
On 10/05/2022 14:39, Remi Forax wrote: This is not a big problem but I have seen peaks with suddenly many, many threads (in test code) where many HttpClients were created for single use and I was wondering if it was ever considered to add a method for disposing the threads explicitly? You can change the Executor (it's one parameter of the builder) to use whatever executors you want so you can shutdown that executor as you want. This should fixed (1). Also once you update to Java 19/21, it seems a good scenario to test the executor that spawn virtual threads instead of platform threads. Some word of caution about shutting down the executor: If you know that the client is no longer used, and there are no requests in progress, what Rémi suggests should be fine. Otherwise shutting down the executor when the client is still in use could lead to undefined behaviour, including not being able to complete the CompletableFutures that have been returned by `sendAsync` - or which `send` calls have joined. This has been fixed in JDK 19 by JDK-8277969, but otherwise, and especially on previous versions of the JDK, you should make sure that all operations have terminated before shutting down the executor (even gracefully). Using virtual threads should be fine - as long as they are not pooled :-) best regards, -- daniel
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Mon, 9 May 2022 22:10:54 GMT, Christoph Langer wrote: > After https://bugs.openjdk.java.net/browse/JDK-8251329, javac throws errors > when the classpath > contains jar files with . or .. in its name. The error message, however, does > not help to find > the culprit. This could be improved. LGTM. I think this is an important improvement. Developers should not be left alone figuring out which .jar file is responsible for the problem. There is currently no hint at all without this change. (Note: Pre-submit test issues on 32 bit are unrelated.) - Marked as reviewed by mdoerr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8616
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Mon, 9 May 2022 22:32:57 GMT, Christoph Langer wrote: > @LanceAndersen @AlanBateman do you think adding the entry name in the > exception in ZipFileSystem is ok? If so, should it maybe go into a different > patch? It should be okay as this is the name of an entry in the zip file. It might be a bit cleaner to add a method to IndexNode to return the name as String. Alternatively maybe its toString could be changed to drop the index (I would need to dig into the history to find out if there is really any use for that in the String representation). - PR: https://git.openjdk.java.net/jdk/pull/8616
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected
On Mon, 9 May 2022 18:59:43 GMT, Naoto Sato wrote:
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support
> second-level resolution, enabling round trips with `java.time.ZoneOffset`s.
> Corresponding CSR is also being drafted.
src/java.base/share/classes/java/util/TimeZone.java line 543:
> 541: return new ZoneInfo(totalSecs == 0 ? "UTC" : GMT_ID + tzid,
> totalSecs);
> 542: } else {
> 543: return getTimeZone(tzid, true);
Before the change in this PR, we used to prefix `GMT` to (non-custom timezone
ids) if the timezone id returned by `ZoneId#getId()` started with the `+` or
`-` sign, before calling `getTimeZone(modifiedTzid, true)`.
With this change, for `ZoneId`s that aren't `ZoneOffset` instance, we now call
`getTimeZone(originalTzid, true)`, without first checking/prefixing the id with
`GMT`. Is that an intentional change and would that potentially cause
`getTimeZone(String, boolean)` to return a different result?
-
PR: https://git.openjdk.java.net/jdk/pull/8606
RFR: 8284493: Fix rounding error in computeNextExponential
Repeatedly adding DoubleZigguratTables.exponentialX0 to extra causes a rounding error to accumulate at the tail of the distribution (probably starting around 2*exponentialX0 == 0x1.e46eff20739afp3); this fixes that by tracking the multiple of exponentialX0 as a long. (This changes the maximum possible output to `1.0p63 * DoubleZigguratTables.exponentialX0 == 0x1.e46eff20739afp65`; previously it would have been `0x1.0p56` because once `extra` reaches that amount, `x + extra == extra` due to the rounding error. This lowers the probability of reaching the maximum with an ideal PRNG from about `1.3877787807814488E-17` to about `1.4323726067488646E-20` (calculated using the identity `ln(x) == Math.log10(x)/Math.log10(Math.exp(1))`). - Commit messages: - Fix rounding error in computeNextExponential; use FMA Changes: https://git.openjdk.java.net/jdk/pull/8131/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8131&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284493 Stats: 43 lines in 1 file changed: 33 ins; 1 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/8131.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8131/head:pull/8131 PR: https://git.openjdk.java.net/jdk/pull/8131
Re: RFR: 8284493: Fix rounding error in computeNextExponential
On Wed, 6 Apr 2022 17:47:53 GMT, Chris Hennick wrote: > Repeatedly adding DoubleZigguratTables.exponentialX0 to extra causes a > rounding error to accumulate at the tail of the distribution (probably > starting around 2*exponentialX0 == 0x1.e46eff20739afp3); this fixes that by > tracking the multiple of exponentialX0 as a long. (This changes the maximum > possible output to `1.0p63 * DoubleZigguratTables.exponentialX0 == > 0x1.e46eff20739afp65`; previously it would have been `0x1.0p56` because once > `extra` reaches that amount, `x + extra == extra` due to the rounding error. > This lowers the probability of reaching the maximum with an ideal PRNG from > about `1.3877787807814488E-17` to about `1.4323726067488646E-20` (calculated > using the identity `ln(x) == Math.log10(x)/Math.log10(Math.exp(1))`). hi, @JesperIRL, Could you help us on OCA coverage? - PR: https://git.openjdk.java.net/jdk/pull/8131
Re: RFR: 8284493: Fix rounding error in computeNextExponential
On Wed, 6 Apr 2022 17:47:53 GMT, Chris Hennick wrote: > Repeatedly adding DoubleZigguratTables.exponentialX0 to extra causes a > rounding error to accumulate at the tail of the distribution (probably > starting around 2*exponentialX0 == 0x1.e46eff20739afp3); this fixes that by > tracking the multiple of exponentialX0 as a long. (This changes the maximum > possible output to `1.0p63 * DoubleZigguratTables.exponentialX0 == > 0x1.e46eff20739afp65`; previously it would have been `0x1.0p56` because once > `extra` reaches that amount, `x + extra == extra` due to the rounding error. > This lowers the probability of reaching the maximum with an ideal PRNG from > about `1.3877787807814488E-17` to about `1.4323726067488646E-20` (calculated > using the identity `ln(x) == Math.log10(x)/Math.log10(Math.exp(1))`). Should we extract a computeWinsorizedNextExponential that can compute a smaller value for line 1216 to compare the output to, so that line 1218 can realistically be covered in a unit test? Such a method might even be worth exposing in the RandomGenerator interface, in case an approximately exponential distribution is ever needed in a hard real-time system. - PR: https://git.openjdk.java.net/jdk/pull/8131
Re: RFR: 8285285: Avoid redundant allocations in WindowsPreferences
On Wed, 20 Apr 2022 19:16:00 GMT, Andrey Turbanov wrote: > 1. No need to call `String.substring` if you need need to compare start of > string with some constant. `String.startWith` suites better. > 2. Unused String array is allocated in `childrenNamesSpi` method Marked as reviewed by jpai (Committer). I don't have knowledge of this area, but these changes look good to me. - PR: https://git.openjdk.java.net/jdk/pull/8322
Re: RFR: 8286473: Drop --enable-preview from Record related tests
On Tue, 10 May 2022 12:03:09 GMT, Aleksey Shipilev wrote:
> There are plenty of tests failing on many architectures due to
> `--enable-preview` VM code introduced by Loom. This improvements eliminates
> some of the redundant `--enable-preview` clauses from the Record tests, since
> Records have been graduated from preview in JDK 16.
>
> Additional testing:
> - [x] Linux x86_64 fastdebug, affected tests still pass
> - [x] Linux x86_32 fastdebug, affected tests start to pass
test/jdk/java/nio/Buffer/BulkPutBuffer.java line 57:
> 55: * @run testng/othervm BulkPutBuffer
> 56: */
> 57: public class BulkPutBuffer {
You can drop the -source option from these tests too.
-
PR: https://git.openjdk.java.net/jdk/pull/8626
Re: RFR: 8282664: Unroll by hand StringUTF16, StringLatin1, and Arrays polynomial hash loops [v12]
> Despite the hash value being cached for Strings, computing the hash still > represents a significant CPU usage for applications handling lots of text. > > Even though it would be generally better to do it through an enhancement to > the autovectorizer, the complexity of doing it by hand is trivial and the > gain is sizable (2x speedup) even without the Vector API. The algorithm has > been proposed by Richard Startin and Paul Sandoz [1]. > > Speedup are as follows on a `Intel(R) Xeon(R) E-2276G CPU @ 3.80GHz` > > > Benchmark(size) Mode Cnt Score > Error Units > StringHashCode.Algorithm.scalarLatin1 0 avgt 25 2.111 > ± 0.210 ns/op > StringHashCode.Algorithm.scalarLatin1 1 avgt 25 3.500 > ± 0.127 ns/op > StringHashCode.Algorithm.scalarLatin110 avgt 25 7.001 > ± 0.099 ns/op > StringHashCode.Algorithm.scalarLatin1 100 avgt 2561.285 > ± 0.444 ns/op > StringHashCode.Algorithm.scalarLatin1 1000 avgt 25 628.995 > ± 0.846 ns/op > StringHashCode.Algorithm.scalarLatin1 1 avgt 25 6307.990 > ± 4.071 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 0 avgt 25 2.358 > ± 0.092 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3.631 > ± 0.159 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 10 avgt 25 7.049 > ± 0.019 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 100 avgt 2533.626 > ± 1.218 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled161000 avgt 25 317.811 > ± 1.225 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3212.333 > ± 14.621 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled80 avgt 25 2.356 > ± 0.097 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3.630 > ± 0.158 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 10 avgt 25 8.724 > ± 0.065 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 100 avgt 2532.402 > ± 0.019 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 1000 avgt 25 321.949 > ± 0.251 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3202.083 > ± 1.667 ns/op > StringHashCode.Algorithm.scalarUTF16 0 avgt 25 2.135 > ± 0.191 ns/op > StringHashCode.Algorithm.scalarUTF16 1 avgt 25 5.202 > ± 0.362 ns/op > StringHashCode.Algorithm.scalarUTF16 10 avgt 2511.105 > ± 0.112 ns/op > StringHashCode.Algorithm.scalarUTF16100 avgt 2575.974 > ± 0.702 ns/op > StringHashCode.Algorithm.scalarUTF16 1000 avgt 25 716.429 > ± 3.290 ns/op > StringHashCode.Algorithm.scalarUTF16 1 avgt 25 7095.459 > ± 43.847 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled160 avgt 25 2.381 > ± 0.038 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 5.268 > ± 0.422 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 10 avgt 2511.248 > ± 0.178 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 100 avgt 2552.966 > ± 0.089 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 1000 avgt 25 450.912 > ± 1.834 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 4403.988 > ± 2.927 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 0 avgt 25 2.401 > ± 0.032 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 5.091 > ± 0.396 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled810 avgt 2512.801 > ± 0.189 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 100 avgt 2552.068 > ± 0.032 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1000 avgt 25 453.270 > ± 0.340 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 4433.112 > ± 2.699 ns/op > > > At Datadog, we handle a great amount of text (through logs management for > example), and hashing String represents a large part of our CPU usage. It's > very unlikely that we are the only one as String.hashCode is such a core > feature of the JVM-based languages with its use in HashMap for example. > Having even only a 2x speedup would allow us to save thousands of CPU cores > per month and improve correspondingly the energy/carbon impact. > > [1] > https://static.rainfocus.com/oracle/oow18/sess/1525822677955001tLqU/PF/codeone18-vector-API-DEV5081_1540354883936001Q3Sv.pdf Ludovic Henry has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 18 commits: - Fix overlapping registers - Actually fix h when hashcode is vectorized - Merge branch 'master' of https://
Re: RFR: 8282664: Unroll by hand StringUTF16, StringLatin1, and Arrays polynomial hash loops [v2]
On Tue, 8 Mar 2022 17:07:34 GMT, Claes Redestad wrote: >>> Can we change the optimizer so that the strength reduction happens only >>> after all transformations have settled? Carelessly changing a >>> multiplication to a shift as today may hurt a lot of potential >>> optimisations. Thanks. >> >> Yes, it's troubling that making a constant non-foldable can lead the JIT >> down a path that ultimately pessimizes the end result (as observed here). If >> we could train the JIT to avoid this pitfall and get to the improvement >> observed in my experiment here without any changes to `String.java` then >> that'd be great. > >> @cl4es Yes, we would need to carefully measure the impact for small array >> sizes (similar to what we had to do when the array mismatch intrinsic was >> implemented and applied to array equals). My sense is to focus on the >> intrinsic and also look for potential opportunities like @merykitty points >> out, as that is where the larger impact is, although it is more work! > > Right, I'm not too thrilled about the prospect of moving ahead with the > de-constantification as an alternative patch here. It's such a crutch, but > it's also simple and has no obvious downsides as of right now. I think it was > a useful experiment to see where some of the gain observed in the unroll > might be coming from. The degradation on many smaller `Strings` in the > unrolled versions is a concern that I think might be a blocker, though. Short > Strings are excessively common as keys in hash maps et.c.. > > Feels like none of the alternatives seen here so far is really _it_. @cl4es that was indeed the issue leading to the crash. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/7700
Re: RFR: 8286473: Drop --enable-preview from Record related tests [v2]
> There are plenty of tests failing on many architectures due to > `--enable-preview` VM code introduced by Loom. This improvements eliminates > some of the redundant `--enable-preview` clauses from the Record tests, since > Records have been graduated from preview in JDK 16. > > Additional testing: > - [x] Linux x86_64 fastdebug, affected tests still pass > - [x] Linux x86_32 fastdebug, affected tests start to pass Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8626/files - new: https://git.openjdk.java.net/jdk/pull/8626/files/d5f0b1be..626e673a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8626&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8626&range=00-01 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8626.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8626/head:pull/8626 PR: https://git.openjdk.java.net/jdk/pull/8626
Re: RFR: 8286473: Drop --enable-preview from Record related tests [v2]
On Tue, 10 May 2022 14:43:06 GMT, Alan Bateman wrote:
>> Aleksey Shipilev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review comments
>
> test/jdk/java/nio/Buffer/BulkPutBuffer.java line 57:
>
>> 55: * @run testng/othervm BulkPutBuffer
>> 56: */
>> 57: public class BulkPutBuffer {
>
> You can drop the -source option from these tests too.
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/8626
Re: RFR: 8286473: Drop --enable-preview from Record related tests [v2]
On Tue, 10 May 2022 14:51:52 GMT, Aleksey Shipilev wrote: >> There are plenty of tests failing on many architectures due to >> `--enable-preview` VM code introduced by Loom. This improvements eliminates >> some of the redundant `--enable-preview` clauses from the Record tests, >> since Records have been graduated from preview in JDK 16. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, affected tests still pass >> - [x] Linux x86_32 fastdebug, affected tests start to pass > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8626
Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests [v2]
> There are plenty of tests failing on many architectures due to > `--enable-preview` VM code introduced by Loom. This improvement eliminates > some of the redundant `--enable-preview` clauses from the Sealed Classes > tests, since Sealed Classes have been graduated from preview in JDK 17. > > Additional testing: > - [x] Linux x86_64 fastdebug, affected tests still pass > - [x] Linux x86_32 fastdebug, affected tests start to pass Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8627/files - new: https://git.openjdk.java.net/jdk/pull/8627/files/9b1a55a3..1b74acf4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8627&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8627&range=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8627.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8627/head:pull/8627 PR: https://git.openjdk.java.net/jdk/pull/8627
Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests [v2]
On Tue, 10 May 2022 14:54:39 GMT, Aleksey Shipilev wrote: >> There are plenty of tests failing on many architectures due to >> `--enable-preview` VM code introduced by Loom. This improvement eliminates >> some of the redundant `--enable-preview` clauses from the Sealed Classes >> tests, since Sealed Classes have been graduated from preview in JDK 17. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, affected tests still pass >> - [x] Linux x86_32 fastdebug, affected tests start to pass > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8627
Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests [v2]
On Tue, 10 May 2022 12:47:08 GMT, Alan Bateman wrote:
>> Aleksey Shipilev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review comments
>
> test/jdk/java/lang/reflect/sealed_classes/SealedClassesReflectionTest.java
> line 29:
>
>> 27: * @summary reflection test for sealed classes
>> 28: * @compile -source ${jdk.version} SealedClassesReflectionTest.java
>> 29: * @run testng/othervm SealedClassesReflectionTest
>
> You should be able to drop` -source ${jdk.version}` too. It was required when
> compiling with `--enable-preview`.
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/8627
Re: RFR: 8286473: Drop --enable-preview from Record related tests [v2]
On Tue, 10 May 2022 14:54:39 GMT, Aleksey Shipilev wrote: >> There are plenty of tests failing on many architectures due to >> `--enable-preview` VM code introduced by Loom. This improvements eliminates >> some of the redundant `--enable-preview` clauses from the Record tests, >> since Records have been graduated from preview in JDK 16. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, affected tests still pass >> - [x] Linux x86_32 fastdebug, affected tests start to pass > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Marked as reviewed by jpai (Committer). - PR: https://git.openjdk.java.net/jdk/pull/8626
Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests [v2]
On Tue, 10 May 2022 14:58:15 GMT, Aleksey Shipilev wrote: >> There are plenty of tests failing on many architectures due to >> `--enable-preview` VM code introduced by Loom. This improvement eliminates >> some of the redundant `--enable-preview` clauses from the Sealed Classes >> tests, since Sealed Classes have been graduated from preview in JDK 17. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, affected tests still pass >> - [x] Linux x86_32 fastdebug, affected tests start to pass > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Marked as reviewed by jpai (Committer). - PR: https://git.openjdk.java.net/jdk/pull/8627
Integrated: 8286363: BigInteger.parallelMultiply missing @since 19
On Mon, 9 May 2022 15:26:20 GMT, Brian Burkhalter wrote: > Add missing `@since 19` tag. This pull request has now been integrated. Changeset: 04bba07d Author:Brian Burkhalter URL: https://git.openjdk.java.net/jdk/commit/04bba07d6588cb96e371f3acdb49d735c9e6536d Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8286363: BigInteger.parallelMultiply missing @since 19 Reviewed-by: alanb, darcy - PR: https://git.openjdk.java.net/jdk/pull/8598
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
On Tue, 10 May 2022 03:21:21 GMT, Joe Darcy wrote:
>> Raffaello Giulietti has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 4511638: Double.toString(double) sometimes produces incorrect results
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 882:
>
>> 880: try {
>> 881: FloatToDecimal.appendTo(f, this);
>> 882: } catch (IOException ignored) {
>
> What is the motivation for wrapping with IOException?
`[Float|Double]ToDecimal` do not have access to `AbstractStringBuilder`, so
have to fail over `Appendable`, which can throw `IOException` in `append(*)`
methods.
I have to find another way if this wrapping to make the compiler happy is
unacceptable.
-
PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 8286473: Drop --enable-preview from Record related tests [v2]
On Tue, 10 May 2022 14:54:39 GMT, Aleksey Shipilev wrote: >> There are plenty of tests failing on many architectures due to >> `--enable-preview` VM code introduced by Loom. This improvements eliminates >> some of the redundant `--enable-preview` clauses from the Record tests, >> since Records have been graduated from preview in JDK 16. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, affected tests still pass >> - [x] Linux x86_32 fastdebug, affected tests start to pass > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8626
Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests [v2]
On Tue, 10 May 2022 14:58:15 GMT, Aleksey Shipilev wrote: >> There are plenty of tests failing on many architectures due to >> `--enable-preview` VM code introduced by Loom. This improvement eliminates >> some of the redundant `--enable-preview` clauses from the Sealed Classes >> tests, since Sealed Classes have been graduated from preview in JDK 17. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, affected tests still pass >> - [x] Linux x86_32 fastdebug, affected tests start to pass > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8627
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Tue, 10 May 2022 14:02:14 GMT, Alan Bateman wrote: > > @LanceAndersen @AlanBateman do you think adding the entry name in the > > exception in ZipFileSystem is ok? If so, should it maybe go into a > > different patch? > > It should be okay as this is the name of an entry in the zip file. It might > be a bit cleaner to add a method to IndexNode to return the name as String. > Alternatively maybe its toString could be changed to drop the index (I would > need to dig into the history to find out if there is really any use for the > index in the String representation). I think this would be OK, but would get to get someone from our security team to bless it. It might not be a bad idea to add a method to return the name as a String. There are a couple of places where we do a new String(name) so would economize any future changes - PR: https://git.openjdk.java.net/jdk/pull/8616
Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests [v2]
On Tue, 10 May 2022 14:58:15 GMT, Aleksey Shipilev wrote: >> There are plenty of tests failing on many architectures due to >> `--enable-preview` VM code introduced by Loom. This improvement eliminates >> some of the redundant `--enable-preview` clauses from the Sealed Classes >> tests, since Sealed Classes have been graduated from preview in JDK 17. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, affected tests still pass >> - [x] Linux x86_32 fastdebug, affected tests start to pass > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8627
Re: RFR: 8282701: Use Class.getInterfaces(false) where possible to reduce allocation pressure [v2]
On Wed, 4 May 2022 09:46:00 GMT, Сергей Цыпанов wrote: >> `Class.getInterfaces(false)` does not clone underlying array and can be used >> in cases when the returned array is only read from. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8282701: Revert some irrelevant changes Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7714
Re: HttpClient has no explicit way of releasing threads
The issue would still exist as the selector thread is not spawned from this executor and also lives until the outer reference is garbage collected. In my case, this quickly resulted in a few hundred threads from a parameterized test. Also, I think that its unlikely that a drive-by instance would be initiated with an explicit instance. I understand it's not easily solved or avoided. For now, an explicit warning in the javadoc might be the most sensitive solution. Remi Forax schrieb am Di., 10. Mai 2022, 15:39: > - Original Message - > > From: "Rafael Winterhalter" > > To: "core-libs-dev" > > Sent: Monday, May 9, 2022 11:43:49 PM > > Subject: HttpClient has no explicit way of releasing threads > > > Hello, > > Hello, > > > > > looking at thread dumps, I realized that the HttpClient implementation > does > > not offer an explicit way to release its threads. Currently, the client: > > > > 1. maintains a cached thread pool with a retention size of 60 seconds. If > > many such clients are created for short lived application, these threads > > pile up. > > 2. has a selector thread that only shuts down if the outer "facade" > > reference is collected via a weak reference. If an application is not > > running GC, this can take a while. > > > > This is not a big problem but I have seen peaks with suddenly many, many > > threads (in test code) where many HttpClients were created for single use > > and I was wondering if it was ever considered to add a method for > disposing > > the threads explicitly? > > You can change the Executor (it's one parameter of the builder) to use > whatever executors you want so you can shutdown that executor as you want. > This should fixed (1). > > Also once you update to Java 19/21, it seems a good scenario to test the > executor that spawn virtual threads instead of platform threads. > > > > > Alternatively, it might be an option to add a method like > > HttpClient.shared() which would return a singleton HttpClient (created on > > the first call, collected if no reference is kept anymore but reused in > the > > meantime) to address such scenarios. I can of course add a singleton in > my > > test project but I find this a bit awkward as it is something to remember > > and to repeat in all applications we maintain. Therefore, it might be > > convenient to add such methods for tests that usually aim to be > decoupled. > > > > Thanks for your consideration, > > best regards, Rafael > > regards, > Rémi >
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Tue, 10 May 2022 16:30:01 GMT, Lance Andersen wrote: > > > @LanceAndersen @AlanBateman do you think adding the entry name in the > > > exception in ZipFileSystem is ok? If so, should it maybe go into a > > > different patch? > > > > > > It should be okay as this is the name of an entry in the zip file. It might > > be a bit cleaner to add a method to IndexNode to return the name as String. > > Alternatively maybe its toString could be changed to drop the index (I > > would need to dig into the history to find out if there is really any use > > for the index in the String representation). > > I think this would be OK, but would get to get someone from our security team > to bless it. > > It might not be a bad idea to add a method to return the name as a String. > There are a couple of places where we do a new String(name) so would > economize any future changes Sounds fair. @LanceAndersen, can you please ask the security team about their ok then and let me know? In case their answer is a yes, I'll work on implementing the suggestion to return the name as String. Shall I maybe do the zipfs change in a different PR then? The more important change in the context of javac is printing out the jar name in javac itself. - PR: https://git.openjdk.java.net/jdk/pull/8616
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Tue, 10 May 2022 16:48:30 GMT, Christoph Langer wrote: > I think this would be OK, but would get to get someone from our security team > to bless it. It's print the entry name, I don't think it is leaking the file path to the zip file. - PR: https://git.openjdk.java.net/jdk/pull/8616
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Tue, 10 May 2022 16:30:01 GMT, Lance Andersen wrote: >>> @LanceAndersen @AlanBateman do you think adding the entry name in the >>> exception in ZipFileSystem is ok? If so, should it maybe go into a >>> different patch? >> >> It should be okay as this is the name of an entry in the zip file. It might >> be a bit cleaner to add a method to IndexNode to return the name as String. >> Alternatively maybe its toString could be changed to drop the index (I would >> need to dig into the history to find out if there is really any use for the >> index in the String representation). > >> > @LanceAndersen @AlanBateman do you think adding the entry name in the >> > exception in ZipFileSystem is ok? If so, should it maybe go into a >> > different patch? >> >> It should be okay as this is the name of an entry in the zip file. It might >> be a bit cleaner to add a method to IndexNode to return the name as String. >> Alternatively maybe its toString could be changed to drop the index (I would >> need to dig into the history to find out if there is really any use for the >> index in the String representation). > > I think this would be OK, but would get to get someone from our security team > to bless it. > > It might not be a bad idea to add a method to return the name as a String. > There are a couple of places where we do a new String(name) so would > economize any future changes > > > > @LanceAndersen @AlanBateman do you think adding the entry name in the > > > > exception in ZipFileSystem is ok? If so, should it maybe go into a > > > > different patch? > > > > > > > > > It should be okay as this is the name of an entry in the zip file. It > > > might be a bit cleaner to add a method to IndexNode to return the name as > > > String. Alternatively maybe its toString could be changed to drop the > > > index (I would need to dig into the history to find out if there is > > > really any use for the index in the String representation). > > > > > > I think this would be OK, but would get to get someone from our security > > team to bless it. > > It might not be a bad idea to add a method to return the name as a String. > > There are a couple of places where we do a new String(name) so would > > economize any future changes > > Sounds fair. @LanceAndersen, can you please ask the security team about their > ok then and let me know? In case their answer is a yes, I'll work on > implementing the suggestion to return the name as String. Shall I maybe do > the zipfs change in a different PR then? The more important change in the > context of javac is printing out the jar name in javac itself. Already did ;-) so hopefully they will share their thoughts soon. - PR: https://git.openjdk.java.net/jdk/pull/8616
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Tue, 10 May 2022 16:51:35 GMT, Alan Bateman wrote: > > I think this would be OK, but would get to get someone from our security > > team to bless it. > > It's print the entry name, I don't think it is leaking the file path to the > zip file. I think you are probably right I am probably being overly cautious - PR: https://git.openjdk.java.net/jdk/pull/8616
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Tue, 10 May 2022 16:58:03 GMT, Lance Andersen wrote: >>> I think this would be OK, but would get to get someone from our security >>> team to bless it. >> >> It's print the entry name, I don't think it is leaking the file path to the >> zip file. > >> > I think this would be OK, but would get to get someone from our security >> > team to bless it. >> >> It's print the entry name, I don't think it is leaking the file path to the >> zip file. > > I think you are probably right I am probably being overly cautious > > > > > @LanceAndersen @AlanBateman do you think adding the entry name in the > > > > > exception in ZipFileSystem is ok? If so, should it maybe go into a > > > > > different patch? > > > > > > > > > > > > It should be okay as this is the name of an entry in the zip file. It > > > > might be a bit cleaner to add a method to IndexNode to return the name > > > > as String. Alternatively maybe its toString could be changed to drop > > > > the index (I would need to dig into the history to find out if there is > > > > really any use for the index in the String representation). > > > > > > > > > I think this would be OK, but would get to get someone from our security > > > team to bless it. > > > It might not be a bad idea to add a method to return the name as a > > > String. There are a couple of places where we do a new String(name) so > > > would economize any future changes > > > > > > Sounds fair. @LanceAndersen, can you please ask the security team about > > their ok then and let me know? In case their answer is a yes, I'll work on > > implementing the suggestion to return the name as String. Shall I maybe do > > the zipfs change in a different PR then? The more important change in the > > context of javac is printing out the jar name in javac itself. > > Already did ;-) so hopefully they will share their thoughts soon. I think it would probably be good for a separate PR for the ZipFS change as it keeps it a bit clearer - PR: https://git.openjdk.java.net/jdk/pull/8616
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v6]
On Tue, 10 May 2022 06:52:00 GMT, Shruthi wrote: >> Removing the Duplicate keys present in XSLTErrorResources.java and >> XPATHErrorResources.java >> >> The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097 > > Shruthi has updated the pull request incrementally with one additional commit > since the last revision: > > update copyright header src/java.xml/share/classes/com/sun/org/apache/xpath/internal/res/XPATHErrorResources_es.java line 2: > 1: /* > 2: * Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights > reserved. Remove "2019, ", same with other classes. The header (along with the LastModified tag) serves as an indication that you've modified the file. As this is the first of such modification, it just needs to be noted with the year it's done, that is "2022, ". - PR: https://git.openjdk.java.net/jdk/pull/8318
Re: HttpClient has no explicit way of releasing threads
Il 10/05/2022 15:39, Remi Forax ha scritto: You can change the Executor (it's one parameter of the builder) to use whatever executors you want so you can shutdown that executor as you want. This should fixed (1). Also once you update to Java 19/21, it seems a good scenario to test the executor that spawn virtual threads instead of platform threads. I am afraid build 19-ea+21-1482 doesn't yet include virtual threads, but it's getting close. Be careful though. The HttpClient.Builder has an enticing method HttpClient.Builder executor(Executor executor) which "Sets the executor to be used for asynchronous and dependent tasks." What this means is anyone's guess, but I know from Heinz Kabutz that it's the executor used for the internal workings of the selector mechanism. See https://bugs.openjdk.java.net/browse/JDK-8204679. I don't think making that a virtual thread does much good. If you want to spawn a virtual thread to process the request (a good idea if the processing blocks), you should use client.sendAsync(request, responseBodyHandler) .thenApplyAsync(HttpResponse::body, executor); where executor provides virtual threads. Cheers, Cay -- Cay S. Horstmann | http://horstmann.com | mailto:c...@horstmann.com
Integrated: 8282701: Use Class.getInterfaces(false) where possible to reduce allocation pressure
On Sat, 5 Mar 2022 13:07:56 GMT, Сергей Цыпанов wrote: > `Class.getInterfaces(false)` does not clone underlying array and can be used > in cases when the returned array is only read from. This pull request has now been integrated. Changeset: 9073a98d Author:Sergey Tsypanov Committer: Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/9073a98d5791dedc5ed4156ec5229164ed1eef50 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8282701: Use Class.getInterfaces(false) where possible to reduce allocation pressure Reviewed-by: redestad, mchung - PR: https://git.openjdk.java.net/jdk/pull/7714
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support > second-level resolution, enabling round trips with `java.time.ZoneOffset`s. > Corresponding CSR is also being drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed offsets in milliseconds, added test variations, refined Custom ID definitions - Changes: - all: https://git.openjdk.java.net/jdk/pull/8606/files - new: https://git.openjdk.java.net/jdk/pull/8606/files/843e86be..81a806e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8606&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8606&range=00-01 Stats: 13 lines in 2 files changed: 3 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/8606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8606/head:pull/8606 PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Tue, 10 May 2022 13:12:10 GMT, Jaikiran Pai wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed offsets in milliseconds, added test variations, refined Custom ID
>> definitions
>
> src/java.base/share/classes/java/util/TimeZone.java line 109:
>
>> 107: *
>> 108: * NormalizedCustomID:
>> 109: * {@code GMT} Sign TwoDigitHours {@code :}
>> Minutes [Seconds]
>
> Hello Naoto,
>
> Should this instead be: `... Minutes [{@code :} Seconds]` -
> i.e. should it have the `:` literal if seconds are present in the custom
> timezone id?
The colon is included in `Seconds` part below. I changed the part name to
`ColonSeconds` to make it clearer.
> src/java.base/share/classes/java/util/TimeZone.java line 543:
>
>> 541: return new ZoneInfo(totalSecs == 0 ? "UTC" : GMT_ID + tzid,
>> totalSecs);
>> 542: } else {
>> 543: return getTimeZone(tzid, true);
>
> Before the change in this PR, we used to prefix `GMT` to (non-custom timezone
> ids) if the timezone id returned by `ZoneId#getId()` started with the `+` or
> `-` sign, before calling `getTimeZone(modifiedTzid, true)`.
> With this change, for `ZoneId`s that aren't `ZoneOffset` instance, we now
> call `getTimeZone(originalTzid, true)`, without first checking/prefixing the
> id with `GMT`. Is that an intentional change and would that potentially cause
> `getTimeZone(String, boolean)` to return a different result?
Yes, it is intentional. The `Time-zone IDs` section in the `ZoneId` class
description is clear that zone id starting with "+/-" is a `ZoneOffset`
instance. Other ZoneIds should have offsets with prefix or region-based ids.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]
On Mon, 9 May 2022 22:29:50 GMT, Uwe Schindler wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed offsets in milliseconds, added test variations, refined Custom ID
>> definitions
>
> src/java.base/share/classes/java/util/TimeZone.java line 539:
>
>> 537: public static TimeZone getTimeZone(ZoneId zoneId) {
>> 538: String tzid = zoneId.getId(); // throws an NPE if null
>> 539: if (zoneId instanceof ZoneOffset zo) {
>
> I like this because it is much faster than the conversion to ZoneId and
> parsing it back! It is similar to use of SimpleTimeZone, but this is better
> as the returned timezone is unmodifiable, correct?
Yes, and it aligns with the other call site (line 588).
> test/jdk/java/util/TimeZone/ZoneOffsetRoundTripTest.java line 43:
>
>> 41: private Object[][] testZoneOffsets() {
>> 42: return new Object[][] {
>> 43: {ZoneId.of("Z"), 0},
>
> I know, `ZoneId.of()` should parse this as a `ZoneOffset` and return a
> `ZoneOffset` instance, but maybe add also the other string variants with
> prefix (`ZoneId.of("UTC+00:00:01")` or `ZoneId.of("GMT+00:00:01")` as data
> items. Maybe also use `ZoneOffset.of()` for the plain zones to be explicit.
Added them except "UTC+...", as it is not recognizable as a Custom ID.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
On Tue, 10 May 2022 15:50:26 GMT, Raffaello Giulietti wrote: > `[Float|Double]ToDecimal` do not have access to `AbstractStringBuilder`, so > have to fail over `Appendable`, which can throw `IOException` in `append(*)` > methods. > > I have to find another way if this wrapping to make the compiler happy is > unacceptable. Ah, I haven't used it myself recently enough to recall the details, but I believe the shared secrets mechanism is intended to allow such cross-package access. If the current code is kept and if the exception can actually be thrown, explicitly throwing an assertion error is preferable to "assert false". - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 8286368: Cleanup problem lists after loom integration [v2]
> 8286368: Cleanup problem lists after loom integration Leonid Mesnik has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge - 8286368: Cleanup problem lists after loom integration - Changes: https://git.openjdk.java.net/jdk/pull/8604/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8604&range=01 Stats: 29 lines in 4 files changed: 1 ins; 22 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8604.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8604/head:pull/8604 PR: https://git.openjdk.java.net/jdk/pull/8604
Integrated: 8286368: Cleanup problem lists after loom integration
On Mon, 9 May 2022 18:17:13 GMT, Leonid Mesnik wrote: > 8286368: Cleanup problem lists after loom integration This pull request has now been integrated. Changeset: dcec1d2a Author:Leonid Mesnik URL: https://git.openjdk.java.net/jdk/commit/dcec1d2a68e2c82e27174c3dc52bb17316530966 Stats: 29 lines in 4 files changed: 1 ins; 22 del; 6 mod 8286368: Cleanup problem lists after loom integration Reviewed-by: alanb - PR: https://git.openjdk.java.net/jdk/pull/8604
Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults
On Fri, 6 May 2022 17:56:44 GMT, Alan Bateman wrote:
>> JDK-6725221 Standardize obtaining boolean properties with defaults
>
> src/java.base/share/classes/java/lang/reflect/AccessibleObject.java line 777:
>
>> 775: if (!printStackPropertiesSet && VM.initLevel() >= 1) {
>> 776: printStackWhenAccessFails = GetBooleanAction.
>> 777:
>> privilegedGetProperty("sun.reflect.debugModuleAccessChecks");
>
> Running with `-Dsun.reflect.debugModuleAccessChecks` should set
> printStackWhenAccessFails to true, not false.
You are right. The old way maps the null string to true, and the new way maps
it to false. I did not notice that. At this point, I see no value in making the
"true".equals and "false".equals changes. Too much can break. I'll reverse
these changes.
-
PR: https://git.openjdk.java.net/jdk/pull/8559
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam wrote:
> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never
> supported the value `"rw"` since the source code was imported to the openjdk
> repo more than 15 years ago. In fact HotSpot throws
> `IllegalArgumentException` when such a mode is specified.
>
> It's unlikely such a mode will be required for future enhancements. Support
> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"`
> is the only supported mode.
>
> I also cleaned up related code in the JDK and HotSpot.
>
> Testing:
> - Passed tiers 1 and 2
> - Tiers 3, 4, 5 are in progress
src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/file/PerfDataBuffer.java
line 60:
> 58: FileChannel fc = new RandomAccessFile(f, "r").getChannel();
> 59: ByteBuffer bb = fc.map(FileChannel.MapMode.READ_ONLY, 0L,
> (int)fc.size());
> 60: fc.close(); // doesn't need to remain open
I think you can change this to:
try (FileChannel fc = FileChannel.open(f.toPath())) {
ByteBuffer bb = ...
createPerfDataBuffer(bb, 0);
}
-
PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam wrote: > The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never > supported the value `"rw"` since the source code was imported to the openjdk > repo more than 15 years ago. In fact HotSpot throws > `IllegalArgumentException` when such a mode is specified. > > It's unlikely such a mode will be required for future enhancements. Support > for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` > is the only supported mode. > > I also cleaned up related code in the JDK and HotSpot. > > Testing: > - Passed tiers 1 and 2 > - Tiers 3, 4, 5 are in progress I skimmed through the changes and I think they look okay. In the distant past there were tools outside of the JDK that used the jvmstat API directly. It's possible that VisualVM still does but it would only compile/run if --add-exports is used to export the sun.jvmstat.* packages. So it might be that dropping the parameter from a method in RemoteHost is noticed and I think that is okay because this package is not exported and is not meant to be used by code outside of the JDK. - PR: https://git.openjdk.java.net/jdk/pull/8622
RFR: 8286287: Reading file as UTF-16 causes Error which "shouldn't happen"
`String.decodeWithDecoder()` method requires the `CharsetDecoder` parameter replaces on malformed/unmappable characters with replacements. However, there was a code path that lacked to set the `CodingErrorAction.REPLACE` on the decoder. Unrelated, one typo in a test was also fixed. - Commit messages: - 8286287: Reading file as UTF-16 causes Error which "shouldn't happen" Changes: https://git.openjdk.java.net/jdk/pull/8640/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8640&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286287 Stats: 55 lines in 3 files changed: 52 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8640.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8640/head:pull/8640 PR: https://git.openjdk.java.net/jdk/pull/8640
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Mon, 9 May 2022 10:28:27 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This PR updates the VM implementation of the foreign linker, by bringing
>> over commits from the panama-foreign repo.
>>
>> This is split off from the main JEP integration for 19, since we have
>> limited resources to handle this. As such, this PR might fall over to 20.
>>
>> I've written up an overview of the Linker architecture here:
>> http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful
>> to read that first.
>>
>> This patch moves from the "legacy" implementation, to what is currently
>> implemented in the panama-foreign repo, except for replacing the use of
>> method handle combinators with ASM. That will come in a later path. To
>> recap. This PR contains the following changes:
>>
>> 1. VM stubs for downcalls are now generated up front, instead of lazily by
>> C2 [1].
>> 2. the VM support for upcalls/downcalls now support all possible call
>> shapes. And VM stubs and Java code implementing the buffered invocation
>> strategy has been removed [2], [3], [4], [5].
>> 3. The existing C2 intrinsification support for the `linkToNative` method
>> handle linker was no longer needed and has been removed [6] (support might
>> be re-added in another form later).
>> 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now
>> implements RuntimeBlob directly. Binding to java classes has been rewritten
>> to use javaClasses.h/cpp (this wasn't previously possible due to these java
>> classes being in an incubator module) [7], [8], [9].
>>
>> While the patch mostly consists of VM changes, there are also some Java
>> changes to support (2).
>>
>> The original commit structure has been mostly retained, so it might be
>> useful to look at a specific commit, or the corresponding patch in the
>> [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr)
>> repo as well. I've also left some inline comments to explain some of the
>> changes, which will hopefully make reviewing easier.
>>
>> Testing: Tier1-4
>>
>> Thanks,
>> Jorn
>>
>> [1]:
>> https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358
>> [2]:
>> https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49
>> [3]:
>> https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3
>> [4]:
>> https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3
>> [5]:
>> https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120
>> [6]:
>> https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a
>> [7]:
>> https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062
>> [8]:
>> https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f
>> [9]:
>> https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9
>
> Jorn Vernee has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 21 commits:
>
> - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
> - Remove unneeded ComputeMoveOrder
> - Remove comment about native calls in lcm.cpp
> - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>
>Reviewed-by: jvernee, mcimadamore
> - Update riscv and arm stubs
> - Remove spurious ProblemList change
> - Pass pointer to LogStream
> - Polish
> - Replace TraceNativeInvokers flag with unified logging
> - Fix other platforms, take 2
> - ... and 11 more:
> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
Nice work! Looks good. Some minor comments/questions follow.
src/hotspot/cpu/aarch64/frame_aarch64.cpp line 379:
> 377: // need unextended_sp here, since normal sp is wrong for interpreter
> callees
> 378: return reinterpret_cast(
> 379: reinterpret_cast(frame.unextended_sp()) +
> in_bytes(_frame_data_offset));
Maybe use `address` instead of `char*`?
src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5531:
> 5529: }
> 5530:
> 5531: // On 64 bit we will store integer like items to the stack as
Time for a cleanup? `64 bit` vs `64bit`, `abi`, `Aarch64`.
src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5547:
> 5545: } else if (dst.first()->is_stack()) {
> 5546: // reg to stack
> 5547: // Do we really have to sign extend???
Obsolete? Remove?
src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 306:
> 304: intptr_t exception_handler_offset = __ pc() - start;
> 305:
> 306: // Native caller has no idea how to handle exceptions,
Can you elaborate, please, how it is expected to work in presence of
asynchronous exceptions? I'd expect to see a code which unconditionally clears
pending exception with an assertion that verifies that the exception is of
expected type.
src/hotspot/cpu/x86/foreign_globals_x86.
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Mon, 28 Mar 2022 12:15:10 GMT, Jorn Vernee wrote: >> Jorn Vernee has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 21 commits: >> >> - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2 >> - Remove unneeded ComputeMoveOrder >> - Remove comment about native calls in lcm.cpp >> - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 >> >>Reviewed-by: jvernee, mcimadamore >> - Update riscv and arm stubs >> - Remove spurious ProblemList change >> - Pass pointer to LogStream >> - Polish >> - Replace TraceNativeInvokers flag with unified logging >> - Fix other platforms, take 2 >> - ... and 11 more: >> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91 > > src/hotspot/share/utilities/growableArray.hpp line 151: > >> 149: return _data; >> 150: } >> 151: > > This accessor is added to be able to temporarily view a stable GrowableArray > instance as a C-style array. It is used to by `NativeCallConv` and > `RegSpiller` in `foreign_globals.hpp`. > > GrowableArray already has an `adr_at` accessor that does something similar, > but using `adr_at(0)` fails on empty growable arrays since it also performs a > bounds check. So it can not be used. Any problems with migrating `CallConv` and `RegSpiller`away from ` VMReg* + int` to `GrowableArray`? - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Tue, 10 May 2022 16:59:41 GMT, Lance Andersen wrote: > > > > > @LanceAndersen @AlanBateman do you think adding the entry name in the > > > > > exception in ZipFileSystem is ok? If so, should it maybe go into a > > > > > different patch? > > > > > > > > > > > > It should be okay as this is the name of an entry in the zip file. It > > > > might be a bit cleaner to add a method to IndexNode to return the name > > > > as String. Alternatively maybe its toString could be changed to drop > > > > the index (I would need to dig into the history to find out if there is > > > > really any use for the index in the String representation). > > > > > > > > > I think this would be OK, but would get to get someone from our security > > > team to bless it. > > > It might not be a bad idea to add a method to return the name as a > > > String. There are a couple of places where we do a new String(name) so > > > would economize any future changes > > > > > > Sounds fair. @LanceAndersen, can you please ask the security team about > > their ok then and let me know? In case their answer is a yes, I'll work on > > implementing the suggestion to return the name as String. Shall I maybe do > > the zipfs change in a different PR then? The more important change in the > > context of javac is printing out the jar name in javac itself. > > Already did ;-) so hopefully they will share their thoughts soon. It's probably ok, but the bug report is either incomplete or I am missing something. It says "This can be improved to something like: ..." but the same text as is emitted now is used. Can you fix this so I have a better example of what will be included in the message? - PR: https://git.openjdk.java.net/jdk/pull/8616
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Tue, 10 May 2022 21:26:42 GMT, Sean Mullan wrote: > It's probably ok, but the bug report is either incomplete or I am missing > something. It says "This can be improved to something like: ..." but the same > text as is emitted now is used. Can you fix this so I have a better example > of what will be included in the message? The bug report also says "The error message does not give a clue which jar file is causing the problem" but the error message includes the name "invalid.jar" so I am also confused about that. - PR: https://git.openjdk.java.net/jdk/pull/8616
RFR: 8286378: Address possibly lossy conversions in java.base
PR#8599 8244681: proposes to add compiler warnings for possible lossy conversions >From the CSR: "If the type of the right-hand operand of a compound assignment is not assignment compatible with the type of the variable, a cast is implied and possible lossy conversion may silently occur. While similar situations are resolved as compilation errors for primitive assignments, there are no similar rules defined for compound assignments." This PR anticipates the warnings and adds explicit casts to replace the implicit casts. In most cases, the cast matches the type of the right-hand side to type of the compound assignment. Since these casts are truncating the value, there might be a different refactoring that avoids the cast but a direct replacement was chosen here. (Advise and suggestions will inform similar updates to other OpenJDK modules). - Commit messages: - 8286378: Address possibly lossy conversions in java.base Changes: https://git.openjdk.java.net/jdk/pull/8642/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8642&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286378 Stats: 57 lines in 33 files changed: 1 ins; 3 del; 53 mod Patch: https://git.openjdk.java.net/jdk/pull/8642.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642 PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam wrote: > The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never > supported the value `"rw"` since the source code was imported to the openjdk > repo more than 15 years ago. In fact HotSpot throws > `IllegalArgumentException` when such a mode is specified. > > It's unlikely such a mode will be required for future enhancements. Support > for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` > is the only supported mode. > > I also cleaned up related code in the JDK and HotSpot. > > Testing: > - Passed tiers 1 and 2 > - Tiers 3, 4, 5 are in progress Nice cleanup! (Some stylistic suggestions inline, feel free to ignore) src/hotspot/os/windows/perfMemory_windows.cpp line 1781: > 1779: // address space. > 1780: // > 1781: void PerfMemory::attach(const char* user, int vmid, One line? src/hotspot/share/prims/perf.cpp line 84: > 82: > 83: // attach to the PerfData memory region for the specified VM > 84: PerfMemory::attach(user_utf, vmid, One line? src/hotspot/share/runtime/perfMemory.hpp line 146: > 144: // methods for attaching to and detaching from the PerfData > 145: // memory segment of another JVM process on the same system. > 146: static void attach(const char* user, int vmid, One line? src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java line 74: > 72: Integer v = lvmid; > 73: RemoteVm stub = null; > 74: StringBuilder sb = new StringBuilder(); Suggestion: String vmidStr = "local://" + lvmid + "@localhost"; - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs wrote: > PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). Looks good. Assuming copyright years will be updated. src/java.base/share/classes/java/time/es line 1: > 1: X Looks like a random file was added by accident? - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs wrote: > PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). The update in security area looks good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8262889: Compiler implementation for Record Patterns [v5]
On Tue, 10 May 2022 09:57:48 GMT, Jan Lahoda wrote: >> 8262889: Compiler implementation for Record Patterns >> >> A first version of a patch that introduces record patterns into javac as a >> preview feature. For the specification, please see: >> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html >> >> There are two notable tricky parts: >> -in the parser, it was necessary to improve the `analyzePattern` method to >> handle nested/record patterns, while still keeping error recovery reasonable >> -in the `TransPatterns`, the desugaring of the record patterns is very >> straightforward - effectivelly the record patterns are desugared into >> guards/conditions. This will likely be improved in some future >> version/preview >> >> `MatchException` has been extended to cover additional cases related to >> record patterns. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing (non-)exhaustiveness for enum. looks good to me, great job! - Marked as reviewed by vromero (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs wrote: > PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). IO, math, and NIO changes look fine. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]
> PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: remove stray file - Changes: - all: https://git.openjdk.java.net/jdk/pull/8642/files - new: https://git.openjdk.java.net/jdk/pull/8642/files/e72ce85c..7ff806a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8642&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8642&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8642.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642 PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
On Tue, 10 May 2022 21:43:44 GMT, Claes Redestad wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> review comments > > src/hotspot/os/windows/perfMemory_windows.cpp line 1781: > >> 1779: // address space. >> 1780: // >> 1781: void PerfMemory::attach(const char* user, int vmid, > > One line? Fixed > src/hotspot/share/prims/perf.cpp line 84: > >> 82: >> 83: // attach to the PerfData memory region for the specified VM >> 84: PerfMemory::attach(user_utf, vmid, > > One line? Fixed > src/hotspot/share/runtime/perfMemory.hpp line 146: > >> 144: // methods for attaching to and detaching from the PerfData >> 145: // memory segment of another JVM process on the same system. >> 146: static void attach(const char* user, int vmid, > > One line? Fixed > src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java line 74: > >> 72: Integer v = lvmid; >> 73: RemoteVm stub = null; >> 74: StringBuilder sb = new StringBuilder(); > > Suggestion: > > String vmidStr = "local://" + lvmid + "@localhost"; Fixed - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never > supported the value `"rw"` since the source code was imported to the openjdk > repo more than 15 years ago. In fact HotSpot throws > `IllegalArgumentException` when such a mode is specified. > > It's unlikely such a mode will be required for future enhancements. Support > for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` > is the only supported mode. > > I also cleaned up related code in the JDK and HotSpot. > > Testing: > - Passed tiers 1 and 2 > - Tiers 3, 4, 5 are in progress Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8622/files - new: https://git.openjdk.java.net/jdk/pull/8622/files/22c22c30..34a01f71 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8622&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8622&range=00-01 Stats: 14 lines in 5 files changed: 1 ins; 7 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8622.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8622/head:pull/8622 PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
On Tue, 10 May 2022 19:59:41 GMT, Alan Bateman wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments
>
> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/file/PerfDataBuffer.java
> line 60:
>
>> 58: FileChannel fc = new RandomAccessFile(f, "r").getChannel();
>> 59: ByteBuffer bb = fc.map(FileChannel.MapMode.READ_ONLY, 0L,
>> (int)fc.size());
>> 60: fc.close(); // doesn't need to remain open
>
> I think you can change this to:
>
>
> try (FileChannel fc = FileChannel.open(f.toPath())) {
> ByteBuffer bb = ...
> createPerfDataBuffer(bb, 0);
> }
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 20:03:45 GMT, Alan Bateman wrote: > I skimmed through the changes and I think they look okay. In the distant past > there were tools outside of the JDK that used the jvmstat API directly. It's > possible that VisualVM still does but it would only compile/run if > --add-exports is used to export the sun.jvmstat.* packages. So it might be > that dropping the parameter from a method in RemoteHost is noticed and I > think that is okay because this package is not exported and is not meant to > be used by code outside of the JDK. The APIs changed by this PR are: - sun.jvmstat.monitor.remote.RemoteHost::attachVm - sun.jvmstat.monitor.VmIdentifier::getMode - sun.jvmstat.monitor.HostIdentifier::getMode - jdk.internal.perf.Perf::attach I checked the latest VisualVM source code. Some of the above classes are referenced, but none of the affected APIs are called. - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v3]
On Mon, 9 May 2022 21:55:27 GMT, Paul Sandoz wrote:
>> Xiaohong Gong has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rename "use_predicate" to "needs_predicate"
>
> I modified the code of this PR to avoid the conversion of `boolean` to `int`,
> so a constant integer value is passed all the way through, and the masked
> load is made intrinsic from the method at which the constants are passed as
> arguments i.e. the public `fromArray` mask accepting method.
Hi @PaulSandoz , thanks for the patch for the constant int parameter. I think
the main change is:
-ByteVector fromArray0Template(Class maskClass, C base, long offset, int
index, M m, boolean offsetInRange,
+ByteVector fromArray0Template(Class maskClass, C base, long offset, int
index, M m, int offsetInRange,
VectorSupport.LoadVectorMaskedOperation defaultImpl) {
m.check(species());
ByteSpecies vsp = vspecies();
-if (offsetInRange) {
-return VectorSupport.loadMasked(
-vsp.vectorType(), maskClass, vsp.elementType(),
vsp.laneCount(),
-base, offset, m, /* offsetInRange */ 1,
-base, index, vsp, defaultImpl);
-} else {
-return VectorSupport.loadMasked(
-vsp.vectorType(), maskClass, vsp.elementType(),
vsp.laneCount(),
-base, offset, m, /* offsetInRange */ 0,
-base, index, vsp, defaultImpl);
-}
+return VectorSupport.loadMasked(
+vsp.vectorType(), maskClass, vsp.elementType(), vsp.laneCount(),
+base, offset, m, offsetInRange == 1 ? 1 : 0,
+base, index, vsp, defaultImpl);
}
which uses `offsetInRange == 1 ? 1 : 0`. Unfortunately this could not always
make sure the `offsetInRange` a constant a the compiler time. Again, this
change could also make the assertion fail randomly:
--- a/src/hotspot/share/opto/vectorIntrinsics.cpp
+++ b/src/hotspot/share/opto/vectorIntrinsics.cpp
@@ -1236,6 +1236,7 @@ bool
LibraryCallKit::inline_vector_mem_masked_operation(bool is_store) {
} else {
// Masked vector load with IOOBE always uses the predicated load.
const TypeInt* offset_in_range = gvn().type(argument(8))->isa_int();
+ assert(offset_in_range->is_con(), "must be a constant");
if (!offset_in_range->is_con()) {
if (C->print_intrinsics()) {
tty->print_cr(" ** missing constant: offsetInRange=%s",
Sometimes, the compiler can parse it a constant. I think this depends on the
compiler OSR and speculative optimization. Did you try an example with IOOBE on
a non predicated hardware?
Here is the main code of my unittest to reproduce the issue:
static final VectorSpecies I_SPECIES = IntVector.SPECIES_128;
static final int LENGTH = 1026;
public static int[] ia;
public static int[] ib;
private static void init() {
for (int i = 0; i < LENGTH; i++) {
ia[i] = i;
ib[i] = 0;
}
for (int i = 0; i < 2; i++) {
m[i] = i % 2 == 0;
}
}
private static void func() {
VectorMask mask = VectorMask.fromArray(I_SPECIES, m, 0);
for (int i = 0; i < LENGTH; i += vl) {
IntVector av = IntVector.fromArray(I_SPECIES, ia, i, mask);
av.lanewise(VectorOperators.ABS).intoArray(ic, i, mask);
}
}
public static void main(String[] args) {
init();
for (int i = 0; i < 1; i++) {
func();
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v7]
> Removing the Duplicate keys present in XSLTErrorResources.java and > XPATHErrorResources.java > > The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097 Shruthi has updated the pull request incrementally with one additional commit since the last revision: Modify copyright year - Changes: - all: https://git.openjdk.java.net/jdk/pull/8318/files - new: https://git.openjdk.java.net/jdk/pull/8318/files/0cf1f9b9..283f8ef9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8318&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8318&range=05-06 Stats: 7 lines in 7 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/8318.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8318/head:pull/8318 PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]
On Tue, 10 May 2022 23:01:33 GMT, Roger Riggs wrote:
>> PR#8599 8244681: proposes to add compiler warnings for possible lossy
>> conversions
>> From the CSR:
>>
>> "If the type of the right-hand operand of a compound assignment is not
>> assignment compatible with the type of the variable, a cast is implied and
>> possible lossy conversion may silently occur. While similar situations are
>> resolved as compilation errors for primitive assignments, there are no
>> similar rules defined for compound assignments."
>>
>> This PR anticipates the warnings and adds explicit casts to replace the
>> implicit casts.
>> In most cases, the cast matches the type of the right-hand side to type of
>> the compound assignment.
>> Since these casts are truncating the value, there might be a different
>> refactoring that avoids the cast
>> but a direct replacement was chosen here.
>>
>> (Advise and suggestions will inform similar updates to other OpenJDK
>> modules).
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove stray file
src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128:
> 126: // timed poll interrupted so need to adjust timeout
> 127: long adjust = System.nanoTime() - startTime;
> 128: to -= (int)TimeUnit.MILLISECONDS.convert(adjust,
> TimeUnit.NANOSECONDS);
Can we change this `to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);` while
we here?
src/java.base/share/classes/jdk/internal/org/objectweb/asm/Frame.java line 615:
> 613: if (outputStackTop >= elements) {
> 614: outputStackTop -= (short)elements;
> 615: } else {
I think we usually try to avoid changing the ASM code if possible but maybe you
have to do this because the compiler option is used for compiling java.base?
src/java.base/unix/classes/sun/nio/ch/PollSelectorImpl.java line 123:
> 121: // timed poll interrupted so need to adjust timeout
> 122: long adjust = System.nanoTime() - startTime;
> 123: to -= (int)TimeUnit.MILLISECONDS.convert(adjust,
> TimeUnit.NANOSECONDS);
This one can also be changed to:
`to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);`
-
PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests [v2]
On Tue, 10 May 2022 14:58:15 GMT, Aleksey Shipilev wrote: >> There are plenty of tests failing on many architectures due to >> `--enable-preview` VM code introduced by Loom. This improvement eliminates >> some of the redundant `--enable-preview` clauses from the Sealed Classes >> tests, since Sealed Classes have been graduated from preview in JDK 17. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, affected tests still pass >> - [x] Linux x86_32 fastdebug, affected tests start to pass > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8627
Integrated: 8286474: Drop --enable-preview from Sealed Classes related tests
On Tue, 10 May 2022 12:07:31 GMT, Aleksey Shipilev wrote: > There are plenty of tests failing on many architectures due to > `--enable-preview` VM code introduced by Loom. This improvement eliminates > some of the redundant `--enable-preview` clauses from the Sealed Classes > tests, since Sealed Classes have been graduated from preview in JDK 17. > > Additional testing: > - [x] Linux x86_64 fastdebug, affected tests still pass > - [x] Linux x86_32 fastdebug, affected tests start to pass This pull request has now been integrated. Changeset: d547a707 Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/d547a707bf1f9e252213fdab7eaf076b5cf884b4 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod 8286474: Drop --enable-preview from Sealed Classes related tests Reviewed-by: alanb, jpai, mchung, lancea - PR: https://git.openjdk.java.net/jdk/pull/8627
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Tue, 10 May 2022 21:30:23 GMT, Sean Mullan wrote: > > It's probably ok, but the bug report is either incomplete or I am missing > > something. It says "This can be improved to something like: ..." but the > > same text as is emitted now is used. Can you fix this so I have a better > > example of what will be included in the message? > > The bug report also says "The error message does not give a clue which jar > file is causing the problem" but the error message includes the name > "invalid.jar" so I am also confused about that. There are two parts to it. In the case of initCEN method, the proposed change is to include the name of the rejected entry in the exception message. This is not the same thing as leaking a file path in the exception message so I don't think we have a concern here. - PR: https://git.openjdk.java.net/jdk/pull/8616
RFR: 8286562: GCC 12 reports some compiler warnings
I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on Fedora 36. As you can see, the warnings spreads several areas. Let me know if I should separate them by area. * -Wstringop-overflow * src/hotspot/share/oops/array.hpp * src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp In member function 'void Array::at_put(int, const T&) [with T = unsigned char]', inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, inlined from 'void ConstantPool::method_at_put(int, int, int)' at /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, inlined from 'ConstantPool* BytecodeConstantPool::create_constant_pool(JavaThread*) const' at /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: - Commit messages: - 8286562: GCC 12 reports some compiler warnings Changes: https://git.openjdk.java.net/jdk/pull/8646/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8646&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286562 Stats: 63 lines in 13 files changed: 51 ins; 1 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8646.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646 PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v2]
On Tue, 10 May 2022 09:07:44 GMT, Adam Sotona wrote: >> Please review this patch adding new lint option, **lossy-conversions**, to >> javac to warn about type casts in compound assignments with possible lossy >> conversions. >> >> The new lint warning is shown if the type of the right-hand operand of a >> compound assignment is not assignment compatible with the type of the >> variable. >> >> The implementation of the warning is based on similar check performed to >> emit "possible lossy conversion" compilation error for simple assignments. >> >> Proposed patch also include complex matrix-style test with positive and >> negative test cases of lossy conversions in compound assignments. >> >> Proposed patch also disables this new lint option in all affected JDK >> modules and libraries to allow smooth JDK build. Individual cases to address >> possibly lossy conversions warnings in JDK are already addressed in a >> separate umbrella issue and its sub-tasks. >> >> Thanks for your review, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > 8244681: Add a warning for possibly lossy conversion in compound assignments > recommended correction of the warning wording > fixed typo in test method name src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties line 210: > 208: > 209: javac.opt.Xlint.desc.lossy-conversions=\ > 210: Warn about compiler possible lossy conversions. I like this warning. But the documentation doesn't even parse as English. I suggest this, as more grammatical and precise: > Warn about possible lossy conversions in compound assignments - PR: https://git.openjdk.java.net/jdk/pull/8599
Re: RFR: 8282664: Unroll by hand StringUTF16, StringLatin1, and Arrays polynomial hash loops [v12]
On Tue, 10 May 2022 14:46:56 GMT, Ludovic Henry wrote: >> Despite the hash value being cached for Strings, computing the hash still >> represents a significant CPU usage for applications handling lots of text. >> >> Even though it would be generally better to do it through an enhancement to >> the autovectorizer, the complexity of doing it by hand is trivial and the >> gain is sizable (2x speedup) even without the Vector API. The algorithm has >> been proposed by Richard Startin and Paul Sandoz [1]. >> >> Speedup are as follows on a `Intel(R) Xeon(R) E-2276G CPU @ 3.80GHz` >> >> >> Benchmark(size) Mode Cnt Score >>Error Units >> StringHashCode.Algorithm.scalarLatin1 0 avgt 25 2.111 >> ± 0.210 ns/op >> StringHashCode.Algorithm.scalarLatin1 1 avgt 25 3.500 >> ± 0.127 ns/op >> StringHashCode.Algorithm.scalarLatin110 avgt 25 7.001 >> ± 0.099 ns/op >> StringHashCode.Algorithm.scalarLatin1 100 avgt 2561.285 >> ± 0.444 ns/op >> StringHashCode.Algorithm.scalarLatin1 1000 avgt 25 628.995 >> ± 0.846 ns/op >> StringHashCode.Algorithm.scalarLatin1 1 avgt 25 6307.990 >> ± 4.071 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 0 avgt 25 2.358 >> ± 0.092 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3.631 >> ± 0.159 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 10 avgt 25 7.049 >> ± 0.019 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 100 avgt 2533.626 >> ± 1.218 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled161000 avgt 25 317.811 >> ± 1.225 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3212.333 >> ± 14.621 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled80 avgt 25 2.356 >> ± 0.097 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3.630 >> ± 0.158 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 10 avgt 25 8.724 >> ± 0.065 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 100 avgt 2532.402 >> ± 0.019 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 1000 avgt 25 321.949 >> ± 0.251 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3202.083 >> ± 1.667 ns/op >> StringHashCode.Algorithm.scalarUTF16 0 avgt 25 2.135 >> ± 0.191 ns/op >> StringHashCode.Algorithm.scalarUTF16 1 avgt 25 5.202 >> ± 0.362 ns/op >> StringHashCode.Algorithm.scalarUTF16 10 avgt 2511.105 >> ± 0.112 ns/op >> StringHashCode.Algorithm.scalarUTF16100 avgt 2575.974 >> ± 0.702 ns/op >> StringHashCode.Algorithm.scalarUTF16 1000 avgt 25 716.429 >> ± 3.290 ns/op >> StringHashCode.Algorithm.scalarUTF16 1 avgt 25 7095.459 >> ± 43.847 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled160 avgt 25 2.381 >> ± 0.038 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 5.268 >> ± 0.422 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 10 avgt 2511.248 >> ± 0.178 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 100 avgt 2552.966 >> ± 0.089 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 1000 avgt 25 450.912 >> ± 1.834 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 4403.988 >> ± 2.927 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 0 avgt 25 2.401 >> ± 0.032 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 5.091 >> ± 0.396 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled810 avgt 2512.801 >> ± 0.189 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 100 avgt 2552.068 >> ± 0.032 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1000 avgt 25 453.270 >> ± 0.340 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 4433.112 >> ± 2.699 ns/op >> >> >> At Datadog, we handle a great amount of text (through logs management for >> example), and hashing String represents a large part of our CPU usage. It's >> very unlikely that we are the only one as String.hashCode is such a core >> feature of the JVM-based languages with its use in HashMap for example. >> Having even only a 2x speedup would allow us to save thousands of CPU cores >> per month and improve correspondingly the energy/carbon impact. >> >> [1] >> https://static.rainfocus.com/oracle/oow18/sess/1525822677955001tLqU/PF/codeone18-vector-API-DEV5081_1540354883936001Q3Sv.pdf > > Ludovic Henry has updated the pull request with a new target base due to a > merge or a reba
