Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Tue, 19 Jan 2021 20:24:21 GMT, Sean Mullan wrote: >> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS >> Extension. >> Test from the bug report and jtreg javax/naming tests are passed. > > Can you add a test for this or is it too hard? There are existing tests for > StartTLS in the security/infra area -- could they be enhanced to test this > case? Unfortunately, I can not find any LDAP StartTLS Extended Operation regression tests. security/infra area contains RevocationChecker tests. They can not be used for this scenario. - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL
> > > By letting canonizeString do the substring calls, but only when it > determines that it is needed, we can remove some unnecessary String and > byte[] allocations. > While English is not my mother tongue, I'm thinking we could maybe let the Vatican deal with canonizations and rename this to the more appropriate "canonicalizeString"? Eirik.
Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL
Hello,
sun.net.www.protocol.jar.Handler.parseURL unconditionally calls
String.substring twice on the spec string, even when
ParseUtil.canonizeString later determines that no canonization was required.
By letting canonizeString do the substring calls, but only when it
determines that it is needed, we can remove some unnecessary String and
byte[] allocations.
Patch:
https://github.com/eirbjo/jdk/commit/87da9032d7fb622f5d466f43faded4de672ebdda
JMH micro:
@Benchmark
public void initURL(Blackhole blackhole) throws MalformedURLException {
blackhole.consume(new URL(new URL("jar:file:/spring-aop.jar!/"),
"org/springframework/aop/framework/AbstractSingletonProxyFactoryBean.class"));
}
It shows a performance win for the patch in terms of throughput and bytes /
operation:
Baseline:
Benchmark Mode Cnt
Score Error Units
ZipBenchmark.initURL thrpt 25
1673457.129 ± 22857.945 ops/s
ZipBenchmark.initURL:·gc.alloc.ratethrpt 25
1410.227 ±19.283 MB/sec
ZipBenchmark.initURL:·gc.alloc.rate.norm thrpt 25
928.080 ± 0.005B/op
ZipBenchmark.initURL:·gc.churn.G1_Eden_Space thrpt 25
1412.557 ±22.575 MB/sec
ZipBenchmark.initURL:·gc.churn.G1_Eden_Space.norm thrpt 25
929.589 ± 5.756B/op
ZipBenchmark.initURL:·gc.churn.G1_Survivor_Space thrpt 25
0.006 ± 0.002 MB/sec
ZipBenchmark.initURL:·gc.churn.G1_Survivor_Space.norm thrpt 25
0.004 ± 0.001B/op
ZipBenchmark.initURL:·gc.count thrpt 25
1441.000 counts
ZipBenchmark.initURL:·gc.time thrpt 25
961.000 ms
After:
Benchmark Mode Cnt
Score Error Units
ZipBenchmark.initURL thrpt 25
1831971.983 ± 35705.091 ops/s
ZipBenchmark.initURL:·gc.alloc.ratethrpt 25
1011.389 ±19.689 MB/sec
ZipBenchmark.initURL:·gc.alloc.rate.norm thrpt 25
608.061 ± 0.001B/op
ZipBenchmark.initURL:·gc.churn.G1_Eden_Space thrpt 25
1011.746 ±20.583 MB/sec
ZipBenchmark.initURL:·gc.churn.G1_Eden_Space.norm thrpt 25
608.275 ± 3.609B/op
ZipBenchmark.initURL:·gc.churn.G1_Survivor_Space thrpt 25
0.007 ± 0.001 MB/sec
ZipBenchmark.initURL:·gc.churn.G1_Survivor_Space.norm thrpt 25
0.004 ± 0.001B/op
ZipBenchmark.initURL:·gc.count thrpt 25
1197.000 counts
ZipBenchmark.initURL:·gc.time thrpt 25
760.000 ms
Thanks,
Eirik.
Re: RFR: 8258917: NativeMemoryTracking is handled by launcher inconsistenly [v3]
> The fix adds NMT handling for non-java launchers Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: Updated comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/2106/files - new: https://git.openjdk.java.net/jdk/pull/2106/files/69c63609..dd6f9617 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2106=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2106=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2106.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2106/head:pull/2106 PR: https://git.openjdk.java.net/jdk/pull/2106
Re: RFR: 8258917: NativeMemoryTracking is handled by launcher inconsistenly [v2]
On Tue, 19 Jan 2021 23:16:30 GMT, David Holmes wrote: > What do you mean by this? The -J args are not "translated" here but later in > TranslateApplicationArgs. I meant that they are translated in TranslateApplicationArgs. Looks like it's not clear. Updated the comment as you suggested. - PR: https://git.openjdk.java.net/jdk/pull/2106
Re: RFR: 8258917: NativeMemoryTracking is handled by launcher inconsistenly [v2]
On Tue, 19 Jan 2021 23:02:54 GMT, Alex Menkov wrote: >> The fix adds NMT handling for non-java launchers > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > Non-lava launchers should process all "-J" arguments Hi Alex, I think this is functionally correct now - though the comment you added is confusing to me (see below). However I remain concerned that this requires processing the arg list twice for non-Java launchers. Is that a startup issue we should be concerned about? Thanks, David src/java.base/share/native/libjli/java.c line 821: > 819: * the -jar argument). > 820: * Non-java launchers: > 821: * All "-J" arguments are translated to VM args (see > TranslateApplicationArgs). What do you mean by this? The -J args are not "translated" here but later in TranslateApplicationArgs. For non-Java launchers AFAICS you have to process the entire argv array because you don't know where they may appear in general. So to me the comment should be: * Other launchers (IsJavaArgs()) * All arguments have to be scanned to see if it is a -J argument - PR: https://git.openjdk.java.net/jdk/pull/2106
Re: RFR: 8258917: NativeMemoryTracking is handled by launcher inconsistenly [v2]
On Sun, 17 Jan 2021 12:55:35 GMT, David Holmes wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Non-lava launchers should process all "-J" arguments
>
> Alex,
>
> This approach results in two scans of the argument list in the IsJavaArgs
> case. I don't know if we care about startup in the non-Java launchers, but
> this will likely affect it.
>
> David
@dholmes-ora
> This approach results in two scans of the argument list in the IsJavaArgs
> case. I don't know if we care about startup in the non-Java launchers, but
> this will likely affect it.
The impact is minimal (cycle through args, check if it starts from the string).
As far as I see to avoid extra scans JLI_Launch code needs to be reordered:
CreateExecutionEnvironment();
if (IsJavaArgs()) {
TranslateApplicationArgs(jargc, jargv, , );
}
ParseArguments(, , , , , jrepath);
LoadJavaVM();
And handle NMT arg in ParseArguments
But this change would be much more risky.
-
PR: https://git.openjdk.java.net/jdk/pull/2106
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v5]
> Simple fix - one line change: > https://openjdk.github.io/cr/?repo=jdk=2000=00#sdiff-0. > > Most of the changes come from an additional test that fails without this fix: > > Error: Unable to load main class somelib.test.TestMain in module somelib > java.lang.IllegalAccessError: superclass access check failed: class > somelib.test.TestMain (in module somelib) cannot access class > java.lang.Object (in module java.base) because module java.base does not > export java.lang to module somelib > > As described in JDK-8259395. > You can view the output of the test without patch here: > https://github.com/DasBrain/jdk/runs/1668078245 > > Thanks to @AlanBateman for pointing out the right fix. Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision: Fix comment, and missing newline in module-info.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/2000/files - new: https://git.openjdk.java.net/jdk/pull/2000/files/fc3b2ac0..15a057d9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2000=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2000=03-04 Stats: 7 lines in 5 files changed: 0 ins; 2 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/2000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2000/head:pull/2000 PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: 8258917: NativeMemoryTracking is handled by launcher inconsistenly [v2]
On Sun, 17 Jan 2021 12:55:35 GMT, David Holmes wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Non-lava launchers should process all "-J" arguments > > Alex, > > This approach results in two scans of the argument list in the IsJavaArgs > case. I don't know if we care about startup in the non-Java launchers, but > this will likely affect it. > > David @dholmes-ora > Also, I'm not sure the scanning logic in SetJvmEnvironment is valid for > the IsJavaArgs case. It states: > > /* > * Since this must be a VM flag we stop processing once we see > * an argument the launcher would not have processed beyond (such > * as -version or -h), or an argument that indicates the following > * arguments are for the application (i.e. the main class name, or > * the -jar argument). > */ > > but the argument rules for other commands are different. yes, you are right. TranslateApplicationArgs translates all "-J" args. I updated the fix. For non-java launchers we don't need to scan java args as we know they don't contain -J-XX:NativeMemoryTracking - PR: https://git.openjdk.java.net/jdk/pull/2106
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v4]
> Simple fix - one line change: > https://openjdk.github.io/cr/?repo=jdk=2000=00#sdiff-0. > > Most of the changes come from an additional test that fails without this fix: > > Error: Unable to load main class somelib.test.TestMain in module somelib > java.lang.IllegalAccessError: superclass access check failed: class > somelib.test.TestMain (in module somelib) cannot access class > java.lang.Object (in module java.base) because module java.base does not > export java.lang to module somelib > > As described in JDK-8259395. > You can view the output of the test without patch here: > https://github.com/DasBrain/jdk/runs/1668078245 > > Thanks to @AlanBateman for pointing out the right fix. Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision: Implement more tests. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2000/files - new: https://git.openjdk.java.net/jdk/pull/2000/files/7c2ed088..fc3b2ac0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2000=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2000=02-03 Stats: 388 lines in 10 files changed: 316 ins; 54 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/2000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2000/head:pull/2000 PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v3]
On Tue, 19 Jan 2021 16:14:51 GMT, Alan Bateman wrote: >>> >>> >>> This issue requires a Reviewer from someone working in this area. Please do >>> not sponsor or integrate until that review has been done. >> >> Ok, increased the number of required reviewers to 2. >> Hope that was the right move, as I don't see any other way to undo >> /integrate. > > Finally getting back to this. The update to ModulePatcher.java is good. Test > coverage needs discussion. There are four scenarios where test coverage is > lacking: > > 1. automatic module on the module path, patched to override or augment > classes/resources > 2. automatic module on the module path, patched to add new packages > 3. automatic module as the initial module, patched to override or augment > classes/resources > 4. automatic module as the initial module, patched to add new packages > > The patch adds automatic/PatchTest.java so it's adding test coverage for 4. > We should probably rename it to leave room for the other tests, or else > create it so that additional test coverage can be added. I assume the test > was copied from another test as there are a few left overs, e.g. `@modules > java.script` but the test does not use this module. I don't want to expand > the scope of this PR too much but I think we should at least cover 3 and 4 in > the test. Thanks @AlanBateman, I now implemented a few more tests. They should cover all four cases you described. - PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: 8258917: NativeMemoryTracking is handled by launcher inconsistenly [v2]
> The fix adds NMT handling for non-java launchers Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: Non-lava launchers should process all "-J" arguments - Changes: - all: https://git.openjdk.java.net/jdk/pull/2106/files - new: https://git.openjdk.java.net/jdk/pull/2106/files/53e876e9..69c63609 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2106=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2106=00-01 Stats: 16 lines in 2 files changed: 8 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2106.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2106/head:pull/2106 PR: https://git.openjdk.java.net/jdk/pull/2106
Integrated: Merge jdk16
On Tue, 19 Jan 2021 21:48:55 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 16 -> JDK 17 This pull request has now been integrated. Changeset: cf25383d Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/cf25383d Stats: 88 lines in 2 files changed: 29 ins; 26 del; 33 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/2151
Re: RFR: 8259498: Reduce overhead of MD5 and SHA digests [v4]
On Mon, 18 Jan 2021 13:39:04 GMT, Claes Redestad wrote: >> - The MD5 intrinsics added by >> [JDK-8250902](https://bugs.openjdk.java.net/browse/JDK-8250902) shows that >> the `int[] x` isn't actually needed. This also applies to the SHA intrinsics >> from which the MD5 intrinsic takes inspiration >> - Using VarHandles we can simplify the code in `ByteArrayAccess` enough to >> make it acceptable to use inline and replace the array in MD5 wholesale. >> This improves performance both in the presence and the absence of the >> intrinsic optimization. >> - Doing the exact same thing in the SHA impls would be unwieldy (64+ element >> arrays), but allocating the array lazily gets most of the speed-up in the >> presence of an intrinsic while being neutral in its absence. >> >> Baseline: >> (digesterName) (length)Cnt Score >> Error Units >> MessageDigests.digestMD516 15 >> 2714.307 ± 21.133 ops/ms >> MessageDigests.digestMD5 1024 15 >> 318.087 ±0.637 ops/ms >> MessageDigests.digest SHA-116 15 >> 1387.266 ± 40.932 ops/ms >> MessageDigests.digest SHA-1 1024 15 >> 109.273 ±0.149 ops/ms >> MessageDigests.digestSHA-25616 15 >> 995.566 ± 21.186 ops/ms >> MessageDigests.digestSHA-256 1024 15 >> 89.104 ±0.079 ops/ms >> MessageDigests.digestSHA-51216 15 >> 803.030 ± 15.722 ops/ms >> MessageDigests.digestSHA-512 1024 15 >> 115.611 ±0.234 ops/ms >> MessageDigests.getAndDigest MD516 15 >> 2190.367 ± 97.037 ops/ms >> MessageDigests.getAndDigest MD5 1024 15 >> 302.903 ±1.809 ops/ms >> MessageDigests.getAndDigestSHA-116 15 >> 1262.656 ± 43.751 ops/ms >> MessageDigests.getAndDigestSHA-1 1024 15 >> 104.889 ±3.554 ops/ms >> MessageDigests.getAndDigest SHA-25616 15 >> 914.541 ± 55.621 ops/ms >> MessageDigests.getAndDigest SHA-256 1024 15 >> 85.708 ±1.394 ops/ms >> MessageDigests.getAndDigest SHA-51216 15 >> 737.719 ± 53.671 ops/ms >> MessageDigests.getAndDigest SHA-512 1024 15 >> 112.307 ±1.950 ops/ms >> >> GC: >> MessageDigests.getAndDigest:·gc.alloc.rate.norm MD516 15 >> 312.011 ±0.005B/op >> MessageDigests.getAndDigest:·gc.alloc.rate.normSHA-116 15 >> 584.020 ±0.006B/op >> MessageDigests.getAndDigest:·gc.alloc.rate.norm SHA-25616 15 >> 544.019 ±0.016B/op >> MessageDigests.getAndDigest:·gc.alloc.rate.norm SHA-51216 15 >> 1056.037 ±0.003B/op >> >> Target: >> Benchmark (digesterName) (length)Cnt >> Score Error Units >> MessageDigests.digestMD516 15 >> 3134.462 ± 43.685 ops/ms >> MessageDigests.digestMD5 1024 15 >> 323.667 ±0.633 ops/ms >> MessageDigests.digest SHA-116 15 >> 1418.742 ± 38.223 ops/ms >> MessageDigests.digest SHA-1 1024 15 >> 110.178 ±0.788 ops/ms >> MessageDigests.digestSHA-25616 15 >> 1037.949 ± 21.214 ops/ms >> MessageDigests.digestSHA-256 1024 15 >> 89.671 ±0.228 ops/ms >> MessageDigests.digestSHA-51216 15 >> 812.028 ± 39.489 ops/ms >> MessageDigests.digestSHA-512 1024 15 >> 116.738 ±0.249 ops/ms >> MessageDigests.getAndDigest MD516 15 >> 2314.379 ± 229.294 ops/ms >> MessageDigests.getAndDigest MD5 1024 15 >> 307.835 ±5.730 ops/ms >> MessageDigests.getAndDigestSHA-116 15 >> 1326.887 ± 63.263 ops/ms >> MessageDigests.getAndDigestSHA-1 1024 15 >> 106.611 ±2.292 ops/ms >> MessageDigests.getAndDigest SHA-25616 15 >> 961.589 ± 82.052 ops/ms >> MessageDigests.getAndDigest SHA-256 1024 15 >> 88.646 ±0.194 ops/ms >> MessageDigests.getAndDigest SHA-51216 15 >> 775.417 ± 56.775 ops/ms >> MessageDigests.getAndDigest SHA-512 1024 15 >> 112.904 ±2.014 ops/ms >>
Re: IllegalStateException in CharsetDecoder when inspecting JarFile contents on Java 15
Hi Claes, You’re fast! :) Thanks for the bug report and working on a fix. Best, Vitaly On Tue, Jan 19, 2021 at 5:00 PM Claes Redestad wrote: > Filed: https://bugs.openjdk.java.net/browse/JDK-8260010 > > I have a potential fix ready, just trying to distill a minimal test case. > > /Claes > > On 2021-01-19 21:46, Claes Redestad wrote: > > Hi, > > > > yes, this seems like a bug introduced by JDK-8243469 where the > > previously thread-safe UTF8ZipCoder is now using a thread-unsafe > > decoder to calculate the normalized hash. I'll file a bug and take a > > look. > > > > /Claes > > > > On 2021-01-19 20:19, Vitaly Davidovich wrote: > >> Hi all, > >> > >> I observed the following stacktrace when inspecting a JarFile's contents > >> using a parallel stream (i.e. FJ pool): > >> > >> Exception in thread "main" java.lang.IllegalStateException: > >> java.lang.IllegalStateException: Current state = CODING_END, new state = > >> FLUSHED > >> > >>at > >> > java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native > > >> > >> Method) > >> > >>at > >> > java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:64) > > >> > >> > >>at > >> > java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) > > >> > >> > >>at > >> > java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500) > > >> > >> > >>at > >> > java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481) > >> > >> > >>at > >> > java.base/java.util.concurrent.ForkJoinTask.getThrowableException(ForkJoinTask.java:600) > > >> > >> > >>at > >> > java.base/java.util.concurrent.ForkJoinTask.reportException(ForkJoinTask.java:678) > > >> > >> > >>at > >> > java.base/java.util.concurrent.ForkJoinTask.invoke(ForkJoinTask.java:737) > >> > >> > >>at > >> > java.base/java.util.stream.ForEachOps$ForEachOp.evaluateParallel(ForEachOps.java:159) > > >> > >> > >>at > >> > java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateParallel(ForEachOps.java:173) > > >> > >> > >>at > >> > java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:233) > > >> > >> > >>at > >> > java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) > > >> > >> > >>at > >> > java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:575) > > >> > >> > >>at > >> .ClassVersions.conflicts(ClassVersions.java:703) > >> > >>at .ClassVersions.check(ClassVersions.java:743) > >> > >>at .ClassVersions.main(ClassVersions.java:839) > >> > >> Caused by: java.lang.IllegalStateException: Current state = > >> CODING_END, new > >> state = FLUSHED > >> > >>at > >> > java.base/java.nio.charset.CharsetDecoder.throwIllegalStateException(CharsetDecoder.java:998) > > >> > >> > >>at > >> java.base/java.nio.charset.CharsetDecoder.flush(CharsetDecoder.java:681) > >> > >>at > >> > java.base/java.nio.charset.CharsetDecoder.decode(CharsetDecoder.java:810) > >> > >> > >>at > >> java.base/java.util.zip.ZipCoder.normalizedHashDecode(ZipCoder.java:136) > >> > >>at > >> > java.base/java.util.zip.ZipCoder$UTF8ZipCoder.normalizedHash(ZipCoder.java:228) > > >> > >> > >>at > >> java.base/java.util.zip.ZipFile$Source.initCEN(ZipFile.java:1527) > >> > >>at > >> java.base/java.util.zip.ZipFile$Source.(ZipFile.java:1249) > >> > >>at > >> java.base/java.util.zip.ZipFile$Source.get(ZipFile.java:1211) > >> > >>at > >> > java.base/java.util.zip.ZipFile$CleanableResource.(ZipFile.java:701) > >> > >> > >>at > >> java.base/java.util.zip.ZipFile.(ZipFile.java:240) > >> > >>at > >> java.base/java.util.zip.ZipFile.(ZipFile.java:171) > >> > >>at > >> java.base/java.util.jar.JarFile.(JarFile.java:347) > >> > >>at > >> java.base/java.util.jar.JarFile.(JarFile.java:318) > >> > >>at > >> java.base/java.util.jar.JarFile.(JarFile.java:284) > >> > >>at > >> .ClassVersions.lookupJar(ClassVersions.java:665) > >> > >>at > >> > java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195) > > >> > >> > >>at > >> > java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177) > > >> > >> > >>at > >> > java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195) > > >> > >> > >>at > >> > java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177) > > >> > >> > >>at > >> >
RFR: Merge jdk16
Forwardport JDK 16 -> JDK 17 - Commit messages: - Merge - 8259796: timed CompletableFuture.get may swallow InterruptedException The webrevs contain the adjustments done while merging with regards to each parent branch: - master: https://webrevs.openjdk.java.net/?repo=jdk=2151=00.0 - jdk16: https://webrevs.openjdk.java.net/?repo=jdk=2151=00.1 Changes: https://git.openjdk.java.net/jdk/pull/2151/files Stats: 88 lines in 2 files changed: 29 ins; 26 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/2151.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2151/head:pull/2151 PR: https://git.openjdk.java.net/jdk/pull/2151
Re: RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
On Mon, 18 Jan 2021 13:32:24 GMT, Jie Fu wrote:
> Hi all,
>
> For this reproducer:
>
> import jdk.incubator.vector.ByteVector;
> import jdk.incubator.vector.VectorSpecies;
>
> public class Test {
> static final VectorSpecies SPECIES_128 = ByteVector.SPECIES_128;
> static byte[] a = new byte[8];
> static byte[] b = new byte[8];
>
> public static void main(String[] args) {
> ByteVector av = ByteVector.fromArray(SPECIES_128, a, 0);
> av.intoArray(b, 0);
> System.out.println("b: " + b[0]);
> }
> }
>
> The following IndexOutOfBoundsException message (length -7) is unreasonable.
>
> Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out
> of bounds for length -7
>
> It might be better to fix it like this.
>
> Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out
> of bounds for length 0
>
> Thanks.
> Best regards,
> Jie
That change may cause performance issues. I would recommend leaving as is for
now even through the error message is not great. Bounds checking is quite
sensitive and WIP. Notice that we also have an option to call
`Objects.checkFromIndexSize` which expresses the intent more accurately, but
that is currently less optimal (at least it was when i last checked since it is
non an intrinsic).
-
PR: https://git.openjdk.java.net/jdk/pull/2127
Re: RFR: 8259911: byteArrayViewVarHandle should throw ArrayIndexOutOfBoundsException
On Tue, 19 Jan 2021 20:59:24 GMT, Claes Redestad wrote: > I agree the change is allowable within the current spec. The behavior change > is observable, though, and however unlikely there might be code that relies > on exact class of the exception being thrown. > So while perhaps not strictly needed, the spec change makes sense coming from > the other way: If I have some array-based code then that will be throwing > AIOOBE on OOBs. But the `byteArrayViewVarHandle` code is specified to throw > IOOBE, so a developer picking it up would have to catch and rewrap exceptions > or accept the compatibility risk. Harmonizing to specify AIOOBE improves the > migration story. I don't think there is any migration story. The exception type is an improved clarity. The spec change is very minor and I'm okay with or without the spec change. > Case in point is #1855 which require changes to some tests that are expecting > AIOOBE. Yes, that's what I was about to ask too. - PR: https://git.openjdk.java.net/jdk/pull/2124
Re: RFR: 8259911: byteArrayViewVarHandle should throw ArrayIndexOutOfBoundsException
On Tue, 19 Jan 2021 20:30:46 GMT, Claes Redestad wrote: >> The change to AIOOBE is reasonable.Have you considered making this just >> as an implementation change and leave the spec as is? > >> The change to AIOOBE is reasonable. Have you considered making this just as >> an implementation change and leave the spec as is? > > What would be the benefits? AFAICT the CSR is still needed since it's a small > behavioral change, and updating the spec helps communicate that change. I agree the change is allowable within the current spec. The behavior change is observable, though, and however unlikely there might be code that relies on exact class of the exception being thrown. So while perhaps not strictly needed, the spec change makes sense coming from the other way: If I have some array-based code then that will be throwing AIOOBE on OOBs. But the `byteArrayViewVarHandle` code is specified to throw IOOBE, so a developer picking it up would have to catch and rewrap exceptions or accept the compatibility risk. Harmonizing to specify AIOOBE improves the migration story. Case in point is #1855 which require changes to some tests that are expecting AIOOBE. - PR: https://git.openjdk.java.net/jdk/pull/2124
Re: RFR: 8259911: byteArrayViewVarHandle should throw ArrayIndexOutOfBoundsException
On 1/19/21 12:33 PM, Claes Redestad wrote: On Tue, 19 Jan 2021 19:42:38 GMT, Mandy Chung wrote: The change to AIOOBE is reasonable. Have you considered making this just as an implementation change and leave the spec as is? What would be the benefits? AFAICT the CSR is still needed since it's a small behavioral change, and updating the spec helps communicate that change. There is no behavioral change since AIIOBE is a subtype of OOBE. No spec change and so no CSR is needed. Mandy
Re: RFR: 8259911: byteArrayViewVarHandle should throw ArrayIndexOutOfBoundsException
On Tue, 19 Jan 2021 19:42:38 GMT, Mandy Chung wrote: > The change to AIOOBE is reasonable. Have you considered making this just as > an implementation change and leave the spec as is? What would be the benefits? AFAICT the CSR is still needed since it's a small behavioral change, and updating the spec helps communicate that change. - PR: https://git.openjdk.java.net/jdk/pull/2124
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Thu, 14 Jan 2021 19:28:27 GMT, Alexey Bakhtin wrote: > Please review a small patch to enable LDAP TLS Channel Binding with StartTLS > Extension. > Test from the bug report and jtreg javax/naming tests are passed. Can you add a test for this or is it too hard? There are existing tests for StartTLS in the security/infra area -- could they be enhanced to test this case? - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259911: byteArrayViewVarHandle should throw ArrayIndexOutOfBoundsException
On Mon, 18 Jan 2021 12:09:23 GMT, Claes Redestad wrote: > Change `MethodHandles.byteArrayViewVarHandle` to throw > `ArrayIndexOutOfBoundsException` rather than the more generic > `IndexArrayOutOfBoundsException`. This feels more natural, and reduces the > risk for subtle behavioral mismatch when migrating code from arrays/Unsafe to > VHs. > > CSR: [JDK-8259912](https://bugs.openjdk.java.net/browse/JDK-8259912) The change to AIOOBE is reasonable.Have you considered making this just as an implementation change and leave the spec as is? - PR: https://git.openjdk.java.net/jdk/pull/2124
Integrated: 8132984: incorrect type for Reference.discovered
On Sat, 26 Dec 2020 03:25:51 GMT, Kim Barrett wrote: > Please review this change which fixes the type of the private > Reference.discovered field. It was Reference, but that's wrong because > it can be any Reference object. > > I've changed it to Reference and let that flow through, updating some > other variables that were previously somewhat incorrectly typed (usually > with an Object type parameter). The interesting change is to the > ReferenceQueue.enqueue parameter, which is now also Reference. > > This ultimately end up with a provably safe and correct, but uncheckable, > cast in ReferenceQueue.enqueue. > > An alternative might be to use a raw type for the discovered field, but I > think that ends up with more @SuppressWarnings of various flavors. I think > the unbounded wildcard approach is clearer and cleaner. > > Note that all of the pending list handling, including the discovered field, > could be moved into a non-public, non-generic, sealed(?) base class of > Reference. The pending list handling has nothing to do with the generic > parameter T. > > Testing: > mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) This pull request has now been integrated. Changeset: 33dcc00c Author:Kim Barrett URL: https://git.openjdk.java.net/jdk/commit/33dcc00c Stats: 17 lines in 1 file changed: 10 ins; 1 del; 6 mod 8132984: incorrect type for Reference.discovered Use unbounded wildcard placeholders, plus a new helper to get back to the Reference domain. Reviewed-by: rkennke, plevart, rriggs, mchung - PR: https://git.openjdk.java.net/jdk/pull/1897
Re: RFR: 8132984: incorrect type for Reference.discovered [v4]
> Please review this change which fixes the type of the private > Reference.discovered field. It was Reference, but that's wrong because > it can be any Reference object. > > I've changed it to Reference and let that flow through, updating some > other variables that were previously somewhat incorrectly typed (usually > with an Object type parameter). The interesting change is to the > ReferenceQueue.enqueue parameter, which is now also Reference. > > This ultimately end up with a provably safe and correct, but uncheckable, > cast in ReferenceQueue.enqueue. > > An alternative might be to use a raw type for the discovered field, but I > think that ends up with more @SuppressWarnings of various flavors. I think > the unbounded wildcard approach is clearer and cleaner. > > Note that all of the pending list handling, including the discovered field, > could be moved into a non-public, non-generic, sealed(?) base class of > Reference. The pending list handling has nothing to do with the generic > parameter T. > > Testing: > mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' into fix_discovered_type - plevart improvement - update copyrights - Merge branch 'master' into fix_discovered_type - Use unbounded wildcard placeholders and final safe but unchecked cast - Changes: - all: https://git.openjdk.java.net/jdk/pull/1897/files - new: https://git.openjdk.java.net/jdk/pull/1897/files/b95f5140..cd66bff1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1897=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1897=02-03 Stats: 3433 lines in 92 files changed: 381 ins; 2791 del; 261 mod Patch: https://git.openjdk.java.net/jdk/pull/1897.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1897/head:pull/1897 PR: https://git.openjdk.java.net/jdk/pull/1897
Re: RFR: 8132984: incorrect type for Reference.discovered [v3]
On Tue, 19 Jan 2021 11:15:17 GMT, Peter Levart wrote: >> Kim Barrett has updated the pull request incrementally with one additional >> commit since the last revision: >> >> plevart improvement > > This looks good. Thanks @plevart , @rkennke , @RogerRiggs , and @mlchung for reviews. - PR: https://git.openjdk.java.net/jdk/pull/1897
[jdk16] Integrated: 8259796: timed CompletableFuture.get may swallow InterruptedException
On Sun, 17 Jan 2021 18:39:55 GMT, Martin Buchholz wrote: > 8259796: timed CompletableFuture.get may swallow InterruptedException This pull request has now been integrated. Changeset: f7b96d34 Author:Martin Buchholz URL: https://git.openjdk.java.net/jdk16/commit/f7b96d34 Stats: 88 lines in 2 files changed: 29 ins; 26 del; 33 mod 8259796: timed CompletableFuture.get may swallow InterruptedException Reviewed-by: dl, alanb - PR: https://git.openjdk.java.net/jdk16/pull/126
Re: RFR: 8132984: incorrect type for Reference.discovered [v3]
On Mon, 18 Jan 2021 23:42:08 GMT, Kim Barrett wrote: >> Please review this change which fixes the type of the private >> Reference.discovered field. It was Reference, but that's wrong because >> it can be any Reference object. >> >> I've changed it to Reference and let that flow through, updating some >> other variables that were previously somewhat incorrectly typed (usually >> with an Object type parameter). The interesting change is to the >> ReferenceQueue.enqueue parameter, which is now also Reference. >> >> This ultimately end up with a provably safe and correct, but uncheckable, >> cast in ReferenceQueue.enqueue. >> >> An alternative might be to use a raw type for the discovered field, but I >> think that ends up with more @SuppressWarnings of various flavors. I think >> the unbounded wildcard approach is clearer and cleaner. >> >> Note that all of the pending list handling, including the discovered field, >> could be moved into a non-public, non-generic, sealed(?) base class of >> Reference. The pending list handling has nothing to do with the generic >> parameter T. >> >> Testing: >> mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > plevart improvement Looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1897
[11u] RFR: 7146776: deadlock between URLStreamHandler.getHostAddress and file.Handler.openconnection
Hi,
JDK-7146776 is backported to 11.0.11-oracle. I'd like to backport it for parity.
Change doesn't apply cleanly because of Copyright year changes and a minor
difference in a hunk which gets removed by this change:
11u Code contains host.equals(""), patch wants to remove host.isEmpty() from
URLStreamHandler.java.
Bug:
https://bugs.openjdk.java.net/browse/JDK-7146776
Original change:
https://git.openjdk.java.net/jdk/commit/db9c114d
11u backport:
http://cr.openjdk.java.net/~mdoerr/7146776_windows_deadlock_11u/webrev.00/
Please review.
Best regards,
Martin
Re: [jdk16] RFR: JDK-8259732: JDK 16 L10n resource file update - msg drop 10 [v3]
On Mon, 18 Jan 2021 05:52:57 GMT, Leo Jiang wrote: >> This is the changes for JDK 16 msg drop 10. > > Leo Jiang has updated the pull request incrementally with one additional > commit since the last revision: > > fix the missing copyright year for standard.properties Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk16/pull/123
Re: RFR: 8259842: Remove Result cache from StringCoding [v7]
On Mon, 18 Jan 2021 09:26:07 GMT, Claes Redestad wrote: >> The `StringCoding.resultCached` mechanism is used to remove the allocation >> of a `StringCoding.Result` object on potentially hot paths used in some >> `String` constructors. Using a `ThreadLocal` has overheads though, and the >> overhead got a bit worse after JDK-8258596 which addresses a leak by adding >> a `SoftReference`. >> >> This patch refactors much of the decode logic back into `String` and gets >> rid of not only the `Result` cache, but the `Result` class itself along with >> the `StringDecoder` class and cache. >> >> Microbenchmark results: >> Baseline >> >> Benchmark (charsetName) Mode Cnt >>ScoreError Units >> decodeCharsetUS-ASCII avgt 15 >> 193.043 ± 8.207 ns/op >> decodeCharset:·gc.alloc.rate.normUS-ASCII avgt 15 >> 112.009 ± 0.001B/op >> decodeCharset ISO-8859-1 avgt 15 >> 164.580 ± 6.514 ns/op >> decodeCharset:·gc.alloc.rate.norm ISO-8859-1 avgt 15 >> 112.009 ± 0.001B/op >> decodeCharset UTF-8 avgt 15 >> 328.370 ± 18.420 ns/op >> decodeCharset:·gc.alloc.rate.norm UTF-8 avgt 15 >> 224.019 ± 0.002B/op >> decodeCharset MS932 avgt 15 >> 328.870 ± 20.056 ns/op >> decodeCharset:·gc.alloc.rate.norm MS932 avgt 15 >> 232.020 ± 0.002B/op >> decodeCharset ISO-8859-6 avgt 15 >> 193.603 ± 12.305 ns/op >> decodeCharset:·gc.alloc.rate.norm ISO-8859-6 avgt 15 >> 112.010 ± 0.001B/op >> decodeCharsetNameUS-ASCII avgt 15 >> 209.454 ± 9.040 ns/op >> decodeCharsetName:·gc.alloc.rate.normUS-ASCII avgt 15 >> 112.009 ± 0.001B/op >> decodeCharsetName ISO-8859-1 avgt 15 >> 188.234 ± 7.533 ns/op >> decodeCharsetName:·gc.alloc.rate.norm ISO-8859-1 avgt 15 >> 112.009 ± 0.001B/op >> decodeCharsetName UTF-8 avgt 15 >> 399.463 ± 12.437 ns/op >> decodeCharsetName:·gc.alloc.rate.norm UTF-8 avgt 15 >> 224.019 ± 0.003B/op >> decodeCharsetName MS932 avgt 15 >> 358.839 ± 15.385 ns/op >> decodeCharsetName:·gc.alloc.rate.norm MS932 avgt 15 >> 208.017 ± 0.003B/op >> decodeCharsetName ISO-8859-6 avgt 15 >> 162.570 ± 7.090 ns/op >> decodeCharsetName:·gc.alloc.rate.norm ISO-8859-6 avgt 15 >> 112.009 ± 0.001B/op >> decodeDefault N/A avgt 15 >> 316.081 ± 11.101 ns/op >> decodeDefault:·gc.alloc.rate.norm N/A avgt 15 >> 224.019 ± 0.002B/op >> >> Patched: >> Benchmark (charsetName) Mode Cnt >>ScoreError Units >> decodeCharsetUS-ASCII avgt 15 >> 159.153 ± 6.082 ns/op >> decodeCharset:·gc.alloc.rate.normUS-ASCII avgt 15 >> 112.009 ± 0.001B/op >> decodeCharset ISO-8859-1 avgt 15 >> 134.433 ± 6.203 ns/op >> decodeCharset:·gc.alloc.rate.norm ISO-8859-1 avgt 15 >> 112.009 ± 0.001B/op >> decodeCharset UTF-8 avgt 15 >> 297.234 ± 21.654 ns/op >> decodeCharset:·gc.alloc.rate.norm UTF-8 avgt 15 >> 224.019 ± 0.002B/op >> decodeCharset MS932 avgt 15 >> 311.583 ± 16.445 ns/op >> decodeCharset:·gc.alloc.rate.norm MS932 avgt 15 >> 208.018 ± 0.002B/op >> decodeCharset ISO-8859-6 avgt 15 >> 156.216 ± 6.522 ns/op >> decodeCharset:·gc.alloc.rate.norm ISO-8859-6 avgt 15 >> 112.010 ± 0.001B/op >> decodeCharsetNameUS-ASCII avgt 15 >> 180.752 ± 9.411 ns/op >> decodeCharsetName:·gc.alloc.rate.normUS-ASCII avgt 15 >> 112.010 ± 0.001B/op >> decodeCharsetName ISO-8859-1 avgt 15 >> 156.170 ± 8.003 ns/op >> decodeCharsetName:·gc.alloc.rate.norm ISO-8859-1 avgt 15 >> 112.010 ± 0.001B/op >> decodeCharsetName UTF-8 avgt 15 >> 370.337 ± 22.199 ns/op >> decodeCharsetName:·gc.alloc.rate.norm
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v3]
On Mon, 11 Jan 2021 14:22:20 GMT, Johannes Kuhn wrote: >> This issue requires a Reviewer from someone working in this area. Please do >> not sponsor or integrate until that review has been done. > >> >> >> This issue requires a Reviewer from someone working in this area. Please do >> not sponsor or integrate until that review has been done. > > Ok, increased the number of required reviewers to 2. > Hope that was the right move, as I don't see any other way to undo /integrate. Finally getting back to this. The update to ModulePatcher.java is good. Test coverage needs discussion. There are four scenarios where test coverage is lacking: 1. automatic module on the module path, patched to override or augment classes/resources 2. automatic module on the module path, patched to add new packages 3. automatic module as the initial module, patched to override or augment classes/resources 4. automatic module as the initial module, patched to add new packages The patch adds automatic/PatchTest.java so it's adding test coverage for 4. We should probably rename it to leave room for the other tests, or else create it so that additional test coverage can be added. I assume the test was copied from another test as there are a few left overs, e.g. `@modules java.script` but the test does not use this module. I don't want to expand the scope of this PR too much but I think we should at least cover 3 and 4 in the test. - PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: 8132984: incorrect type for Reference.discovered [v3]
On Mon, 18 Jan 2021 23:42:08 GMT, Kim Barrett wrote: >> Please review this change which fixes the type of the private >> Reference.discovered field. It was Reference, but that's wrong because >> it can be any Reference object. >> >> I've changed it to Reference and let that flow through, updating some >> other variables that were previously somewhat incorrectly typed (usually >> with an Object type parameter). The interesting change is to the >> ReferenceQueue.enqueue parameter, which is now also Reference. >> >> This ultimately end up with a provably safe and correct, but uncheckable, >> cast in ReferenceQueue.enqueue. >> >> An alternative might be to use a raw type for the discovered field, but I >> think that ends up with more @SuppressWarnings of various flavors. I think >> the unbounded wildcard approach is clearer and cleaner. >> >> Note that all of the pending list handling, including the discovered field, >> could be moved into a non-public, non-generic, sealed(?) base class of >> Reference. The pending list handling has nothing to do with the generic >> parameter T. >> >> Testing: >> mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > plevart improvement Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1897
Re: [jdk16] RFR: 8259796: timed CompletableFuture.get may swallow InterruptedException [v2]
On Tue, 19 Jan 2021 02:50:54 GMT, Martin Buchholz wrote: >> 8259796: timed CompletableFuture.get may swallow InterruptedException > > Martin Buchholz has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains one commit: > > JDK-8259796 Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk16/pull/126
Re: RFR: 8259947: (fs) Optimize UnixPath.encode implementation [v3]
On Tue, 19 Jan 2021 12:26:08 GMT, Claes Redestad wrote: >> This patch improves `UnixPath.encode` by reusing `JLA.getBytesNoRepl` (which >> has fast-paths for common encoding) and avoiding a `toCharArray` call on the >> input by refactoring the `normalizeNativePath` code to operate on `String`. >> This might have a cost on files on Mac that need additional native >> normalization. >> >> This removes another `ThreadLocal` and a source of `SoftReference`s. >> Together with the UTF-8 fast-path my UTF-8 encoded file system see >> substantial speed-ups in a trivial `new File(str).toPath()` microbenchmark. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Move JLA to top, add imports Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2135
Re: RFR: 8259947: (fs) Optimize UnixPath.encode implementation [v3]
> This patch improves `UnixPath.encode` by reusing `JLA.getBytesNoRepl` (which > has fast-paths for common encoding) and avoiding a `toCharArray` call on the > input by refactoring the `normalizeNativePath` code to operate on `String`. > This might have a cost on files on Mac that need additional native > normalization. > > This removes another `ThreadLocal` and a source of `SoftReference`s. Together > with the UTF-8 fast-path my UTF-8 encoded file system see substantial > speed-ups in a trivial `new File(str).toPath()` microbenchmark. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Move JLA to top, add imports - Changes: - all: https://git.openjdk.java.net/jdk/pull/2135/files - new: https://git.openjdk.java.net/jdk/pull/2135/files/18c3105b..b023c0a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2135=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2135=01-02 Stats: 8 lines in 1 file changed: 5 ins; 3 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2135.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2135/head:pull/2135 PR: https://git.openjdk.java.net/jdk/pull/2135
Re: RFR: 8259867: Move encoding checks into ZipCoder
On Sat, 16 Jan 2021 17:49:38 GMT, eirbjo wrote: > ZipFile.Source.initCEN verifies that entry names are encoding into bytes > valid in the entry's encoding. It does so by calling encoding-specific > checking methods, so it also needs to determine which check method to call > for each entry. > > By moving the encoding-variant checks into ZipCoder, initCEN can instead > simply call ZipCoder.checkEncoding. This makes the code easier to follow and > also removes a duplication of flag checking logic found in zipCoderForPos. The change looks good to me as well - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2110
Re: RFR: 8259947: (fs) Optimize UnixPath.encode implementation [v2]
On Tue, 19 Jan 2021 12:02:13 GMT, Claes Redestad wrote: >> This patch improves `UnixPath.encode` by reusing `JLA.getBytesNoRepl` (which >> has fast-paths for common encoding) and avoiding a `toCharArray` call on the >> input by refactoring the `normalizeNativePath` code to operate on `String`. >> This might have a cost on files on Mac that need additional native >> normalization. >> >> This removes another `ThreadLocal` and a source of `SoftReference`s. >> Together with the UTF-8 fast-path my UTF-8 encoded file system see >> substantial speed-ups in a trivial `new File(str).toPath()` microbenchmark. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Fold ToPath into FileOpen, add root benchmarks to keep mix comparable src/java.base/unix/classes/sun/nio/fs/UnixPath.java line 112: > 110: private static final jdk.internal.access.JavaLangAccess JLA = > 111: jdk.internal.access.SharedSecrets.getJavaLangAccess(); > 112: Can you move this to the top, before the instance fields? Also let's import the jdk.internal.acces classes rather than using fully qualified names. That will keep it consistent with the existing code in this area. - PR: https://git.openjdk.java.net/jdk/pull/2135
Re: RFR: 8259947: (fs) Optimize UnixPath.encode implementation [v2]
On Tue, 19 Jan 2021 12:02:13 GMT, Claes Redestad wrote:
>> This patch improves `UnixPath.encode` by reusing `JLA.getBytesNoRepl` (which
>> has fast-paths for common encoding) and avoiding a `toCharArray` call on the
>> input by refactoring the `normalizeNativePath` code to operate on `String`.
>> This might have a cost on files on Mac that need additional native
>> normalization.
>>
>> This removes another `ThreadLocal` and a source of `SoftReference`s.
>> Together with the UTF-8 fast-path my UTF-8 encoded file system see
>> substantial speed-ups in a trivial `new File(str).toPath()` microbenchmark.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fold ToPath into FileOpen, add root benchmarks to keep mix comparable
src/java.base/unix/classes/sun/nio/fs/UnixPath.java line 118:
> 116: try {
> 117: return JLA.getBytesNoRepl(input, Util.jnuEncoding());
> 118: } catch (CharacterCodingException cce) {
The encode method pre-dates JLA.getBytesNoRepl and the recent optimisations so
this is a good cleanup.
-
PR: https://git.openjdk.java.net/jdk/pull/2135
Re: RFR: 8259947: (fs) Optimize UnixPath.encode implementation [v2]
> This patch improves `UnixPath.encode` by reusing `JLA.getBytesNoRepl` (which > has fast-paths for common encoding) and avoiding a `toCharArray` call on the > input by refactoring the `normalizeNativePath` code to operate on `String`. > This might have a cost on files on Mac that need additional native > normalization. > > This removes another `ThreadLocal` and a source of `SoftReference`s. Together > with the UTF-8 fast-path my UTF-8 encoded file system see substantial > speed-ups in a trivial `new File(str).toPath()` microbenchmark. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Fold ToPath into FileOpen, add root benchmarks to keep mix comparable - Changes: - all: https://git.openjdk.java.net/jdk/pull/2135/files - new: https://git.openjdk.java.net/jdk/pull/2135/files/27a55ee0..18c3105b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2135=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2135=00-01 Stats: 128 lines in 2 files changed: 50 ins; 72 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2135.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2135/head:pull/2135 PR: https://git.openjdk.java.net/jdk/pull/2135
Re: RFR: 8259947: (fs) Optimize UnixPath.encode implementation
On Tue, 19 Jan 2021 10:46:32 GMT, Aleksey Shipilev wrote: >> This patch improves `UnixPath.encode` by reusing `JLA.getBytesNoRepl` (which >> has fast-paths for common encoding) and avoiding a `toCharArray` call on the >> input by refactoring the `normalizeNativePath` code to operate on `String`. >> This might have a cost on files on Mac that need additional native >> normalization. >> >> This removes another `ThreadLocal` and a source of `SoftReference`s. >> Together with the UTF-8 fast-path my UTF-8 encoded file system see >> substantial speed-ups in a trivial `new File(str).toPath()` microbenchmark. > > test/micro/org/openjdk/bench/java/io/FileToPath.java line 46: > >> 44: public String root = "/"; >> 45: public String trailingSlash = "/test/dir/file/name.txt/"; >> 46: public String notNormalizedFile = "/test/dir/file//name.txt"; > > Can be `private`, I think. As long as those are not `static final`... Agree this can be cleaned up. The micro was derived/copied from the `FileOpen` micro in the same package, so comments apply to a pre-existing micro. I'll refactor this to be a subclass inside that micro and clean up both. - PR: https://git.openjdk.java.net/jdk/pull/2135
Re: RFR: 8132984: incorrect type for Reference.discovered [v3]
On Mon, 18 Jan 2021 23:42:08 GMT, Kim Barrett wrote: >> Please review this change which fixes the type of the private >> Reference.discovered field. It was Reference, but that's wrong because >> it can be any Reference object. >> >> I've changed it to Reference and let that flow through, updating some >> other variables that were previously somewhat incorrectly typed (usually >> with an Object type parameter). The interesting change is to the >> ReferenceQueue.enqueue parameter, which is now also Reference. >> >> This ultimately end up with a provably safe and correct, but uncheckable, >> cast in ReferenceQueue.enqueue. >> >> An alternative might be to use a raw type for the discovered field, but I >> think that ends up with more @SuppressWarnings of various flavors. I think >> the unbounded wildcard approach is clearer and cleaner. >> >> Note that all of the pending list handling, including the discovered field, >> could be moved into a non-public, non-generic, sealed(?) base class of >> Reference. The pending list handling has nothing to do with the generic >> parameter T. >> >> Testing: >> mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > plevart improvement This looks good. - Marked as reviewed by plevart (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1897
Re: RFR: 8259947: (fs) Optimize UnixPath.encode implementation
On Tue, 19 Jan 2021 00:35:51 GMT, Claes Redestad wrote: > This patch improves `UnixPath.encode` by reusing `JLA.getBytesNoRepl` (which > has fast-paths for common encoding) and avoiding a `toCharArray` call on the > input by refactoring the `normalizeNativePath` code to operate on `String`. > This might have a cost on files on Mac that need additional native > normalization. > > This removes another `ThreadLocal` and a source of `SoftReference`s. Together > with the UTF-8 fast-path my UTF-8 encoded file system see substantial > speed-ups in a trivial `new File(str).toPath()` microbenchmark. Changes requested by shade (Reviewer). test/micro/org/openjdk/bench/java/io/FileToPath.java line 53: > 51: bh.consume(new File(normalFile).toPath()); > 52: bh.consume(new File(trailingSlash).toPath()); > 53: bh.consume(new File(root).toPath()); No singular test for `new File(root)`, but here it is in the `mix` anyway? What would probably be not comparable. test/micro/org/openjdk/bench/java/io/FileToPath.java line 46: > 44: public String root = "/"; > 45: public String trailingSlash = "/test/dir/file/name.txt/"; > 46: public String notNormalizedFile = "/test/dir/file//name.txt"; Can be `private`, I think. As long as those are not `static final`... - PR: https://git.openjdk.java.net/jdk/pull/2135
Re: RFR: 8259947: Optimize UnixPath.encode
On Tue, 19 Jan 2021 00:35:51 GMT, Claes Redestad wrote: > This patch improves `UnixPath.encode` by reusing `JLA.getBytesNoRepl` (which > has fast-paths for common encoding) and avoiding a `toCharArray` call on the > input by refactoring the `normalizeNativePath` code to operate on `String`. > This might have a cost on files on Mac that need additional native > normalization. > > This removes another `ThreadLocal` and a source of `SoftReference`s. Together > with the UTF-8 fast-path my UTF-8 encoded file system see substantial > speed-ups in a trivial `new File(str).toPath()` microbenchmark. I think that this looks good ( I had a similar thought when looking through this code recently, for a separate issue ). - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2135
Re: RFR: 8132984: incorrect type for Reference.discovered [v3]
On Mon, 18 Jan 2021 23:42:08 GMT, Kim Barrett wrote: >> Please review this change which fixes the type of the private >> Reference.discovered field. It was Reference, but that's wrong because >> it can be any Reference object. >> >> I've changed it to Reference and let that flow through, updating some >> other variables that were previously somewhat incorrectly typed (usually >> with an Object type parameter). The interesting change is to the >> ReferenceQueue.enqueue parameter, which is now also Reference. >> >> This ultimately end up with a provably safe and correct, but uncheckable, >> cast in ReferenceQueue.enqueue. >> >> An alternative might be to use a raw type for the discovered field, but I >> think that ends up with more @SuppressWarnings of various flavors. I think >> the unbounded wildcard approach is clearer and cleaner. >> >> Note that all of the pending list handling, including the discovered field, >> could be moved into a non-public, non-generic, sealed(?) base class of >> Reference. The pending list handling has nothing to do with the generic >> parameter T. >> >> Testing: >> mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > plevart improvement Looks good to me! - Marked as reviewed by rkennke (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1897
Re: Performance regression in BuiltinClassLoader?
Since it's the exploded jdk build, why not `make ejb`?! Only half joking: it's short and easy to remember for those who prefer just building the exploded image - while not setting up trip wires for newcomers and the unwary. /Claes On 2021-01-19 06:48, Thomas Stüfe wrote: Renaming that thing would make sense. It tripped me up too when I was new to OpenJDK. ..Thomas On Mon, Jan 18, 2021 at 9:07 PM Claes Redestad mailto:claes.redes...@oracle.com>> wrote: No problem :-) I've been advocating for renaming the /jdk intermediary into something that would make it perfectly obvious to newcomers that _this is not it_, but I keep getting shot down. Short name convenient! /Claes On 2021-01-18 20:53, Eirik Bjørsnøs wrote: > Alan, > > Apologies for wasting everyone's time (my own included, although I learned > a lot!) > > I found images/jdk, and with that there is no regression. > > Back to square one :-) > > Thanks, > Eirik. > > > On Mon, Jan 18, 2021 at 8:35 PM Eirik Bjørsnøs mailto:eir...@gmail.com>> wrote: > >> >> Alan, >> >> I have been using "make images" all along. This >> produces build/macosx-x86_64-server-release/jdk/modules with unpacked >> modules. >> >> I'm a bit confused since "make help" seems to indicate that "make jdk" >> should create unpacked modules, while "make images" should perhaps not? Or >> did I misunderstand? >> >> Eirik. >> >> >> >> On Mon, Jan 18, 2021 at 8:31 PM Alan Bateman mailto:alan.bate...@oracle.com>> >> wrote: >> >>> On 18/01/2021 19:24, Eirik Bjørsnøs wrote: For good measure, I did a JFR recording which revealed that ExplodedModuleReader was doing file stat in 263 of 277 native method samples. Which lie explains all this, since the 15 I used was not shipped with exploded jmods.. How do I build OpenJDK with packaged modules? >>> Have you done "make images"? You should see images/jdk in your build >>> output. >>> >>> -Alan >>> >>
Re: RFR: 8259867: Move encoding checks into ZipCoder
On Sat, 16 Jan 2021 17:49:38 GMT, eirbjo wrote: > ZipFile.Source.initCEN verifies that entry names are encoding into bytes > valid in the entry's encoding. It does so by calling encoding-specific > checking methods, so it also needs to determine which check method to call > for each entry. > > By moving the encoding-variant checks into ZipCoder, initCEN can instead > simply call ZipCoder.checkEncoding. This makes the code easier to follow and > also removes a duplication of flag checking logic found in zipCoderForPos. Welcome back! Patch looks good to me. I'll sponsor it after allowing some time for other reviewers to have a look. - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2110
